Ensure consistent scheme for unspeakable names.#45930
Ensure consistent scheme for unspeakable names.#45930AlekseyTs merged 2 commits intodotnet:masterfrom
Conversation
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
I will add one test per name in one of the future PRs.
Actually, I am going to do this in this PR. #Resolved
|
Thanks! |
Closes dotnet#45564. Related to dotnet#45110.
|
@cston, @jcouv, @RikkiGibson, @333fred, @dotnet/roslyn-compiler Please review. |
|
@cston, @RikkiGibson, @333fred, @dotnet/roslyn-compiler Please review, need a second sign-off. |
|
@cston, @RikkiGibson, @333fred, @dotnet/roslyn-compiler Please review, need a second sign-off on a very small product change. |
|
|
||
| // internal until we settle on this long-term | ||
| internal const string CloneMethodName = "<>Clone"; | ||
| internal const string CloneMethodName = "<Clone>$"; |
There was a problem hiding this comment.
$ [](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
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
@tmat, should the names be "<>Clone", "<>Program", and "<>Main" instead to be more consistent with other generated names?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
we would need to update the condition here to include $
Why include "$" in the names?
There was a problem hiding this comment.
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?
| var expectedMembers = new[] | ||
| { | ||
| "C C.<>Clone()", | ||
| "C C." + WellKnownMemberNames.CloneMethodName + "()", |
There was a problem hiding this comment.
." + WellKnownMemberNames.CloneMethodName + "()" [](start = 20, length = 48)
Please consider using "<Clone>$" explicitly, here and throughout the tests. #Resolved
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
You mean before we release? We can't rename these symbols once we ship.
There was a problem hiding this comment.
(I guess we can the Clone one whose WellKnownMembers name is internal, not the others)
|
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 |
There are no plans to do anything about that component at this point. In reply to: 658338515 [](ancestors = 658338515) |
Closes #45564.
Related to #45110.