-
Notifications
You must be signed in to change notification settings - Fork 10.4k
Add initial support for parameters with AsParameters
attribute
#47914
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
Merged
Merged
Changes from all commits
Commits
Show all changes
18 commits
Select commit
Hold shift + click to select a range
107e1d6
Migrate some AsParmaeters tests to shared infra
captainsafia ed7aa5c
Add initial support for AsParameters
captainsafia d48b77e
Get migrated tests working
captainsafia 8253822
Polish
captainsafia b317c60
Update baselines after rebase
captainsafia 3097baa
Move helper types to separate file
captainsafia 4d5477f
Address feedback from peer review
captainsafia 0f9c534
Update TryGetAttributeImplementingInterface references in Analyzers
captainsafia 26d174a
Emit diagnostics and add PropertyAsParameterInfo implementation
captainsafia 001e108
Fix shared code references in analyzers and address feedback
captainsafia 4209c95
Fix requiresPropertyAsParameterInfo and address feedback
captainsafia 0b11555
Seal generated classes
captainsafia af47a20
Merge remote-tracking branch 'origin/main' into safia/rdg-asparams
captainsafia 2884635
Merge remote-tracking branch 'origin/main' into safia/rdg-asparams
captainsafia 05f1f04
Update diagnostics list and baselines
captainsafia fd18c62
Merge remote-tracking branch 'origin/main' into safia/rdg-asparams
captainsafia cb599f1
Update emitter context to fix tests
captainsafia c8fe848
Address feedback from peer review
captainsafia File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
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.
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.
I'm not super excited about this. Do we Assert in other generated code already?
It feels like we are doing something wrong. Can the user actually get into this situation? And if they can, do they know what to do to get out of it?
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.
Ha! @mitchdenny and I had a similar conversation about this yesterday.
We introduced this assertion here in b138bde. I'm not aware of the use
Debug.Assert
in other generated code (like the runtime's generators) but I'll attempt to justify why I think this is appropriate here.Nope, not if they are using RDG.
The nullable parameters in the API signatures of
MetadataPopulator
andRequestDelegateFactoryFunc
exists primarily because theRequestDelegateFactory
has public APIs that allow uses to construct delegates and populate metadata standalone without going through our endpoint resolution logic (see this and this. Internally, these APIs use fallbacks if null values are provided for the nullable parameters, as seen here.With the generated code, the more specific
Map
overloads can only be called during endpoint resolution when RequestDelegateFactoryOptions are guaranteed to be set for route handlers (ref, ref).If there's every a scenario where null values are passed to the generated code, that means we did something wrong in our framework. There's not really anything the user can do to address that issue. IMO, the
Debug.Assert
here reflects that this is bookkeeping we're responsible for as framework authors, not a nullability requirement that the end-user violated.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.
Would it make sense to just disable nullability checking in these files?
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 nullability checks provide value when executing the requiredness checks that we have built in as part of parameter binding. Minimal APIs has a feature where in if you don't annotate a type as nullable, code-gen some logic that validates that it is provided (kind of like the
Required
attribute inSystem.ComponentModel.DataAnnotations
). For example, the code below will bind theage
value from the route parameter but check that the route parameter has been provided before invoking the users handler.Being able to leverage nullable flow analysis in generated code to ensure that we've done the appropriate requiredness checks is more valuable than not having it for the sake of avoiding two assertions, IMO.
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.
I would just
!
away the nullability checks on this one line:If the user built for Release and somehow got into a
null
situation here, the behavior is the same. If they are in Debug and get asserts in here, they have no idea what to do either. Either way, it being a bug in our code.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.
Yep. My preference towards using
Debug.Assert
vs. the null forgiving operator is thatDebug.Assert
is that we fail upfront in our debug builds if we're in an invalid state instead of waiting until theoptions
are used. It makes more sense to be to alert about this inconsistency earlier.Also, for users reading the generated code when in debug mode, I think the
Debug.Assert
communicates more clearly what is going on here than the null-forgiving operator (which personally gives me the ick when I see it in code that I don't own and don't understand why the developer made that guarantee).