-
Notifications
You must be signed in to change notification settings - Fork 10.3k
Razor generic parameter cascading #29767
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
Razor generic parameter cascading #29767
Conversation
Still has a lot of missing cases, and is inefficient (causes repeated evaluations).
…ference by creating variables
AFAICT the previous implementation was incorrect to treat child content as something that can "cover" a generic parameter, since it's always equivalent to an untyped lambda (i.e., doesn't specify its own types). It wouldn't have mattered before because you'd always have a different param on the same component providing the type, but now it does matter because we need to know whether to infer from an ancestor.
...eCodeGenerationTest/ChildComponent_Generic_Explicit_OverrideCascade/TestComponent.codegen.cs
Show resolved
Hide resolved
...imeCodeGenerationTest/ChildComponent_Generic_TypeInference_Cascaded/TestComponent.codegen.cs
Show resolved
Hide resolved
...Microsoft.AspNetCore.Razor.Language/test/IntegrationTests/ComponentCodeGenerationTestBase.cs
Show resolved
Hide resolved
AssertCSharpDocumentMatchesBaseline(generated.CodeDocument); | ||
CompileToAssembly(generated); | ||
} | ||
|
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.
Do we have a test for what happens when a parent and a grand parent cascade the same generic type parameter?
Who wins? (I believe the parent wins over the GP, but we should capture that behavior in a test)
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 thinking of stuff like TValue and TItem)
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.
Good point - added.
} | ||
|
||
[Fact] | ||
public void ChildComponent_Generic_TypeInference_CascadedWithUnrelatedGenericType_CreatesDiagnostic() |
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.
We might want to capture some other behaviors here just to make sure we don't unknowingly change them in the future. I believe this test covers the case where the parent cascades more values than the child. There are a few other cases that might be interesting to cover:
- Same number of parameters but different names.
- Child has more generic parameters than the parent.
- Combined ancestors provide all generic parameters for a child component but none of them covers all of them.
- Child has more generic parameters, the parent covers some of them and the rest are explicitly specified.
We might already have some of these tests, but I'm not sure I've seen them covered.
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.
Same number of parameters but different names.
The non-matchingness of different names is covered by ChildComponent_Generic_TypeInference_CascadedWithMultipleTypes
. I don't think we specifically need a test about the number of parameters being the same, as that's not a consequential aspect in either the design or implementation.
Child has more generic parameters than the parent.
This is covered by ChildComponent_Generic_TypeInference_CascadedWithMultipleTypes
Child has more generic parameters, the parent covers some of them and the rest are explicitly specified.
Update: I read this more carefully and realised this is deliberately unsupported. Just like C# in general, we don't allow partial generic type inference. If any of the parameters are specific explicitly, they must all be. I've added a further test to show this.
Combined ancestors provide all generic parameters for a child component but none of them covers all of them.
Good idea. Added.
AssertCSharpDocumentMatchesBaseline(generated.CodeDocument); | ||
|
||
var diagnostic = Assert.Single(generated.Diagnostics); | ||
Assert.Same(ComponentDiagnosticFactory.GenericComponentTypeInferenceUnderspecified.Id, diagnostic.Id); |
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.
It might be good to update this diagnostic message to include the list of considered and rejected candidates. Right now, if type inference fails, there is not a good way to know why.
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.
It would be nice if we had more useful information to give, but I don't see that we do. Candidates are matched on name, so it's not useful to the developer to talk about things with unrelated names - that would be more confusing. Likewise, talking about all the ancestor components would be confusing too because there's no reason to think they should be involved in cascading.
The likely case where someone hits this doesn't involve cascading at all - it's the existing case where someone simply fails to supply a sufficiently covering set of parameters.
If we spot any common patterns in support requests we can certainly try to work out whether they are things we could detect programmatically.
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 was thinking we could list the ancestors in the tree that offer cascading parameter values and their names. That at least gives a hint of what was considered and can help troubleshoot when a parameter is not being inferenced.
The way I see this, is we go through the process of determining if the parameters a component supplies can be applied to a component that receives them for all the ancestors in the tree. At that point, we could record the reason why we think some component exposing cascading values is not a good match for the types that require inference and to display that information along the diagnostic error in case we aren't able to infer the parameters for the child component. Many compilers do this type of thing, as well as some parts of our runtime, like routing.
It helps people solve the issue by themselves in many cases and limits the number of issues we receive because developers don't understand the rules and can't figure out why it's not working.
Something along the lines of:
The type for TItem was not specified and could not be inferred from its ancestors. The following candidates were considered:
- Parent cascades TKey, TValue parameters, but they were rejected because X.
- Grandparent cascades TContext but was rejected because Y.
Either update a component on the ancestor chain to cascade the parameter TItem or specify the attribute explicitly.
Not exactly what I wrote above, but something along the lines.
I know this is more work (believe me, I implemented this for routing) but it greatly increases the "explainability" of the system. If you still don't think it's worth it, at least, let's file an issue to track this improvement and schedule on a future milestone if we receive feedback about this.
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.
It's an honourable thing to try to make better error messages, but I think something along the following lines risks causing a lot more confusion than would exist without it:
The type for TItem was not specified and could not be inferred from its ancestors. The following candidates were considered:
Parent cascades TKey, TValue parameters, but they were rejected because X.
Grandparent cascades TContext but was rejected because Y.
Either update a component on the ancestor chain to cascade the parameter TItem or specify the attribute explicitly.
Talking about cascading generic types in the error makes the developer think that cascading is somehow part of either the problem or the solution. But in the typical case, cascading is completely unrelated to the situation. The most common case is that the component isn't meant to receive the type via a cascade at all, but is meant to receive it via one of its own parameters.
Certainly if we get some patterns of feedback saying that it's hard to understand and we believe we can distinguish those cases where cascading is intended as the solution, then we can consider making an error message that leads the developer to think about that. But let's be careful not to mislead developers in the more common case of just using a normally type-inferred component.
var diagnostic = Assert.Single(generated.Diagnostics); | ||
Assert.Same(ComponentDiagnosticFactory.GenericComponentTypeInferenceUnderspecified.Id, diagnostic.Id); | ||
} | ||
|
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.
Will it make sense to have a test that covers multiple nested levels of type inference?
Something like
<Parent>
<InferredChild>
<SomeOtherInferenceRoot>
<AnotherInferredChild></AnotherInferredChild>
</SomeOtherInferenceRoot>
</InferredChild>
</Parent>
Just to exercise a bit more the code generation logic
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.
Also, along the lines of this. We probably also want a test for this https://github.com/dotnet/aspnetcore/pull/29767/files#r569414863
<InferenceRoot>
<Child></Child>
</InferenceRoot>
<InferenceRoot>
<Child></Child>
</InferenceRoot>
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.
These are now covered by the newly-added ChildComponent_Generic_TypeInference_MultiLayerOverrideCascade
(and partially by ChildComponent_Generic_TypeInference_MultiLayerCascaded
which was there before).
var variableName = $"__typeInferenceArg_{_scopeStack.Depth}_{parameter.ParameterName}"; | ||
context.CodeWriter.Write(variableName); | ||
|
||
TrackCapturedCascadingGenericParameterVariable(node, parameter, variableName); |
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.
nit: Consider a more explicit method name: SubstituteProvidedGenericParameterValueWithCapturingVariable
or something along those lines.
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.
OK. I've gone with UseCapturedCascadingGenericParameterVariable
because it's not just doing a one-time substitution - it's also keeping track of the variable for future use.
@javiercn Could you let me know when you’re done with the review so I can try to address things in one pass? |
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.
Looks great so far!
I have suggested a few additional tests to make sure we capture the behavior for the feature, but I have no complains about the code!
@@ -3152,6 +3152,391 @@ public class MyComponent<TItem> : ComponentBase | |||
CompileToAssembly(generated); | |||
} | |||
|
|||
[Fact] |
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.
We might want to have a test that uses splatting
as well as other features like @key
and so on to validate they are integrated correctly.
It might also be interesting to have a test where we simulate a side effect to make sure the order is preserved.
<Comp Something1="object.SideEffect()" Something2="object" />
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 really skeptical about whether it adds value to snapshot the code generated for an unrelated combination of features, as it's very hard to reason about. We have to be cautious about doing this too much if we want to be able to manage the whole test suite and be able to say with confidence that the baselines mean something and when they should or shouldn't change. @key
and splatting are really orthogonal to cascaded generic type inference.
Nonetheless I'm willing to add one such test here and have done so.
It might also be interesting to have a test where we simulate a side effect to make sure the order is preserved.
All of the tests do that, in that they are showing the exact order of evaluation when there are multiple parameters.
@javiercn Updated and ready for re-review |
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.
Looks superb!
Can't wait to try it out!
Thanks for the review. I know this required a bit of time investment to get through. |
6.0 preview 2? |
Hi @craigajohnson. It looks like you just commented on a closed PR. The team will most probably miss it. If you'd like to bring something important up to their attention, consider filing a new issue and add enough details to build context. |
@craigajohnson Yes |
@SteveSandersonMS this PR significantly expands @pranavkm's work to resolve #9431. It's also causing problems in Arcade promotions due to the many new long paths (see dotnet/arcade#12212). Can the path lengths please be shortened -- very soon❔ Long paths are a general barrier to entry for users of this repo. /fyi @dotnet/aspnet-build |
Thanks for letting me know. What path length budget are we working to? I know the Windows limit is 260 chars, but presumably we don't rely on people cloning at the root of their filesystem. If we're within striking distance of the path length budget, I might be able to just rename the new test cases to something slightly shorter. |
Not sure whether the Arcade validation error about a 230-character path
indicates a longer root than the code sync error about the same file
Either way, probably the shorter the better. Can you aim for shorter than one of the files @mkArtakMSFT mentions in #9431 (139 characters)❔
|
Note we're hoping to resolve #9411 by shortening that folder (and similar long ones in the analyzers tests) further. |
* Begin adding test for type parameter cascading * Cascade explicit ancestor type args * Switch to a push model for cascading generic types * Refactoring to simplify next change * Further refactoring to simplify next change * Make inferred generic types actually cascade. Still has a lot of missing cases, and is inefficient (causes repeated evaluations). * Minor refactoring (name change etc.) * Avoid multiple evaluations of the expression used for generic type inference by creating variables * Make cascaded type inference work with generic child content AFAICT the previous implementation was incorrect to treat child content as something that can "cover" a generic parameter, since it's always equivalent to an untyped lambda (i.e., doesn't specify its own types). It wouldn't have mattered before because you'd always have a different param on the same component providing the type, but now it does matter because we need to know whether to infer from an ancestor. * Handle lambdas on both provider and receiver sides * Handle inference of multiple generic parameters without duplicate evaluations * Update design time to match previous * Steps towards handling further cases * Refactoring that will simplify subsequent updates * Revert some changes I no longer want * Use the refactorings to simplify and ensure consistency about ordering * Emit _CaptureParameters variant of type inference method * Call the _CaptureParameters method to ensure single evaluation * Supply captured variables to descendants for cascading generic type inference * Update new baselines * Update comments * Step towards being able to handle unrelated generic types * Handle provision of unrelated generic types via diagnostic * Remove obsolete comment * Begin filtering which type params cascade * Now actually filter provided cascading generic types * Eliminate unnecessary parameter capturing for non-cascading-components * Update baselines * More test cases * Show we can have siblings * Show type cascading can pass through multiple levels * Clarify how we're retaining back-compat in a very obscure case * Better comments * Clean up APIs * CR: Null check * Rename attribute for consistency with other framework terminology * Add CascadingTypeParameterAttribute * Update tests to match new attribute name * CR: More tests and simplification * CR: Rename Commit migrated from dotnet/aspnetcore@116d21e0ec38
Implements #29349
This is a pretty big chunk of changes, but hopefully with some explanations of the high-level ideas it will make sense.
Outcome
This PR implements a refined version of "option 2" from #29349 (see Extend the type inference method to take extra synthetic parameters used for inference only). The intent with option 2 is that we can write arbitrary logic in the Razor compiler that (1) identifies unspecified generic type parameters, (2) matches them up with suitable candidates from ancestors (either explicitly specified or inferred), and (3) reuses the matching ancestor parameters during type inference.
The basic example given in #29349 is this grid/column scenario:
... used as follows:
The original idea (not the final outcome in this PR!) for how the type inference would work looks like this:
As you see, the Razor compiler sees that
<Column Title="Product name" />
doesn't specify anyTItem
, so it goes looking through the chain of ancestors to find one that explicitly cascades aTItem
type parameter. It finds this on<Grid>
, and sees that<Grid>
itself doesn't have an explicit value forTItem
but rather infers it from the parameter of typeIEnumerable<TItem>
. So the Razor compiler adds ontoEmitColumnComponent
a synthetic argumentsyntheticArg0
, with both the type (IEnumerable<TItem>
) and value (GetItems()
) coming from the ancestor. This is enough to resolve the generic type.The problem, and a solution
If you're paying close attention you'll probably spot the flaw here - we're now evaluating
GetItems()
three times, when the developer only intended to evaluate it once. My first attempt to fix this involved pulling out the synthetic arg into a local variable when generating the method calls:This seems like a cool plan, but causes multiple other problems:
<Grid>
parameters. There could be any number of them, some cascading and some not. It's not safe to capture just some of them in variables. At the very least we now have to capture allGrid
parameters in variables to preserve evaluation order.typeInferenceValue0
withvar
. That's convenient, but what if it's something not assignable tovar
, such as a lambda? Remember that we don't know the concrete type - that's the whole point of type inference. We can't emit something likeFunc<T> typeInferenceValue1 = () => 123
- the C# compiler won't allow it, and it's clearly meaningless in a case likeFunc<T0, T1> typeInferenceValue2 = x => x.Age
. Same problem if the value expression is a methodgroup.So we must capture everything as variables, but we can't capture everything as variables. Hmm.
We can solve this though. Instead of assuming we can immediately just assign the captured expressions to variables, we can re-use the same type inference technique we have been using all along. That is, let's take the existing type inference system and split it in two:
As well as emitting
EmitGridComponent
, also emit the following, which is a modified/simplified version of it:Now at the call site we can emit this:
This is good because now:
<Grid>
parameters exactly onceOf course, it is a bit more code to emit and actually does have some (extremely small, non-allocating) runtime cost. So we should only go through the
*_CaptureParameters
phase for components that have at least one cascadable generic type. But that's OK: we wanted to have a way of opting into cascading anyway, so the developer is going to tell us when to do it.Refactorings
The changes above involve quite a bit more complexity and repetition in the logic to emit this code. The existing code involved quite a bit of complexity and repetition too, as various different classes duplicated assumptions about which parameters we'd pass to the generated methods, what order they'd go in, etc. If any of these assumptions were out of sync, we'd generate code that doesn't compile. Of course, this started relatively simple back in the day, but as we added more features like
@key
and@ref
, it got increasingly complicated.The main refactoring I've done to make this more manageable is to consolidate all the decisions about what we pass to type inference methods, and in what order, into a single method
GetTypeInferenceMethodParameters
insideComponentNodeWriter
. There's a new classTypeInferenceMethodParameter
that describes all the metadata we ever want for each of these parameters. Then, all the other code that needs to do stuff with type inference just gets this list of parameters, and trusts it to be complete and correctly ordered, greatly reducing the number of decisions that happen in multiple places. This explains a lot of the code changes you'll find in this PR.There's still quite a bit of duplication between
ComponentDesignTimeNodeWriter
andComponentRuntimeNodeWriter
, perhaps a little more than I'd prefer, but it's mostly by design. These two writers are intended to at least be able to produce different outputs, even if in practice most of what they do is parallel to each other. I've done a couple of bits of factoring-out to share some logic, but am not inclined to do anything more dramatic to avoid the remaining duplication. I don't think it's hurting us and does serve a meaningful (albeit subtle) purpose, since obscure bits of the codegen do have to be different at design time to make code completions work best.