Skip to content

Commit

Permalink
feat(jsii): detect changing visibility when overriding (#1876)
Browse files Browse the repository at this point in the history
Since C# does not allow changing the visibility of a member when
overriding it, added a check to ensure nobody attempts to make a
protected member public when overriding it.

This is a somewhat common pitfall for users (as seen for example in
aws/aws-cdk#9616), and the check will allow faster detection and
correction (i.e: at compilation time instead of during `jsii-pacmak`).
  • Loading branch information
RomainMuller authored Aug 12, 2020
1 parent 42a9823 commit cfc99c2
Show file tree
Hide file tree
Showing 3 changed files with 42 additions and 0 deletions.
16 changes: 16 additions & 0 deletions packages/jsii/lib/validator.ts
Original file line number Diff line number Diff line change
Expand Up @@ -392,6 +392,14 @@ function _defaultValidations(): ValidationFunction[] {
label: string,
action: string,
) {
if (!!expected.protected !== !!actual.protected) {
const expVisibility = expected.protected ? 'protected' : 'public';
const actVisibility = actual.protected ? 'protected' : 'public';
diagnostic(
ts.DiagnosticCategory.Error,
`${label} changes visibility when ${action} (expected ${expVisibility}, found ${actVisibility})`,
);
}
if (!deepEqual(actual.returns, expected.returns)) {
const expType = spec.describeTypeReference(expected.returns?.type);
const actType = spec.describeTypeReference(actual.returns?.type);
Expand Down Expand Up @@ -446,6 +454,14 @@ function _defaultValidations(): ValidationFunction[] {
label: string,
action: string,
) {
if (!!expected.protected !== !!actual.protected) {
const expVisibility = expected.protected ? 'protected' : 'public';
const actVisibility = actual.protected ? 'protected' : 'public';
diagnostic(
ts.DiagnosticCategory.Error,
`${label} changes visibility when ${action} (expected ${expVisibility}, found ${actVisibility})`,
);
}
if (!deepEqual(expected.type, actual.type)) {
const expType = spec.describeTypeReference(expected.type);
const actType = spec.describeTypeReference(actual.type);
Expand Down
6 changes: 6 additions & 0 deletions packages/jsii/test/__snapshots__/negatives.test.js.snap

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

20 changes: 20 additions & 0 deletions packages/jsii/test/negatives/neg.override-changes-visibility.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
// Note: not testing "public -> protected" because this is invalid TypeScript,
// so the type checker will already have caught it for us.

export class BaseClass {
protected readonly property?: string;

protected method() {
return;
}
}

export class ChildClass extends BaseClass {
// This property cannot be public as it overrides the one on BaseClass which is protected
public readonly property?: string;

// This method cannot be public as it overrides the one on BaseClass which is protected
public method() {
throw new Error('Not Implemented');
}
}

0 comments on commit cfc99c2

Please sign in to comment.