Skip to content

fix: add diagnose when getter is not as accessible as the setter #2801

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Nov 17, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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");