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 all commits
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
6 changes: 5 additions & 1 deletion gh-pages/content/language-support/assembly.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,4 +167,8 @@ Methods that are `static` cannot feature the `overrides` attribute, as `static`
Properties that are `const` must have a `name` that is `UPPER_SNAKE_CASED`. They represent constants similar to [Enum
Members], which can be proactively resolved by the `jsii` runtimes.

Properties that are `static` cannot feature the `overrides` attribute, as `static` members are not inherited.
!!! danger
Properties and methods that are `static` **can** feature the `overrides` attribute, as `static` members are
inherited with the prototype in **JavaScript** (as part of the ES6 specification). Not all target languages have
this capability (most, like **C#** and **Java**, only support *hiding* static declarations), and consequently code
generators may ignore this (or explicitly hide parent declarations) instead.
33 changes: 33 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,36 @@ 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 carry statics in the inheritance chain at all,
* so they cannot be overridden, only hidden.
*
* The difference is fairly minor (for typical use-cases, the end result is the
* same), however this has implications on what the generated code should look
* like.
*/
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();
}
}
98 changes: 97 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,102 @@
}
]
},
"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": 2863
},
"methods": [
{
"docs": {
"stability": "stable"
},
"locationInModule": {
"filename": "lib/compliance.ts",
"line": 2868
},
"name": "method",
"overrides": "jsii-calc.StaticHelloParent",
"static": true
}
],
"name": "StaticHelloChild",
"properties": [
{
"docs": {
"stability": "stable"
},
"immutable": true,
"locationInModule": {
"filename": "lib/compliance.ts",
"line": 2864
},
"name": "property",
"overrides": "jsii-calc.StaticHelloParent",
"static": true,
"type": {
"primitive": "number"
}
}
]
},
"jsii-calc.StaticHelloParent": {
"assembly": "jsii-calc",
"docs": {
"remarks": "The difference is fairly minor (for typical use-cases, the end result is the\nsame), however this has implications on what the generated code should look\nlike.",
"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 carry statics in the inheritance chain at all, so they cannot be overridden, only hidden."
},
"fqn": "jsii-calc.StaticHelloParent",
"initializer": {
"docs": {
"stability": "stable"
}
},
"kind": "class",
"locationInModule": {
"filename": "lib/compliance.ts",
"line": 2854
},
"methods": [
{
"docs": {
"stability": "stable"
},
"locationInModule": {
"filename": "lib/compliance.ts",
"line": 2859
},
"name": "method",
"static": true
}
],
"name": "StaticHelloParent",
"properties": [
{
"docs": {
"stability": "stable"
},
"immutable": true,
"locationInModule": {
"filename": "lib/compliance.ts",
"line": 2855
},
"name": "property",
"static": true,
"type": {
"primitive": "number"
}
}
]
},
"jsii-calc.Statics": {
"assembly": "jsii-calc",
"docs": {
Expand Down Expand Up @@ -14345,5 +14441,5 @@
}
},
"version": "0.0.0",
"fingerprint": "XfCnzPEGbEJR6hhBX8zWyFzWJP2wH9ztW2ZcW6Wdb+4="
"fingerprint": "8y6c87ScX7dJInuohtU3oLM8TCUz8yoYZ+j14fnHG9s="
}
23 changes: 14 additions & 9 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,14 @@ export class DotNetGenerator extends Generator {
overrides = true;
}
}
if (method.static) {
staticKeyWord = 'static ';
} else 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 ';
if (overrides) {
// Add the override key word if the method is emitted for a proxy or data type or is defined on an ancestor. If
// the member is static, use the "new" keyword instead, to indicate we are intentionally hiding the ancestor
// declaration (as C# does not inherit statics, they can be hidden but not overridden). The "new" keyword is
// optional in this context, but helps clarify intention.
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 @@ -832,8 +834,11 @@ export class DotNetGenerator extends Generator {
prop,
);
if (implementedInBase || datatype || proxy) {
// Override if the property is in a datatype or proxy class or declared in a parent class
isOverrideKeyWord = 'override ';
// Override if the property is in a datatype or proxy class or declared in a parent class. If the member is
// static, use the "new" keyword instead, to indicate we are intentionally hiding the ancestor declaration (as
// C# does not inherit statics, they can be hidden but not overridden).The "new" keyword is optional in this
// context, but helps clarify intention.
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 +850,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
92 changes: 92 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.

Loading