Ensure @typeparam directive descriptor is always set #33456
Merged
Conversation
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
pranavkm
approved these changes
Jun 11, 2021
...Microsoft.AspNetCore.Razor.Language/src/Components/ComponentConstrainedTypeParamDirective.cs
Show resolved
Hide resolved
| @@ -100,7 +100,12 @@ protected override CodeTarget CreateTarget(RazorCodeDocument codeDocument, Razor | |||
| { | |||
| @class.BaseType = ComponentsApi.ComponentBase.FullTypeName; | |||
|
|
|||
| var typeParamReferences = documentNode.FindDirectiveReferences(ComponentTypeParamDirective.Directive); | |||
| // Constrained type parameters are only supported in Razor language versions v6.0 | |||
| var razorLanguageVersion = codeDocument.GetParserOptions()?.Version ?? RazorLanguageVersion.Latest; | |||
There was a problem hiding this comment.
Wouldn't we want to assume lowest version instead of highest if there isn't one specified?
There was a problem hiding this comment.
AFAIK, the only scenario where it is undefined is in some test cases for this pass that don't fully configure the ComponentDocumentClassifierPass with parser options. For that scenario, I figured it made sense to update to the latest.
Assuming this happens to a user, I think it also makes sense to default to the latest since the constrained type param directive can still support unconstrained scenarios.
src/Razor/Microsoft.AspNetCore.Razor.Language/src/Components/ComponentTypeParamDirective.cs
Show resolved
Hide resolved
NTaylorMullen
approved these changes
Jun 11, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Fixes #32592 and fixes #32193 and probably also fixes #32237.
The way the
TypeParamDirectivewas instantiated before ran the risk of having an undefinedDirectiveproperty if multiple project engines are instantiated. This is more likely to happen in tests that run in parallel and in our compilation flow.I've cleaned this up so that the correct
TypeParamDirectiveconstruct is produced depending on the language version.