roslyn: Improved UX (error message) for generator users missing partial modifiers
[Update from jcouv]: summary of the thread, the simplest solution would be improve the diagnostic message:
- add something like “did you forget to add the ‘partial’ modifier to this class?”
- and/or add “there are other ‘partial’ parts that were present in generator code”
So, this is an issue that keeps happening, and we’re getting lots of issues in our own repo (for the MVVM Toolkit source generator) from people trying out our generators, forgetting the partial modifier (or simply not knowing it’s needed or that it even exists at all), and then opening issues thinking our generators are broken. For instance, see: https://github.com/CommunityToolkit/dotnet/issues/286.
This to me seems like a very, very common problem that pretty much all authors of source generators creating members in existing types will have to face, so I’m wondering what the correct solution in the long run might be. Wanted to get some thoughts from folks here before going ahead and doing extra work in the MVVM Toolkit to eg. emit additional diagnostics there.
There’s multiple ways we could go about this, that I can imagine:
- Each source generator author should implement additional logic to detect whether a target type (including any types in the tree if the type is nested) are missing the
partialmodifier, and (1) emit a specific diagnostic and (2) maybe stop generation there. - Treat all partial types in source generators as not needing partial in their corresponding type in the project. This has been suggested before but has some drawbacks, namely that it would hide from authors which types are receiving additional generated members.
- Have Roslyn detect cases like this (ie. a non-partial user type that has a partial version generated by a referenced generator), and emit a better error message, informing the user that they’re missing a partial modifier, that the colliding file in the source generator needs that, etc.
To me, option 3 seems like the one that might require the least work and that will benefit everyone.
Note, we already do get a similar diagnostic, specifically this:
But apparently this is not really enough, since people keep stumbling over this and reporting issues.
Would it maybe be possible to customize this error message when Roslyn can see that the partial declaration is from a source generator, to try to give clearer and more helpful instructions to the user (especially if they’re beginners)?
About this issue
- Original URL
- State: open
- Created 2 years ago
- Reactions: 4
- Comments: 55 (50 by maintainers)
Yes, and we’re back at the issue about asking every single SG out there to reimplement all of this every single time. Not to mention this would be inconsistent, because 10 different SGs will have this 10 different ways (and some not at all). Just having the compiler emit a better message seems fine to me. It’s already checking this and emitting a diagnostics. And it clearly has all the information to determine when this is the case versus the user just defining the types themselves.
If we just provide a code fix for this case that adds the
partialmodifier, it would be easy for the user to correct it. Also, we should make sure that if the user hits <kbd>F12</kbd> at the location of the error, they see all files defining the type.The guidance I’ve been giving is source generators generally do not need to provide a diagnostic for any case where the compiler is already going to report a compilation error.
@CyrusNajmabadi On Discord, @333fred suggested that this should work for any files that are considered generated, ie. not just from source generators, but also files that end with .g.cs or have the <auto-generated> comment, so that this would provide a helpful error message even in eg. WinForms, where they can’t provide any custom diagnostics, but it could still be useful for the compiler to tell the user that there’s another partial declaration in a generated file that the user might not know about.
I would rather not put the onus on the SG authors here. I think we can solve it once and for all here. I see 2 action items for us at this point.
but I’m actually not sure that’s as necessary.I edited my post here after reviewing the diagnostic message and replies. Sorry for any confusion.
To me and many others would be, but I should note that:
I feel like part of the issue is that whereas this is arguably a more advanced language feature (yeah I can see that it’s not that complicated, but you get what I mean), you need to consider that lots of users of source generators are really just beginners, just putting together their very first programs or applications, and this is a recurring issue for them. As I’ve said, I keep seeing the same exact issue being raised by early adopters of the new MVVM Toolkit source generators. If we could try to make this first experience on their part easier and better, it would both make their live easier, and also reduce churn on maintainers having to triage and explain the same thing over and over again 🙂
There is no UX defined for providing a code fix for a diagnostic reported by a source generator. If you create a duplicate diagnostic, you’re going to end up with one “clear but not automatically fixable” error and one “unclear but automatically fixable” error. Users will navigate to the former and not be able to quickly correct it.
We coudl jsut say “some sibling parts are in generated files”.
Not really no. I dont see the “churn” aspect. And yes, i woudl expect generators need to check the state of hte world to make sure they’re appropriate for the generation they do. if some author thinks htis is super common and would benefit from a helper (which should be a 1-2 liner), they could always publish an extension.
Doesn’t it seem like a lot of churn and also inevitably inconsistent behavior across all existing generators if we just decide the right approach here is to ask every single source generator author to do this check and emit a custom diagnostic themselves? 😟
While I don’t want to try and argue for more work than is needed, I worry that the underlying issue here is based on people not reading the existing error messages. Will altering/improving the messages make a sufficient difference? I guess it’s going to be necessary to try this and see if it helps. More speculation isn’t going to help without actual data.