Add initial support for parameters with AsParameters attribute

[captainsafia]

Part of #46336.

• Update EndpointParameter type to support construction from IPropertySymbol and IParameterSymbol
• Update parameter emission to support recursing into populated EndpointParameters
• Move setting parameter-specific emitter context properties to EmitParameterPreparation
• Update logic within EndpointParameter class to take INamedTypeSymbol or ImmutableArray<AttributeData> directly instead of deriving them from an IParameterSymbol

Note: this PR does't encompass all the details of AsParameters or migrate all tests. Notably missing is:

The following features are now supported:

• Warning if a parameter marked with AsParameters is marked as nullable
• Warning if the constructors for the type associated with a parameter marked with AsParameters are not valid
• Warning if a nested AsParameter is encountered

[mitchdenny]

This is looking good. The questions I had about invalid constructors you've already called out. Do you think this is something that you want to merge and then iterate, or do you want to get to full fidelity first?

[captainsafia]

This is looking good. The questions I had about invalid constructors you've already called out. Do you think this is something that you want to merge and then iterate, or do you want to get to full fidelity first?

I was intending to merge and iterator. At the moment, we flag the different types of unsupported constructors in the TryGetAsParametersConstructor call when we return false:

https://github.com/dotnet/aspnetcore/blob/2b3b0a4d89b6394655b1ead1dca0a44825c59019/src/Http/Http.Extensions/gen/StaticRouteHandlerModel/EndpointParameter.cs#L447

In the handling, this marks the parameter with an Unknown parameter source which opts it into our generic "this parameter is not supported warning experience". We'll want to flow the state up to the diagnostic handling in the Endpoint model itself. The reason I want to pause in it is we'll want to bubble up diagnostics for the other scenarios mentioned in my bulleted list as well so I'd figure I'd handle all of the diagnostics-related scenarios in one PR.

[amcasey]

I don't really understand the scenario, so I'm just nitpicking. 😉

[amcasey]

Is this only called from the ctor above?

[captainsafia]

Correct, this contains the "core" implementation of the EndpointParameter that parameters sourced directly from the handler like so:

app.MapGet("/{name", (string name) => ...);

And those that are resolved as properties from an AsParameters attributed type resolve from:

app.MapGet("/{name", ([AsParameters] MyArgs args) => ...);

public class MyArgs
{
  string Name { get; set; }
}

[amcasey]

From the outside, the logical boundary isn't all that clear and it looks like a candidate for inlining. If it's clear, in context, why it should be a separate method, then no objection.

[amcasey]

Return early if there aren't any?

[amcasey]

I think indexers can have the same name. Maybe you meant to filter those out above?

[captainsafia]

Good question that led me down an interesting rabbit hole!

I'm assuming that you're referring to indexers on types like in the case below:

public class ClassWithIndexer
{
    private int[] arr = new int[100];
    private Dictionary<string, int> map = new Dictionary<string, int>();
    public int this[int i]
    {
        get { return arr[i]; }
        set { arr[i] = value; }
    }

    public int this[string i]
    {
        get { return map[i]; }
        set { map[i] = value; }
    }
}

Looking to RDF as guidance, we observe two problems here:

It looks like the AsParameters implementation in RDF isn't supporting indexers at all. Amongst other things, it produces incorrect code including initializations like the following:

$args_local = .New Microsoft.AspNetCore.Http.Generators.Tests.ClassWithIndexer(){
            Item = $Item_local
        };

which disregard that the indexer cannot be initialized with an object initializer. It seems like support for indexers on AsParameters-enabled types wasn't considered as part of the original proposal so we'll have to think through how the implementation will work in both RDF and RDG. I've filed #48118 to track this.

Second is with regards to the scenario where multiple indexers are defined (both which emit the underlying Item property name). RDF has support for this in its argument checking. It'll through an exception at startup warning that duplicate argument names where found:

ArgumentException: An item with the same key has already been added. Key: Item

This is smelly code though because we're just throwing the unhandled exception from attempting to add to FactoryContext.TrackedParameters without informing the user why.

I'll noodle on how to handle this more elegantly in RDG but may approach this in a follow-up when we migrate the RequestDelegateThrowsWhenParameterNameConflicts test.

[amcasey]

Maybe we should just skip properties with parameters for now?

[captainsafia]

👈🏽 for reviews here.

[eerhardt]

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?

[captainsafia]

Ha! @mitchdenny and I had a similar conversation about this yesterday.

Do we Assert in other generated code already?

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.

Can the user actually get into this situation?

Nope, not if they are using RDG.

The nullable parameters in the API signatures of MetadataPopulator and RequestDelegateFactoryFunc exists primarily because the RequestDelegateFactory 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.

[amcasey]

Would it make sense to just disable nullability checking in these files?

[captainsafia]

Would it make sense to just disable nullability checking in these files?

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 in System.ComponentModel.DataAnnotations). For example, the code below will bind the age value from the route parameter but check that the route parameter has been provided before invoking the users handler.

app.MapGet("/birthday/{age}", (int age) => ...)

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.

[eerhardt]

I would just ! away the nullability checks on this one line:

-var serviceProvider = options?.ServiceProvider ?? options?.EndpointBuilder?.ApplicationServices;
+var serviceProvider = options!.ServiceProvider ?? options!.EndpointBuilder!.ApplicationServices;

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.

[captainsafia]

Yep. My preference towards using Debug.Assert vs. the null forgiving operator is that Debug.Assert is that we fail upfront in our debug builds if we're in an invalid state instead of waiting until the options 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).

[amcasey]

More sniping from the peanut gallery.

[amcasey]

Remind me what ToDisplayString does if the type is something like class @class { }?

[captainsafia]

It looks like the @ is stripped from the type symbol's identifier.

[amcasey]

That sounds bad. Won't you end up with typeof(class), which is a syntax error?

[captainsafia]

Ah! I misinterpreted your comment a little bit. I thought your inquiry was around the use of the @ prefix on class names in general (which ends up producing something like typeof(MyClass) which is valid) but you were referring to the literal use of class @class.

My initial reaction to this is that it's the kind of edge case that I'd be comfortable not addressing specifically in RDG.

My second reaction is to explore what the implementation complexity would look like with this and see if it is worth pursuing given cost/reward. It might very well be we have to pass a formatter to ToDisplayString here to get the desired behavior.

My third reaction is wondering if the current behavior (compiler errors on generated code) is a valid experience for users who run into these kinds of buggy experiences.

In any case, this is worth tracking in a follow-up issue if you'd like to file one?

[amcasey]

I would expect this to come up in a large variety of source generators, so I would guess there's a standard solution (maybe a DisplayString variant or an escaping API).

[amcasey]

How does it fail if someone does do that? I guess their build fails and the mitigation is currently to rename their type?

I'll file it.

[halter73]

I'm not going to wade into the Debug.Assert vs ! discussion. Both sides make good points, but the asserts are consistent with what we've done so far.