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

Options source generator shouldn't use random names #90990

Closed
eerhardt opened this issue Aug 23, 2023 · 8 comments
Closed

Options source generator shouldn't use random names #90990

eerhardt opened this issue Aug 23, 2023 · 8 comments
Assignees
Labels
area-Extensions-Options source-generator Indicates an issue with a source generator feature
Milestone

Comments

@eerhardt
Copy link
Member

eerhardt commented Aug 23, 2023

string suffix = $"_{new Random().Next():X8}";
_staticValidationAttributeHolderClassName += suffix;
_staticValidatorHolderClassName += suffix;

Generated code should be deterministic. Many companies and orgs have requirements around deterministic builds.

          Is this the code that prompted https://github.com/dotnet/roslyn-analyzers/pull/6882/? Should we change this code to not use Random, and instead be deterministic?

cc @jaredpar

Originally posted by @eerhardt in #89148 (comment)

@dotnet-issue-labeler dotnet-issue-labeler bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Aug 23, 2023
@ghost ghost added the untriaged New issue has not been triaged by the area owner label Aug 23, 2023
@eerhardt eerhardt added area-Extensions-Options source-generator Indicates an issue with a source generator feature and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Aug 23, 2023
@ghost
Copy link

ghost commented Aug 23, 2023

Tagging subscribers to this area: @dotnet/area-extensions-options
See info in area-owners.md if you want to be subscribed.

Issue Details

Generated code should be deterministic. Many companies and orgs have requirements around deterministic builds.

          Is this the code that prompted https://github.com/dotnet/roslyn-analyzers/pull/6882/? Should we change this code to not use Random, and instead be deterministic?

cc @jaredpar

Originally posted by @eerhardt in #89148 (comment)

Author: eerhardt
Assignees: -
Labels:

untriaged, area-Extensions-Options, source-generator, needs-area-label

Milestone: -

@tarekgh tarekgh added this to the 8.0.0 milestone Aug 23, 2023
@tarekgh tarekgh self-assigned this Aug 23, 2023
@ghost ghost removed the untriaged New issue has not been triaged by the area owner label Aug 23, 2023
@tarekgh
Copy link
Member

tarekgh commented Aug 23, 2023

@jaredpar what is the suggestion here? We need to have a class name that is global internal and not conflict when two independent compilations using the generator and one of them is internal visible to the other one. The source gen supports Lang versions 8+. We already detect if using Lang Version 11+ we use file keyword and not using random at all.

@stephentoub
Copy link
Member

what is the suggestion here?

Incorporate a hash of more data, like the assembly's name.

Or use C#11 and/or don't use InternalsVisibleTo.

@tarekgh
Copy link
Member

tarekgh commented Aug 23, 2023

Or use C#11 and/or don't use InternalsVisibleTo.

I want to avoid restricting the language version only because of that. I am not sure what do you mean by don't use InternalsVisibleTo.? It is the user who is using it and not the generator. Do you mean we shouldn't support this scenario? This is a real scenario we got from R9.

Incorporate a hash of more data, like the assembly's name.

This looks like a reasonable idea to try. Thanks!

@jaredpar
Copy link
Member

I want to avoid restricting the language version only because of that.

I would start with requiring C# 11 and wait to see if users push back. Other generators like the regex generator have set that standard and, to my understanding at least, that has not been a problem thus far. I would not invest in making my generator more complex to support this scenario unless I had data saying that would be valuable.

@tarekgh
Copy link
Member

tarekgh commented Aug 23, 2023

@geeknoid @stephentoub @eerhardt would you be ok if we require Language version 11+ for the options source gen?

CC @ericstj

@stephentoub
Copy link
Member

. I am not sure what do you mean by don't use InternalsVisibleTo.? It is the user who is using it and not the generator. Do you mean we shouldn't support this scenario?

I mean if incorporating assembly name is insufficient and you absolutely need IVT and you're using the options generator in two assemblies connected by IVT and you're using a language version less than 11, bump it to 11. This is why file exists.

@ghost ghost added in-pr There is an active PR which will close this issue when it is merged and removed in-pr There is an active PR which will close this issue when it is merged labels Aug 31, 2023
@tarekgh
Copy link
Member

tarekgh commented Sep 1, 2023

This is fixed by #91432

@tarekgh tarekgh closed this as completed Sep 1, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Oct 1, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-Extensions-Options source-generator Indicates an issue with a source generator feature
Projects
None yet
Development

No branches or pull requests

4 participants