search

dotnet / roslyn

The Roslyn .NET compiler provides C# and Visual Basic languages with rich code analysis APIs.
https://docs.microsoft.com/dotnet/csharp/roslyn-sdk/
MIT License
18.91k stars 4.01k forks source link

Compiler and IDE do not gracefully handle a generator being added twice #56619

Open jasonmalinowski opened 2 years ago

jasonmalinowski commented 2 years ago

If a project has the same generator added twice, the compiler may assert here:

https://github.com/dotnet/roslyn/blob/2eddced21a8d9dcfea0509dec368be07644b4302/src/Compilers/Core/Portable/SourceGeneration/GeneratorDriver.cs#L32

And the IDE may start throwing exceptions here:

https://github.com/dotnet/roslyn/blob/2eddced21a8d9dcfea0509dec368be07644b4302/src/Workspaces/Core/Portable/Workspace/Solution/SolutionState.CompilationTracker.cs#L902

The compiler's assert is simply that this shouldn't happen; the IDE breaks because we generate DocumentId GUIDs as a stable hash of the generator identity (assembly/type name) and then the hint paths; if a generator is added twice then we'll collide that way too,

jinujoseph commented 2 years ago

cc @jaredpar for info

jaredpar commented 2 years ago

This is in the context of a single compilation correct? Essentially if the same generator is registered twice via /analyzer calls to the same DLL in different locations?

jasonmalinowski commented 2 years ago

@jaredpar Yes.

chsienki commented 2 years ago

Chatted with @jasonmalinowski offline about this.

There are two options:

  1. We make the generator driver contract explicit, and only allow a single instance per driver
  2. We handle multiple copies of the same type

Generally it doesn't make much sense to have the same generator twice: it's going to just generate the same code and you'll end up with a compilation failure. However, from an API perspective, its natural to want to be able to give two different instances of the same generator with differing behaviors. We essentially already emulate this via inheritance here: https://github.com/dotnet/roslyn/blob/342907828c55cded1fdd7e60f37c0927ccaf75c1/src/Compilers/Test/Core/SourceGeneration/TestGenerators.cs#L35

Given that, I think we should harden our code to accept the same type twice, but not on any high priority.

jaredpar commented 2 years ago

Agree this isn't a high priority. How hard is it to make this an error though? Essentially explicitly fail if multiple generators of the same type are used? That may give us some better feedback from customers about why they think this is a valid use case and that would help us prioritize us removing the restriction.

Then again, I don't feel too strongly about adding the diagnostic either ...