Skip to content

Commit

Permalink
fix: add diagnose when getter is not as accessible as the setter (#2801)
Browse files Browse the repository at this point in the history
  • Loading branch information
HerrCai0907 authored Nov 17, 2023
1 parent 5ee17be commit 41f395a
Show file tree
Hide file tree
Showing 5 changed files with 44 additions and 22 deletions.
2 changes: 1 addition & 1 deletion src/diagnosticMessages.json
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,6 @@
"Operator '{0}' cannot be applied to types '{1}' and '{2}'.": 2365,
"A 'super' call must be the first statement in the constructor.": 2376,
"Constructors for derived classes must contain a 'super' call.": 2377,
"Getter and setter accessors do not agree in visibility.": 2379,
"'get' and 'set' accessor must have the same type.": 2380,
"Overload signatures must all be public, private or protected.": 2385,
"Constructor implementation is missing.": 2390,
Expand Down Expand Up @@ -200,6 +199,7 @@
"Duplicate property '{0}'.": 2718,
"Property '{0}' is missing in type '{1}' but required in type '{2}'.": 2741,
"Type '{0}' has no call signatures.": 2757,
"Get accessor '{0}' must be at least as accessible as the setter.": 2808,
"This member cannot have an 'override' modifier because it is not declared in the base class '{0}'.": 4117,

"File '{0}' not found.": 6054,
Expand Down
18 changes: 18 additions & 0 deletions src/program.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3088,6 +3088,13 @@ export abstract class Element {
return (this.flags & vis) == (other.flags & vis);
}

visibilityNoLessThan(other: Element): bool {
if (this.isPublic) return true; // public is a most frequent case
if (this.is(CommonFlags.Private)) return other.is(CommonFlags.Private);
if (this.is(CommonFlags.Protected)) return other.isAny(CommonFlags.Private | CommonFlags.Protected);
return assert(false);
}

/** Tests if this element is bound to a class. */
get isBound(): bool {
let parent = this.parent;
Expand Down Expand Up @@ -4174,6 +4181,17 @@ export class Property extends VariableLikeElement {
get isField(): bool {
return this.prototype.isField;
}

checkVisibility(diag: DiagnosticEmitter): void {
let propertyGetter = this.getterInstance;
let propertySetter = this.setterInstance;
if (propertyGetter && propertySetter && !propertyGetter.visibilityNoLessThan(propertySetter)) {
diag.errorRelated(
DiagnosticCode.Get_accessor_0_must_be_at_least_as_accessible_as_the_setter,
propertyGetter.identifierNode.range, propertySetter.identifierNode.range, propertyGetter.identifierNode.text
);
}
}
}

/** A resolved index signature. */
Expand Down
22 changes: 1 addition & 21 deletions src/resolver.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3384,7 +3384,6 @@ export class Resolver extends DiagnosticEmitter {
// Resolve instance members
let prototype = instance.prototype;
let instanceMemberPrototypes = prototype.instanceMembers;
let properties = new Array<Property>();
if (instanceMemberPrototypes) {
// TODO: for (let member of instanceMemberPrototypes.values()) {
for (let _values = Map_values(instanceMemberPrototypes), i = 0, k = _values.length; i < k; ++i) {
Expand Down Expand Up @@ -3464,26 +3463,6 @@ export class Resolver extends DiagnosticEmitter {
}
}

// Check that property getters and setters match
for (let i = 0, k = properties.length; i < k; ++i) {
let property = properties[i];
let propertyGetter = property.getterInstance;
if (!propertyGetter) {
this.error(
DiagnosticCode.Property_0_only_has_a_setter_and_is_missing_a_getter,
property.identifierNode.range, property.name
);
} else {
let propertySetter = property.setterInstance;
if (propertySetter && !propertyGetter.visibilityEquals(propertySetter)) {
this.errorRelated(
DiagnosticCode.Getter_and_setter_accessors_do_not_agree_in_visibility,
propertyGetter.identifierNode.range, propertySetter.identifierNode.range
);
}
}
}

if (instance.kind != ElementKind.Interface) {

// Check that all required members are implemented
Expand Down Expand Up @@ -3707,6 +3686,7 @@ export class Resolver extends DiagnosticEmitter {
}
}
}
instance.checkVisibility(this);
return instance;
}

Expand Down
8 changes: 8 additions & 0 deletions tests/compiler/getter-setter-errors.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
{
"asc_flags": [
],
"stderr": [
"TS2808: Get accessor 'm2' must be at least as accessible as the setter.",
"EOF"
]
}
16 changes: 16 additions & 0 deletions tests/compiler/getter-setter-errors.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
class GetSetWithoutDifferenceVisibility {
public get m1(): i32 {
return 1;
}
private set m1(v: i32) {}

private get m2(): i32 {
return 1;
}
public set m2(v: i32) {}
}

new GetSetWithoutDifferenceVisibility().m1; // m1 is valid
new GetSetWithoutDifferenceVisibility().m2; // m2 is invalid

ERROR("EOF");

0 comments on commit 41f395a

Please sign in to comment.