Closed SteveSandersonMS closed 3 years ago
Based on the out-of-scope "partial inference" comment, would it still be possible to support implicit inference from both immediate parameter and ancestor types? In other words as long as there are no explicit type parameters provided, all other inference types would be allowed?
Just thinking out loud... instead of inferring the generic type from ancestor components, would it be possible to infer it from Cascading Parameters? This would avoid the unique name dependency, and potentially could even allow inference across files. It would also provide a form of a more strongly-typed generic inference.
I know we've already established that this feature is desirable, so I'm moving on to some candidate implementation suggestions.
Before we start, first understand how the existing generic type inference works. As of 5.0, Razor's generic type inference works by converting "the code that emits component frames" into "a method call that emits those component frames". As a simplified example, without generic inference we might have this, where Items
is a parameter of type IEnumerable<TItem>
:
// <Column TItem=Person ColumnName="@someColumnName" Items=@people />
void BuildRenderTree(RenderTreeBuilder builder)
{
builder.OpenComponent<Column<Person>>(0);
builder.AddAttribute(1, "ColumnName", someColumnName);
builder.AddAttribute(2, "Items", people);
builder.CloseComponent();
}
Whereas with generic type inference we have this:
// <Column ColumnName="@someColumnName" Items=@people />
void BuildRenderTree(RenderTreeBuilder builder)
{
EmitColumnComponent(builder, arg0: someColumnName, arg1: people);
}
// This is generated by the Razor compiler
static void EmitColumnComponent<TItem>(RenderTreeBuilder builder, string arg0, IEnumerable<TItem> arg1)
{
builder.OpenComponent<Column<TItem>>(0);
builder.AddAttribute(1, "ColumnName", arg0);
builder.AddAttribute(2, "Items", arg1);
builder.CloseComponent();
}
The C# compiler does the fancy work of inferring the TItem
type at the call site inside BuildRenderTree
based on the arg1
expression type.
Notice how EmitColumnComponent
has to have parameters for all the parameters of Column<TItem>
, even those not involved in generic type inference. We can't just inline the expressions for their values into the code inside EmitColumnComponent
, because those expressions might reference other variables that only exist in scope at the original call site inside BuildRenderTree
. For example, someColumnName
might be a local variable that only exists at this point during rendering (e.g., a loop variable), so we can't reference it directly from EmitColumnComponent
.
It's actually slightly more complicated still, but that's the core idea (by @rynowak originally, mentioning him in case he wants to come and share any further wisdom!) and is all we need to know to understand the following.
Spoiler: this is not my preferred option - see later
What if, instead of EmitColumnComponent
taking parameters for all the parameters of Column<TItem>
, it only took ones for those involved in generic type inference? The other parameter values could be inlined into the generated code. For example, if you're passing a ChildContent
fragment (which is compiled as a lambda), then the source for that lambda would go inside EmitColumnComponent
rather than at the call site.
Then to make this safe so you can still reference variables that are only in scope at the call site, we could change the whole thing to be a local function:
// <Column ColumnName="@someColumnName" Items=@people />
void BuildRenderTree(RenderTreeBuilder builder)
{
EmitColumnComponent(builder, arg1: people);
// This is generated by the Razor compiler
static void EmitColumnComponent<TItem>(RenderTreeBuilder builder, IEnumerable<TItem> arg1)
{
builder.OpenComponent<Column<TItem>>(0);
builder.AddAttribute(1, "ColumnName", someColumnName);
builder.AddAttribute(2, "Items", arg1);
builder.CloseComponent();
}
}
Now, if we have multiple nested levels of type inference, they will get compiled as multiple nested levels of local functions, hence they are within each other's scopes, e.g.:
void BuildRenderTree(...)
{
...
EmitGridComponent<TItem>(...)
{
...
EmitColumnComponent(...)
{
...
}
}
}
So at this point, all the Razor compiler has to do is not put any generic type declarations on the nested Emit...
methods if their names exactly match one of the generic type declarations on an ancestor, and by lexical scope, the ancestor's value will get used.
As an alternative, instead of changing where the type inference methods go, we could just add some more parameters to them. As of 5.0, they take one parameter for each [Parameter]
on the component you're rendering. But what if we extend this to include not only the [Parameter]
s from the component you're rendering, but also include selected [Parameter]
s from ancestor components in the same .razor
markup?
For example, given this markup (where both Grid
and Column
declare @typeparam TItem
):
<Grid Items="@people">
<Column IsCoolColumn="true" />
</Grid>
... we generated the following:
void BuildRenderTree(RenderTreeBuilder builder)
{
EmitGridComponent(builder, arg0: people, arg1: (RenderFragment)(builder =>
{
EmitColumnComponent(builder, syntheticArg0: people, arg0: true);
});
}
static void EmitGridComponent<TItem>(RenderTreeBuilder builder, IEnumerable<TItem> arg0, RenderFragment arg1)
{
builder.OpenComponent<Grid<TItem>>(0);
builder.AddAttribute(1, "Items", arg0);
builder.AddAttribute(2, "ChildContent", arg1);
builder.CloseComponent();
}
static void EmitColumnComponent<TItem>(RenderTreeBuilder builder, IEnumerable<TItem> syntheticArg0, bool arg0)
{
builder.OpenComponent<Column<TItem>>(0);
builder.AddAttribute(1, "IsCoolColumn", arg0);
builder.CloseComponent();
}
That is, when rendering Column
, we notice that its TItem
isn't "covered" by either an explicit declaration (TItem=...
), nor by a [Parameter]
on the component itself. So we fall back on scanning up the ancestor hierarchy to look for the closest thing that has a generic parameter called TItem
and was supplied with any [Parameter]
value that could "cover" it (or an explicit declaration).
We discover Grid
is the closest candidate, and that its type inference made use of a parameter called Items
of type IEnumerable<TItem>
. So for EmitColumnComponent
, we add a synthetic extra argument whose type is IEnumerable<TItem>
, and we pass the people
value which is known to be in scope because descendant rendering is always within a lambda within the call site for the ancestor.
Even though EmitColumnComponent
's logic doesn't use syntheticArg0
in any way, simply passing it makes generic type inference work.
<CascadingValue>
We've had a couple of community suggestions (here, here) that <CascadingValue>
should be extended to supply generic types as well as actual parameter values.
I don't personally think this works in any way, because generic type inference has to be resolved at compile time in order for generic types to do all the things people want (e.g., produce correct intellisense when operating on instances of those generic types, and give compile-time errors if you reference non-existent members on them).
CascadingValue is a runtime feature, since the actual set of component ancestors can't be known in general until runtime (you could decide it dynamically based on user actions, for example). So it's not meaningful to have it impact compiler output.
More generally, I don't think the problem of inheriting types across .razor
files can have any solution. Consider sources like this:
@* Index.razor *@
<Grid Items=@people>
<MyColumns />
</Grid>
@*MyColumns.razor*@
<Column Name="First name">@context.FirstName</Column>
<Column Name="E-mail address">@context.Email</Column>
That can't possibly work at compile time because MyColumns.razor
has no way to know it's only ever going to be used from within a specific place inside Index.razor
. Even if at runtime that happens to be true, the compiler can't know it. So it can't possibly specify any particular TItems
value for the <Column>
component.
What you'd actually need to do is add a type parameter onto MyColumns.razor
, e.g.,
@*MyColumns.razor*@
@typeparam TItem
<Column TItem=TItem Name="First name">@context.FirstName</Column>
<Column TItem=TItem Name="E-mail address">@context.Email</Column>
Now this can work with design 2 above, because Index.razor
infers the type to pass to <MyColumns>
based on the match local to that file, and MyColumns.razor
explicitly passes that on to <Column>
(or maybe we even make the inference match @typeparam
on the component itself and pass default(TItem)
to the inference function, so you can omit the TItem=TItem
parts).
So in summary, I don't think that <CascadingValue>
is relevant for solving this problem, and isn't needed because design 2 already produces the best possible result anyway.
The designs above assumed that, when we're looking for a value for TItem
on a child component, we'd match that by name against any same-file ancestor that also has a type parameter called TItem
. That is, matching on the name as a string. Clearly this has the drawback of not being fully strongly typed.
It would be nice if there was some way to define the exact set of types you were willing to match against. For example:
@typeparam TItem from Grid<>
or, without needing new syntax:
@typeparam TItem
@attribute [InferTypeParameter(nameof(TItem), FromComponent: typeof(Grid<>))]
Notice that we can't refer to Grid<TItem>
in the attribute, because attributes can't reference type parameters from their own host class (because the attribute is on the open generic type, not any particular closed one), so we're limited to saying Grid<>
, so it doesn't lead to any obvious way to say "I can only be inside a Grid<TItem>
with the same type arg". Also notice that if Grid
had multiple type parameters, we still have no way to say which of them we mean other than stating a name string, so there's still a level of string-based name matching.
However there's a bigger problem still with this, which is that the Razor compiler doesn't know about type compatibility. If somebody subclasses Grid<TItem>
to make SuperGrid<TItem>
, the Razor compiler doesn't naturally know about this relationship. Likewise with inferface implementations. Unless we actually walk the chain of all base types and interface implementations and recreate all the "is assignable from" rules inside the Razor compiler. In short, we're not going to get anywhere near doing that.
I tried to think of some way to express the type inference method such that it would end up picking a type arg based on type assignment compatibility, but it's absolutely mind-bending and I think at the very least would involve passing a separate parameter for every ancestor (and to do that, involves an extra layer of lambda-capturing for each level of ancestry). It seems like a truly bad direction to go, but by all means correct me if you see a clean way to do it!
Since we can't truly have strongly-typed type parameter matching, are we at risk of accidental matches and general confusion when unrelated components both have @typeparam T
?
A solution that @javiercn and I came up with is simply to make the inference matching opt-in. That is, type parameters don't flow by default. The developer explicitly enables the flow on particular type parameters, and when they do so, we interpret that as meaning "I promise the name is unique enough for my purposes".
We could either do it on the receiving side:
@* Column.razor *@
@typeparam TItem
@attribute [ReceiveTypeParamFromAncestors(nameof(TItem))]
... or on the sending side:
@* Grid.razor *@
@typeparam TItem
@attribute [CascadeTypeParam(nameof(TItem))]
Perhaps this is where @ivanchev and @ebekker's intuition about an explicit "cascade" gesture does line up with what we can do.
I prefer option 2 above because:
All this said, it's still a pretty complicated thing to implement, so if there are suggestions of better approaches from @NTaylorMullen, @rynowak, etc., I'd be very interested!
Just to harp on the cascading param idea a bit more... perhaps I'm missing a subtle point, but while I agree that populating a cascading parameter happens at runtime, isn't the type specification fully declared and known at compile time? You have to define a property on a component class and annotate it to declare a cascading parameter, so it's type should be fully resolvable at compile time. For example:
public class MyComponent<TItem> : ComponentBase
{
[CascadingParameter]
public SomeType<TItem> SomeValue { get; set; }
}
It wouldn't matter if SomeValue
was ever populated or not, the type is still known at compile time.
Though, I'm sure I'm missing some subtle point here...
@ebekker Sorry but I don't see how that fits in with how generic type inference works. You would need to provide a code sample showing exactly what the compiler would generate, for example in the case I was using above (<Grid Items=@people><Column /></Grid>
). Additionally we don't want to give developers an extra job of converting their parameters into cascading parameters when regular parameters would suffice, as in the designs 1 and 2 above.
If you pass the synthetic arguments before the rest of the values to the emit-function I guess that something like <Column ValueSelector = @(x => x.Age) />
can resolve to the type Column<Person, int>
, where Person
is resolved from the ancestor parameter and int
is resolved by the lambda declaration. To get intellisens on lambda. Is that correct and is that the plan? If yes, then I really like it.
@Liander As far as I can tell, yes it would be able to infer the type in this case. However I don't see any reason why it matters what order we put the arguments on the emit function. I think it would work exactly the same if the synthetic arguments go last.
It matters for the intellisens, at least if it works like ordinary factory methods.
It doesn't seem to make a difference when I tried it. If you could post a Gist showing a minimal example of a case where this matters I'd be interested.
I started to look at the generated code of a sample supporting the cascading parameters and I see the errors of my way -- the surrounding page doesn't have any way to infer the type being passed from parent to child via a cascading parameter between the two -- I see your point now, thanks for humoring me!
I was originally only thinking of the developer/user ergonomics but I can see the implementation is not feasible.
@SteveSandersonMS I am confused. A gist will not show intellisens. I referring to the general c# behavior when having:
public void EmitColumn<T, TValue>(Func<T, TValue> ValueSelector, IEnumerable<T> people) {}
and start typing a call to EmitColumn(x => x.
You won't get intellisens after the dot since T is yet to be resolved. But it would be able to infer if you change the order, agree?
Will it work differently here?
@Liander Ah yes, I see what you mean! Thanks for clarifying. I thought you were talking about the order of the parameters declared on the emit function, but now I see you mean the order of the arguments on the invocation of the emit function (which can be a different order since we can use named parameters at the call site).
I'll update the example code above to show the synthetic args going first in both cases to make this clearer and simpler.
@SteveSandersonMS The goal of this is ultimately to allow for strongly typed RenderFragments.
public class Grid<T> : ComponentBase
{
[ViewParameter]
public IEnumerable<Column<T>> Columns { get; set; }
}
Nobody wants to allow any old content in their
This would solve the inferred type requirement explicitly rather than via complex same-name rules.
@mrpmorris I appreciate the appeal of that, but it sounds like a completely different kind of feature proposal. It sounds like the “restrict view hierarchy” suggestion from before, and I’m not sure how it would lead to any type inference based on how the C# compiler works.
Rather than risking confusing people by discussing it in this issue, would you be able to file a separate issue describing your suggestion in more detail? Please include:
If there’s a good and practical way to do this we’d definitely be interested, but let’s do separate feature designs in separate issues. Thanks!
@SteveSandersonMS I very strongly suspect (99.9%) that this is what the original report by @ivanchev is trying to achieve.
If I open a new ticket it is basically going to be the same as #7268 which you have closed in favour of this one.
I really don't think people want magic(ish) propagation of generic type parameters, what they actually want is strongly typed child components. The grid columns in a grid in the original ticket is a perfect example. The original poster doesn't want the consumer to be able to do something like this
<Grid TData="Person">
<Columns>
<h1>Hello</h1>
<p>This isn't a column</p>
<Column Field=@(x => x.GivenName)/>
</Columns>
</Grid>
and then have to only render
Please correct me if I am wrong @ivanchev
Please file a separate issue with the details requested.
Done, thanks!
@mrpmorris - these are two separate issues indeed. What you are referring to is to restrict the children of a certain RenderFragment Parameter(Columns), or component. While this is also useful, it's not the feature I requested.
What I requested is exactly what is being proposed here by @SteveSandersonMS - a way to infer the type of child components. This will allow strongly typed child fragments, without specifying a type on each column.
Thanks @ivanchev for clarifying. Since this proposal is not about restricting the type hierarchy, it allows for certain scenarios that are more general than would be allowed if we did restrict the type hierarchy. For example, having a layer of <CascadingValue>
in between the <Grid>
and a specific collection of <Column>
. I've added this example into the proposal above. This helps to make clear how it's a different objective than #29379 (which is also a valid and useful proposal, but addresses a different need).
Also, yesterday @javiercn and I discussed these proposals at length and came up with a very simple improvement to the name-based matching to avoid most cases of accidental matches. See the new section about Matching rules above.
Maybe it is not an intentional detail, but I do not think you can have the Emit-functions in proposal 2 as static because you should be able to access component instance members, correct?
(That said, I actually do want to have the option of generating them static, to control what instance state to access and have change-detection of that explicit dependency if any exist, but that is another story...)
Oh.. looks like it is only the BuildRenderTree that nests the Emit-functions... I must have recalled wrong... Ignore my note.
They can be static, and in fact can be in an entirely different class, because they don't directly access anything from scope or other component members. They receive all the parameters as arguments (for example, child content is a RenderFragment
delegate). This is true for the existing type inference and AFAIK will continue to work the same with proposal 2.
@SteveSandersonMS this actually opens up a whole set of possibilities and I wonder if it syncs up a bit with the concept of a child component knowing as much as possible about its parent. We're having to do some tricks now to build out the component hierarchy. I wonder if having the TParent type param along with a corresponding TParent Parent parameter available for all components would solve this? We're effectively doing this now but only by subclassing ComponentBase and monkeying more than we'd like.
I have been summoned...
Option 2 seems pretty well thought through.
This isn't that similar to how I'd imagined we'd solve this when I was on the team but seeing it all laid out with an example makes it really clear. I'm pleased that this solution avoids evaluation-order issues, which was one of my primary concerns implementing both this and the original type inference feature.
For those that are driving by and don't get what I mean, it's pretty important that the generated code evaluates the component parameters in the order that you pass them from left to right (since that's what C# does).
<Grid Items="@GetPeople()" AnotherThing="@DoSomething()">
<Column IsCoolColumn="true" />
</Grid>
If mutate state in GetPeople()
and DoSomething()
you really don't want these calls to be reordered when you do something subtle like add type inference for a body.
Synthesizing a 'thunk' and calling it avoids that problem since you're now working with a variable that already holds a value rather than copy-pasting code. The temptation for folks that are new to Razor and compilers in general is to assume that we can just copy-paste your code into a new context, and examples like the above demonstrate why you can't.
The other path that I had in mind for this issue was related to discovery and nested classes. I'm a nested-class-a-holic, if you look at any codebase I've worked on seriously you will seem them everywhere! I wanted the world to share my joy.
The insight is that a container like Grid
and the Column
that goes inside it are tightly coupled. We've encountered this scenario as well for tag helpers - where you want to build a Grid
but you want to only allow it contain Column
s. The nested class solution to this is nice for the reason that it's something static - we can detect at discovery time the relationship between these, and Column
can't appear outside of a Grid
(decide on your own if that's good or not 😆).
Nested classes can solve both of these problems at once - but only work really well if you always want to solve them both together. For instance if Column
is a nested class of Grid
then it's proper name is actually Grid<T>.Column
and Column itself has no type parameter of its own. I hadn't thought through this to the codegen level so a solution like Option 2 above might also be needed to make the nested class thing work.
The one part of this that I don't love is @attribute [ReceiveTypeParamFromAncestors(nameof(TItem))]
. It feels a little arbitrary and clumsy. I'd want control to say which ancestors.
Anyways much ❤️ to the community and team. I'm off on a new adventure now, and I'm really have to see the community so engaged and the team cranking out awesome work.
Done in #29767
Summary
Currently, component generic type inference is based exclusively on parameters passed to that component, ignoring any other context such as ancestor components. This imposes problematic limitations in more sophisticated scenarios, particularly for component vendors trying to create a really smooth consumption experience.
See customer report in #7268, but note this has also been requested independently by component vendors.
Motivation and goals
The classic example is a generic
<Grid>
component containing generic<Column>
children. As of today you have to do something like this:...when what you actually want to do is this:
Notice how the consumer has to re-specify
TItem
on each column, because it can't be inferred from the enclosing<Grid>
(even though<Grid>
itself can infer based on its ownItems
parameter).In scope
.razor
file<Grid TItem=Person>
)<Grid Items=@someEnumerableOfPerson>
)<Column TItem=Person>
) - that overrules all inference[Parameter]
- that overrules inference based on ancestorsOpen questions:
@typeparam TItem from Grid
. I'm not keen on inventing new syntax like that..razor
files, and in that case it's not really flexible enough to handle the requirements for generic type inference. For generic type inference, we want to support arbitrary depth ancestors within the same.razor
file.Out of scope
.razor
file. We can't know which other.razor
files are going to contain a reference to your component, hence we don't know what the ancestors will be. Even if we did, there might be multiple ones that would "supply" different generic types. Altogether it's meaningless to imagine we're matching against "ancestors" in entirely separate files.Risks / unknowns
It's taking a fairly magic thing and making it more magic still. It will become fairly hard to explain the exact rules around type inference.
It makes the declared generic type parameter name more important. It's no longer just named for explanatory purposes, but also for uniqueness purposes. Some existing components might have generic type parameter names that aren't unique enough (e.g., just
T
) and so the inference-based-on-ancestors mechanism might match a different ancestor than you intended. This isn't massively bad because you'd know about it at compile time, plus in most cases, developers can change their generic type names to make them more unique when required. We should check that our own generic components have good type generic type names.If we implement an "Extract Component"-type refactoring in the future, it will have to account for this. That is, it will have to work out which generic types were being inferred from an ancestor, and convert those into
@typeparam
declarations on the extracted component.Examples
See the
<Grid>
example above as the primary scenario.Other scenarios are anything where you have multiple child components that all take a
RenderFragment<T> ChildContent
parameter, each of which does something with the same data source. Example: a<Chart>
component containing multiple<Series>
.Even if literally the only scenario was "grids", I think that would still be important enough to warrant this work.
Also bear in mind cases where the inference needs to flow through multiple levels in the component hierarchy. You don't always infer based on the immediate parent. For example,