Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix(pacmak): illegal static overrides in java & c# #2373

Merged
merged 5 commits into from
Dec 22, 2020
Merged

fix(pacmak): illegal static overrides in java & c# #2373

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
db6376d
fix(pacmak): illegal static overrides in java & c#
RomainMuller Dec 21, 2020
1c72511
Implemented feedback from @NetaNir.
RomainMuller Dec 22, 2020
d74f5f6
update docs, too
RomainMuller Dec 22, 2020
641db86
fixup test expectations
RomainMuller Dec 22, 2020
4aaac98
Merge branch 'main' into rmuller/static-homonyms
mergify[bot] Dec 22, 2020
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
28 changes: 28 additions & 0 deletions packages/jsii-calc/lib/compliance.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2840,3 +2840,31 @@ export namespace LevelOne {
readonly value: boolean;
}
}

/**
* Static methods that override parent class are technically overrides (the
* inheritance of statics is part of the ES6 specification), but certain other
* languages such as Java do not share this feature.
*/
RomainMuller marked this conversation as resolved.
Show resolved Hide resolved
export class StaticHelloParent {
public static get property(): number {
return 1337;
}

public static method(): void {
/* A static method */
}
}
export class StaticHelloChild extends StaticHelloParent {
public static get property(): number {
return 42;
}

public static method(): void {
/* An ES6 Override */
}

private constructor() {
super();
}
}
97 changes: 96 additions & 1 deletion packages/jsii-calc/test/assembly.jsii
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11422,6 +11422,101 @@
}
]
},
"jsii-calc.StaticHelloChild": {
"assembly": "jsii-calc",
"base": "jsii-calc.StaticHelloParent",
"docs": {
"stability": "stable"
},
"fqn": "jsii-calc.StaticHelloChild",
"kind": "class",
"locationInModule": {
"filename": "lib/compliance.ts",
"line": 2858
},
"methods": [
{
"docs": {
"stability": "stable"
},
"locationInModule": {
"filename": "lib/compliance.ts",
"line": 2863
},
"name": "method",
"overrides": "jsii-calc.StaticHelloParent",
"static": true
}
],
"name": "StaticHelloChild",
"properties": [
{
"docs": {
"stability": "stable"
},
"immutable": true,
"locationInModule": {
"filename": "lib/compliance.ts",
"line": 2859
},
"name": "property",
"overrides": "jsii-calc.StaticHelloParent",
"static": true,
"type": {
"primitive": "number"
}
}
]
},
"jsii-calc.StaticHelloParent": {
"assembly": "jsii-calc",
"docs": {
"stability": "stable",
"summary": "Static methods that override parent class are technically overrides (the inheritance of statics is part of the ES6 specification), but certain other languages such as Java do not share this feature."
},
"fqn": "jsii-calc.StaticHelloParent",
"initializer": {
"docs": {
"stability": "stable"
}
},
"kind": "class",
"locationInModule": {
"filename": "lib/compliance.ts",
"line": 2849
},
"methods": [
{
"docs": {
"stability": "stable"
},
"locationInModule": {
"filename": "lib/compliance.ts",
"line": 2854
},
"name": "method",
"static": true
}
],
"name": "StaticHelloParent",
"properties": [
{
"docs": {
"stability": "stable"
},
"immutable": true,
"locationInModule": {
"filename": "lib/compliance.ts",
"line": 2850
},
"name": "property",
"static": true,
"type": {
"primitive": "number"
}
}
]
},
"jsii-calc.Statics": {
"assembly": "jsii-calc",
"docs": {
Expand Down Expand Up @@ -14345,5 +14440,5 @@
}
},
"version": "0.0.0",
"fingerprint": "XfCnzPEGbEJR6hhBX8zWyFzWJP2wH9ztW2ZcW6Wdb+4="
"fingerprint": "lMjqpro90RI1jZrcELFmEzcSJQCxkEhm77olVYW0fc8="
}
13 changes: 6 additions & 7 deletions packages/jsii-pacmak/lib/targets/dotnet/dotnetgenerator.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -476,7 +476,7 @@ export class DotNetGenerator extends Generator {
const returnType = method.returns
? this.typeresolver.toDotNetType(method.returns.type)
: 'void';
let staticKeyWord = '';
const staticKeyWord = method.static ? 'static ' : '';
let overrideKeyWord = '';
let virtualKeyWord = '';

Expand All @@ -496,12 +496,11 @@ export class DotNetGenerator extends Generator {
overrides = true;
}
}
if (method.static) {
staticKeyWord = 'static ';
} else if (overrides) {
if (overrides) {
// Add the override key word if the method is emitted for a proxy or data type or is defined on an ancestor
overrideKeyWord = 'override ';
overrideKeyWord = method.static ? 'new ' : 'override ';
} else if (
!method.static &&
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this logic be moved to the if (method.overrides) clause above? Reasoning being that we treat static overrides as not overrides, and this can reduce the nesting in the code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have been internally debating this a little bit... Initially I wanted to simply change the compiler so that statics are never considered to be overrides; but this caused problems, because in JavaScript (and hence, TypeScript), they actually are overrides... This manifests when you try the following:

export class Parent { public static foo: number; }

//! Child.foo incorrectly implements Parent.foo (types don't match)
export class Child extends Parent { public static foo: string; }

This made me decide that it was a more coherent story from the author's standpoint if we reproduced the ES6 semantics in the jsii assembly (where statics are inherited with the rest of the prototype). Carrying them as overrides has advantages - in languages where static are inherited (although JS is the only one that comes to mind now), this allows overrides to use super.

We could distinguish static overrides from non-static overrides, however this distinction is very much a code generation concern, and not so much a type model concern (after all, all we care about is whether this is an override or not, period).

(method.abstract || !definedOnAncestor) &&
!emitForProxyOrDatatype
) {
Expand Down Expand Up @@ -833,7 +832,7 @@ export class DotNetGenerator extends Generator {
);
if (implementedInBase || datatype || proxy) {
// Override if the property is in a datatype or proxy class or declared in a parent class
RomainMuller marked this conversation as resolved.
Show resolved Hide resolved
isOverrideKeyWord = 'override ';
isOverrideKeyWord = prop.static ? 'new ' : 'override ';
RomainMuller marked this conversation as resolved.
Show resolved Hide resolved
} else if (prop.abstract) {
// Abstract members get decorated as such
isAbstractKeyword = 'abstract ';
Expand All @@ -845,7 +844,7 @@ export class DotNetGenerator extends Generator {

const propTypeFQN = this.typeresolver.toDotNetType(prop.type);
const isOptional = prop.optional ? '?' : '';
const statement = `${access} ${isAbstractKeyword}${isVirtualKeyWord}${isOverrideKeyWord}${staticKeyWord}${propTypeFQN}${isOptional} ${propName}`;
const statement = `${access} ${isAbstractKeyword}${isVirtualKeyWord}${staticKeyWord}${isOverrideKeyWord}${propTypeFQN}${isOptional} ${propName}`;
this.code.openBlock(statement);

// Emit getters
Expand Down
6 changes: 3 additions & 3 deletions packages/jsii-pacmak/lib/targets/java.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1274,7 +1274,7 @@ class JavaGenerator extends Generator {
if (includeGetter) {
this.code.line();
this.addJavaDocs(prop);
if (overrides) {
if (overrides && !prop.static) {
this.code.line('@Override');
}
this.emitStabilityAnnotations(prop);
Expand Down Expand Up @@ -1307,7 +1307,7 @@ class JavaGenerator extends Generator {
for (const type of setterTypes) {
this.code.line();
this.addJavaDocs(prop);
if (overrides) {
if (overrides && !prop.static) {
this.code.line('@Override');
}
this.emitStabilityAnnotations(prop);
Expand Down Expand Up @@ -1364,7 +1364,7 @@ class JavaGenerator extends Generator {
this.code.line();
this.addJavaDocs(method);
this.emitStabilityAnnotations(method);
if (overrides) {
if (overrides && !method.static) {
this.code.line('@Override');
}
if (method.abstract && !defaultImpl) {
Expand Down
87 changes: 87 additions & 0 deletions packages/jsii-pacmak/test/generated-code/__snapshots__/target-dotnet.test.ts.snap
View file
Open in desktop

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

80 changes: 80 additions & 0 deletions packages/jsii-pacmak/test/generated-code/__snapshots__/target-go.test.ts.snap
View file
Open in desktop

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

Loading