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.

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

Do not use $Program and $Main for generated top-level code type/method names #45564

Closed
tmat opened this issue Jun 30, 2020 · 8 comments · Fixed by #45930
Closed

Do not use $Program and $Main for generated top-level code type/method names #45564

tmat opened this issue Jun 30, 2020 · 8 comments · Fixed by #45930
Assignees
Milestone

Comments

@tmat
Copy link
Member

tmat commented Jun 30, 2020

$Program and $Main are valid identifiers in EE context, so there might be a potential confusion.

It would be better to use the pattern already established for other generated names in C# compiler:
http://sourceroslyn.io/#Microsoft.CodeAnalysis.CSharp/Symbols/Synthesized/GeneratedNames.cs,25

@tmat
Copy link
Member Author

tmat commented Jun 30, 2020

@AlekseyTs

@AlekseyTs AlekseyTs self-assigned this Jun 30, 2020
@AlekseyTs
Copy link
Contributor

@tmat

$Program and $Main are valid identifiers in EE context, so there might be a potential confusion.

Why would this be a problem? The type and the method are not meant to be referred to by name. Please provide concrete scenario where that would create a problem.

It would be better to use the pattern already established for other generated names in C# compiler

What particular in that pattern gives guarantee to avoid a conflict? Any particular character that EE never uses? Something else?

@tmat
Copy link
Member Author

tmat commented Jun 30, 2020

I do not have a specific scenario. I'm just saying that EE parses $Xyz as an identifier and that we should keep a consistent pattern for generated names. I don't see why we wouldn't, if only to avoid confusion. E.g. as I pointed out GeneratedNames.IsGeneratedMemberName will currently return false for these names, because they don't follow the pattern.

@tmat
Copy link
Member Author

tmat commented Jun 30, 2020

What particular in that pattern gives guarantee to avoid a conflict?

See GeneratedNames.TryParseGeneratedName

<, > characters are used in these names.

@AlekseyTs
Copy link
Contributor

We don't necessarily want them to follow the pattern or be recognized by the Generatedname API(s). In fact, we want them to be available as constants from WellKnownMemberNames so that analyzers and other consumers can get to them. I am fine with changing what characters are used in the names. Would <$Program> and <$Main> work for you?

@tmat
Copy link
Member Author

tmat commented Jun 30, 2020

We don't necessarily want them to follow the pattern or be recognized by the Generatedname API(s). In fact, we want them to be available as constants from WellKnownMemberNames so that analyzers and other consumers can get to them

We can still expose names that use the pattern defined by GeneratedName from WellKnownMemberNames, I don't think there would be any problem with that.

I'm not sure why would we not want GeneratedName to not recognize these special names.

Would <$Program> and <$Main> work for you?

That would work - we could define GeneratedName.Other category for these (currently the category is determined by the character following the closing >). Since there is none GeneratedNames.TryParseGeneratedName will currently return false for these names. But that can be easily changed to return GeneratedName.Other.

BTW, just noticed another new generated name in WellKnownMemberNames:

internal const string CloneMethodName = "<>Clone";

Technically it matches the generated pattern. GeneratedNames.TryParseGeneratedName will return true but with GeneratedNameKind equal to C, which is not in the GeneratedNames enum and probably unintended.

@AlekseyTs
Copy link
Contributor

'''GeneratedName''' is an internal API and I don't want to spend any time on it, unless there is a real-world scenario that is going to be affected. So far, I am not aware of any.

@tmat
Copy link
Member Author

tmat commented Jun 30, 2020

Then I propose we change the names to

<Program>$, <Main>$, <Clone>$

which would require no changes in GeneratedName APIs, other than adding $ to the enum.

UPDATE: Actually, a minor change would be needed in this condition.

@jaredpar jaredpar added this to the 16.8 milestone Jul 10, 2020
@jaredpar jaredpar added the Bug label Jul 10, 2020
AlekseyTs added a commit to AlekseyTs/roslyn that referenced this issue Jul 14, 2020
AlekseyTs added a commit that referenced this issue Jul 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants