Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Json serializer type discovery #18
Json serializer type discovery #18
Changes from all commits
1c6fc68
4129ba4
ba611d8
290b0d9
487aeef
da676fa
b9e7c48
cace916
7951012
a35baa6
97988ef
c2edfb2
1bc8b16
a79f9b8
38f6526
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The trouble with this approach is that you can't guarantee that the attribute name will actually be called
"JsonSerializable"
For example, here are a bunch of ways I can rename the attribute https://sharplab.io/#v2:EYLgxg9gTgpgtADwGwBYA0AXEBDAzhgHwAEAmARgFgAoIgBgAIBBAOwgwAsYoA5bAWxi4ADtjAx6AXnpEyAbmp16AZQgCOAS2YBzAKIAbXDB3MM62HoCek6WQB0Sruux71AL2zA9MRhgxR1wACuGDDyVNQA2g7+zm4eXgC6CgDM0iRM9ADeAL7UkTL2jrHunt6+/kEhSTSppPQAQlm54VQRLGycPPyCImKFMS4liSlp9ADCTXmtKmrsmroGRiZmMJbVRLXpACJNQA===
Basically when dealing with syntax, you can only make guesses at things, not full decisions. You won't be able to actually know if the same attribute without doing binding. That's what the compiler does when you ask for a semantic model (it's not a trivial operation ;))
The way the other 'attribute based' generators solve this is by just selecting candidates for inclusion (basically anything that has an attribute):
https://github.com/dotnet/roslyn-sdk/blob/94087f6f1d5414ca187d8836caf7c8a114f22978/samples/CSharp/SourceGenerators/SourceGeneratorSamples/AutoNotifyGenerator.cs#L176
Then doing the actual selection during generation. https://github.com/dotnet/roslyn-sdk/blob/94087f6f1d5414ca187d8836caf7c8a114f22978/samples/CSharp/SourceGenerators/SourceGeneratorSamples/AutoNotifyGenerator.cs#L50
As your attribute is provided via references (not injected directly via the generator) you won't need to do the 'create a new compilation' step you'll see above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc @AaronRobinsonMSFT https://github.com/dotnet/runtimelab/blob/DllImportGenerator/DllImportGenerator/DllImportGenerator/DllImportGenerator.cs#L210 seems to have the same issues.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jkotas Not really. I handle all those cases. I ensure the name ends with
GeneratedDllImportAttribute
orGeneratedDllImport
. This handles all the cases in that example seeruntimelab/DllImportGenerator/DllImportGenerator/DllImportGenerator.cs
Lines 188 to 193 in 35c9adc
This doesn't handle the other case of
TrickGeneratedDllImportAttribute
though. Still on the fence whether it is worth addressing based on the cost of building up the semantic model where it is being done.