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

Ensure consistent scheme for unspeakable names. #45930

Merged
merged 2 commits into from
Jul 14, 2020
Merged

Ensure consistent scheme for unspeakable names. #45930

merged 2 commits into from
Jul 14, 2020

Conversation

AlekseyTs
Copy link
Contributor

Closes #45564.
Related to #45110.

@AlekseyTs AlekseyTs added Area-Compilers PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. labels Jul 12, 2020
tmat
tmat reviewed Jul 13, 2020
View reviewed changes
src/Compilers/CSharp/Test/Semantic/Semantics/AnonymousFunctionTests.cs
@@ -1241,7 +1241,7 @@ void local(Func<nint, nint> fn)
{
Console.WriteLine(fn(0));
}";
VerifyInPreview(source, expectedOutput: "1", metadataName: "$Program.<>c.<$Main>b__0_0", expectedIL: @"
VerifyInPreview(source, expectedOutput: "1", metadataName: WellKnownMemberNames.TopLevelStatementsEntryPointTypeName + ".<>c.<" + WellKnownMemberNames.TopLevelStatementsEntryPointMethodName + ">b__0_0", expectedIL: @"
Copy link
Member

@tmat tmat Jul 13, 2020 •
edited by AlekseyTs
Loading

Choose a reason for hiding this comment

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

WellKnownMemberNames [](start = 71, length = 20)

I think it'd be better if tests did not use these constants. Since the symbol names are effectively public API we would want tests to fail if the names change. #Resolved

Copy link
Contributor Author

@AlekseyTs AlekseyTs Jul 13, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

The names are captured as values for public APIs in the public API files. However, I agree, extra checks are good to have. I will add one test per name in one of the future PRs. #Resolved

Copy link
Contributor Author

@AlekseyTs AlekseyTs Jul 14, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I will add one test per name in one of the future PRs.

Actually, I am going to do this in this PR. #Resolved

@tmat
Copy link
Member

tmat commented Jul 13, 2020

Thanks!

@AlekseyTs AlekseyTs removed the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label Jul 14, 2020
@AlekseyTs AlekseyTs requested review from RikkiGibson, cston, jcouv, 333fred and a team July 14, 2020 00:05
@AlekseyTs AlekseyTs marked this pull request as ready for review July 14, 2020 00:06
@AlekseyTs
Copy link
Contributor Author

@cston, @jcouv, @RikkiGibson, @333fred, @dotnet/roslyn-compiler Please review.

jcouv
jcouv approved these changes Jul 14, 2020
View reviewed changes
Copy link
Member

@jcouv jcouv left a comment

Choose a reason for hiding this comment

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

LGTM Thanks

@jcouv jcouv self-assigned this Jul 14, 2020
@AlekseyTs
Copy link
Contributor Author

@cston, @RikkiGibson, @333fred, @dotnet/roslyn-compiler Please review, need a second sign-off.

@AlekseyTs
Copy link
Contributor Author

@cston, @RikkiGibson, @333fred, @dotnet/roslyn-compiler Please review, need a second sign-off on a very small product change.

cston
cston reviewed Jul 14, 2020
View reviewed changes
src/Compilers/Core/Portable/Symbols/WellKnownMemberNames.cs
@@ -344,6 +344,16 @@ public static class WellKnownMemberNames
public const string SliceMethodName = "Slice";

// internal until we settle on this long-term
internal const string CloneMethodName = "<>Clone";
internal const string CloneMethodName = "<Clone>$";
Copy link
Member

@cston cston Jul 14, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

$ [](start = 49, length = 8)

Is this consistent with other generated names? I think most generated names from the C# compiler are "<id1>id2" where "id1" is optional. The original "<>Clone" seemed consistent. #Resolved

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this consistent with other generated names? I think most generated names from the C# compiler are "id2" where "id1" is optional. The original "<>Clone" seemed consistent.

According to Tomas it is consistent, in fact he suggested making this change. See linked issue.

In reply to: 454453400 [](ancestors = 454453400)

Copy link
Member

Choose a reason for hiding this comment

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

@tmat, should the names be "<>Clone", "<>Program", and "<>Main" instead to be more consistent with other generated names?

Copy link
Member

Choose a reason for hiding this comment

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

The implementation of GeneratedName.TryParseGeneratedName parses [CS$]<[middle]>c[__[suffix]] patterns, where c is a required character.
In order for this method to recognize these names we would need to update the condition here to include $ and add $ to GeneratedNameKind enum, but that can easily be done when needed.

Copy link
Member

@cston cston Jul 14, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

we would need to update the condition here to include $

Why include "$" in the names?

Copy link
Contributor Author

@AlekseyTs AlekseyTs Jul 14, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Why include "$" in the names?

To avoid any possible collisions with other generated names. For example we use <Main> in names for async main scenario. Is presence of the "$" going to cause any problems?

cston
cston reviewed Jul 14, 2020
View reviewed changes
src/Compilers/CSharp/Test/Semantic/Semantics/RecordTests.cs
@@ -336,7 +336,7 @@ record C(int X, int Y)
var actualMembers = comp.GetMember<NamedTypeSymbol>("C").GetMembers().ToTestDisplayStrings();
var expectedMembers = new[]
{
"C C.<>Clone()",
"C C." + WellKnownMemberNames.CloneMethodName + "()",
Copy link
Member

@cston cston Jul 14, 2020 •
edited by AlekseyTs
Loading

Choose a reason for hiding this comment

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

." + WellKnownMemberNames.CloneMethodName + "()" [](start = 20, length = 48)

Please consider using "<Clone>$" explicitly, here and throughout the tests. #Resolved

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please consider using "<Clone>$" explicitly, here and throughout the tests.

I think this is going to create an unnecessary maintainability burden if we decide to rename the method in the future again. For each name I left a test that verifies its actual value.

In reply to: 454516444 [](ancestors = 454516444)

Copy link
Member

@tmat tmat Jul 14, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

You mean before we release? We can't rename these symbols once we ship.

Copy link
Member

Choose a reason for hiding this comment

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

(I guess we can the Clone one whose WellKnownMembers name is internal, not the others)

@333fred
Copy link
Member

333fred commented Jul 14, 2020 •
edited by AlekseyTs
Loading

Do we have to update http://sourceroslyn.io/#Microsoft.CodeAnalysis.CSharp/Symbols/Synthesized/GeneratedNames.cs,317 now, or is that going to be done at a later point? #Resolved

@AlekseyTs
Copy link
Contributor Author

Do we have to update http://sourceroslyn.io/#Microsoft.CodeAnalysis.CSharp/Symbols/Synthesized/GeneratedNames.cs,317 now, or is that going to be done at a later point?

There are no plans to do anything about that component at this point.

In reply to: 658338515 [](ancestors = 658338515)

@AlekseyTs AlekseyTs merged commit e2ed258 into dotnet:master Jul 14, 2020
@ghost ghost added this to the Next milestone Jul 14, 2020
@JoeRobich JoeRobich modified the milestones: Next, 16.8.P1 Jul 20, 2020
@Youssef1313 Youssef1313 mentioned this pull request Sep 30, 2020
Closed
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tmat tmat tmat left review comments

@cston cston cston approved these changes

@jcouv jcouv jcouv approved these changes

@RikkiGibson RikkiGibson Awaiting requested review from RikkiGibson

@333fred 333fred Awaiting requested review from 333fred

Assignees

@jcouv jcouv

Labels
Area-Compilers
Projects
None yet
Milestone
16.8.P1
Development

Successfully merging this pull request may close these issues.

Do not use $Program and $Main for generated top-level code type/method names
6 participants
@AlekseyTs @tmat @333fred @cston @jcouv @JoeRobich