fix(pacmak): illegal static overrides in java & c# #2373
Conversation
RomainMuller
added
effort/small
RomainMuller
force-pushed
the
rmuller/static-homonyms
branch
from
57743b8
to
920a4d0
Compare
Stop emitting "overrides" in the respecting language idiomatic way for static methods and properties (while ES6 supports those, this is not true of Java and C#). In Java, this is mostly getting rid of `@Overrides` on static declarations. C# *allows* (but does not require) explicitly hiding the parent declaration using the `new` keyword. This was introduced as it provides additional safeguards against our generating incorrect code. Fixes #2358
RomainMuller
force-pushed
the
rmuller/static-homonyms
branch
from
920a4d0
to
db6376d
Compare
There was a problem hiding this comment.
Looks good to me, mostly "take it or leave it" comments, approved and added do not merge (we really need an emoji for this type of comments).
I initially thought we should implements this fix by emitting the override annotation for the assembly file, but Im guessing it will be less ideal since we will be losing important information?
Maybe we can formalize this behavior in the jsii spec level? Adding a static-overrides section in the assembly?
| } else if ( | ||
| !method.static && |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
RomainMuller
removed
the
pr/do-not-merge
|
Thank you for contributing! ❤️ I will now look into making sure the PR is up-to-date, then proceed to try and merge it! |
|
Merging (with squash)... |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
Merging (with squash)... |
Stop emitting "overrides" in the respecting language idiomatic way for
static methods and properties (while ES6 supports those, this is not
true of Java and C#).
In Java, this is mostly getting rid of
@Overrideson staticdeclarations. C# allows (but does not require) explicitly hiding the
parent declaration using the
newkeyword. This was introduced as itprovides additional safeguards against our generating incorrect code.
Fixes #2358
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.