fix(jsii): class members named after the class result in illegal C# #1903
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
RomainMuller
added
bug
RomainMuller
commented
Aug 17, 2020
Comment on lines
+98
to
+129
| ### Overriding | ||
|
|
||
| The visibility of a type member cannot be changed when it is overridden, even if | ||
| the change increases the visibility of said member, as this would result in | ||
| illegal **C#** code: | ||
|
|
||
| ```ts | ||
| export class Base { | ||
| protected method(): void { /* ... */ } | ||
| } | ||
|
|
||
| export class Child { | ||
| // ⛔️ Illegal change of visibility when overriding a member | ||
| public method(): void { /* ... */ } | ||
| } | ||
| ``` | ||
|
|
||
| Additionally, **C#** does not allow changing the type signature of members while | ||
| overriding them, even if the updated type signature is a strict specialization | ||
| of the original one, and this is consequently also forbidden in `jsii`: | ||
|
|
||
| ```ts | ||
| export class Base { | ||
| public method(): any { /* ... */ } | ||
| } | ||
|
|
||
| export class Child { | ||
| // ⛔️ Illegal change of signature when overriding a member | ||
| public method(): string { /* ... */ } | ||
| } | ||
| ``` | ||
|
|
There was a problem hiding this comment.
Those restrictions already existed, but were not documented...
eladb
approved these changes
Aug 17, 2020
It is illegal in C# to name a class' member (method, property) with the same name as the class itself (even if the member is static). Since both class and member names are pascal-cased in C#, this means a class' members cannot share the same PascalCased representation. This adds a compile-time validation for this condition, effectively prohibiting member names to have the same PascalCased representation as their declaring class' name. Fixes #1880
RomainMuller
force-pushed
the
rmuller/disallow-member-naled-after-class
branch
from
8e673df
to
88e8c12
Compare
…mber-naled-after-class
RomainMuller
force-pushed
the
rmuller/disallow-member-naled-after-class
branch
from
e3be451
to
5253684
Compare
RomainMuller
force-pushed
the
rmuller/disallow-member-naled-after-class
branch
from
91fc2af
to
c392304
Compare
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
It is illegal in C# to name a class' member (method, property) with the
same name as the class itself (even if the member is static). Since both
class and member names are pascal-cased in C#, this means a class'
members cannot share the same PascalCased representation.
This adds a compile-time validation for this condition, effectively
calling out member names to have the same PascalCased representation
as their declaring class' name.
This is currently only a warning, as this is "made to work" by altering
the type name by appending
_at the end of it, which is ugly anddangerous but works, and is currently done in several places).
As all warnings, this turns into an error when operating under
--strict,and future work (i.e: #1931) will allow more granular configuration
of these errors, so we can hopefully opt all new codebases into the
strict behavior and eventually drop the slugification.
Fixes #1880
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.