Do not report conflicting types errors for source-generated APIs #71546
Conversation
dotnet-issue-labeler
bot
added
Area-Compilers
untriaged
Yes, and threw out that idea immediately as I don't want Dave or Cyrus to come murder me in my sleep. |
|
I am not a fan of this change. I would very much prefer for source generators to have an API with access to metadata references prior to the creation of a |
|
Could you please elaborate @sharwell? |
|
I went into it a bit here: The summary is CS0436 is only getting reported because of a limitation in the |
Sarcasm aside, why is that a problem? Just because it would increase the memory usage? |
I think there is definitely scope to have a post-init context that would allow some form of metadata lookup. Things like parse options would be useful too: (see #71299). We do already re-parse post-init sources in certain circumstances, so it's unlikely to be a huge perf hit. I also wonder if we want to consider a different direction though and make attribute addition more declarative: rather than force users to lookup an attribute and add it if it's missing, we could design an API that effectively says 'add this attribute if it's not there'. Some of the early designs for FAWMN had the same idea, which we could always resurrect too, where you not only say 'find this attribute' but 'add and find this attribute' |
If we did go that direction, it seems like we'd want to do it as a helper API on top of a lower-level API that works for both that and other scenarios. Attributes aren't the only types users may want to add. Another example would be adding |
Yes. If we couldn't fit a bit on SyntaxTree to indicate if it contains any attributes, we can't fit a bit on it to indicate if it's source generated. |
| // IVT assemblies that may also run the same source generator. We don't want to warn in these cases, so we suppress the warning | ||
| // if the source symbol is from a source-generated file. We only do this for types with a single location, since that is the very | ||
| // specific scenario we are targeting. | ||
| if (srcSymbol.Locations is not [{ SourceTree: { } srcTree }] || !Compilation.IsSourceGeneratedTree(srcTree)) |
There was a problem hiding this comment.
It feels wrong to me to tie this to generated code because it doesn't satisfy all the scenarios. Consider:
- A key scenario for source generators is that you can walk away from them. Essentially at any point you can set
<EmitGeneratedCompilerFiles>true, check in the files, remove the generators and move on with life. Tieing the diagnostic to this heuristic means that scenario breaks - This problem comes up frequently in non-generated code. Consider that there are many NuPkg out there which distribute source not DLLs. They have this exact problem and it's not generated code, it's plain old code.
My instinct is to lean towards a solution where a type is marked as
I know there may be multiple definitions of me, so what, pick the source.
That does not help. Consider that there are essentially the following scenarios when a generator is trying to use an attribute (let's call it a. There are zero Knowing the set of metadata references ahead of time only helps with scenarios (a) and (b). For (c) there is no solution other than issuing a diagnostic to tell the user to pick one and attach and |
|
@jaredpar What would scenario C look like after this change? |
|
It sounds like Jared is suggesting that the way to resolve 2 or more types in metadata references would be to define a third type in the current assembly, at which point the compilation prefers that type over all of the types of the same name coming from metadata references. If that is truly the goal, it seems like it could be better resolved by the following:
Source generators that intentionally inject code that hides other code from metadata references can suppress the warning for (2) at the location of that source generated declaration. Users would not be required to have a source generator in order to use this feature. |
This change is being done rationale of providing a simple story to source generator authors. Rather than dig through metadata just emit the attribute every time. The problem with that approach is you end up with the ambiguity warnings. From that perspective B and C are the same scenario cause the compiler will prefer the source definition over the metadata one. All this change does is remove the warning in that case.
No. There is a discussion on going about best practices for generators. The recommended best practice of always generate your attribute falls into the warning trap I've been describing. This PR is an attempt to get sane behavior from the compiler in the face of our desired best practice for source generators.
That is the current behavior, the compiler just emits a warning which is undesirable for the generator case.
The compiler does issue a warning. The problem is the warning is on the consumption of the type, not the definition. That means source generator consumers have to know about this mess which is a bad user experience. We're exploring ways to either remove the warning or put it in a place where generator authors can suppress it so customers don't know about it. |
Yes, this is what I was suggesting as a preferred behavior (relocating the warning to declaration). |
333fred
force-pushed
the
source-gen-warning
branch
from
49ce6e5 to
7d156de
Compare
333fred
force-pushed
the
source-gen-warning
branch
from
7d156de to
ea2ae9d
Compare
|
Closing in favor of #76523. |
It is a common scenario for source generators to make
internalattributes in an assembly via ourPostInitcallbacks. This is mostly fine, but can result in annoying CS0436 "this type conflicts" warnings. To better support this scenario, we suppress this warning for this specific case. In order to plumb this knowledge through, I did have to update a bit larger tail than I would have otherwise preferred, but we now have the building block for implementing #56810 as well.