Match component properties as case insensitive

[jjonescz]

Resolves #8854.

Commit-by-commit review might be useful.

[DustinCampbell]

The change itself looks good. However, just to be clear that the expected result is that only bound attributes will be case-insensitive. The name of the component should still be case sensitive?

[jjonescz]

However, just to be clear that the expected result is that only bound attributes will be case-insensitive. The name of the component should still be case sensitive?

Yes, exactly. The component parameters are case-insensitive at runtime, so now the compiler will match that. The component name is a compile-time thing (there is no string matching of the name at runtime) and we definitely want <Input> to be a component but <input> should be an HTML element.

[DustinCampbell]

However, just to be clear that the expected result is that only bound attributes will be case-insensitive. The name of the component should still be case sensitive?

Yes, exactly. The component parameters are case-insensitive at runtime, so now the compiler will match that. The component name is a compile-time thing (there is no string matching of the name at runtime) and we definitely want <Input> to be a component but <input> should be an HTML element.

I just checked the code and it looks like case-sensitivity for BoundAttributeParameterDescriptors follow BoundAttributeDescriptors. So, marking attribute names as case-insensitive will implicitly mark the attribute parameter names as well. Is that relevant here? Would it make sense to add additional tests for that scenario?

[chsienki]

Is this correct? The test is specifying PlaceHolder, but it looks like we're now choosing Placeholder which seems incorrect?

[jjonescz]

So, the code that this is testing would fail at runtime anyway with

System.InvalidOperationException: The type 'Component' declares more than one parameter matching the name 'placeholder'. Parameter names are case-insensitive and must be unique.

We are choosing Placeholder simply because the matching is case-insensitive, so we choose the first one.

I could alternatively implement what you suggested in the issue - matching the casing if there are multiple properties. In fact I already implemented that (see 0a6e708) but then removed it because it just seems redundant if runtime would fail anyway; but I can return it if you think that's better.

[jjonescz]

I just checked the code and it looks like case-sensitivity for BoundAttributeParameterDescriptors follow BoundAttributeDescriptors. So, marking attribute names as case-insensitive will implicitly mark the attribute parameter names as well. Is that relevant here? Would it make sense to add additional tests for that scenario?

From my understanding BoundAttributeParameterDescriptors are used for things like @bind-Value:get - and the case sensitivity is for the get part - again there is no runtime equivalent there, that's completely handled by the compiler, so I think the case sensitive matching should remain, but I can add tests.

But that made me think about @bind-Value - looks like that's handled in a different code path which should also be changed to case insensitive matching for consistency (for the Value part not @bind, I will add tests for both). Thanks.

Note: I'm again updating the baselines in a separate commit so the effect of the product change can be reviewed.

[jjonescz]

Hm, EditorRequired parameter matching could be also case-insensitive - but that seems like a separate issue - maybe we want that to be case-sensitive so users can fix the casing to match. I will add tests without changing the behavior in this PR.

[DustinCampbell]

From my understanding BoundAttributeParameterDescriptors are used for things like @bind-Value:get - and the case sensitivity is for the get part - again there is no runtime equivalent there, that's completely handled by the compiler, so I think the case sensitive matching should remain, but I can add tests.

I'm a little confused. The BoundAttributeParameterDescriptor.CaseSensitive will match whatever value BoundAttributeDescriptor.CaseSensitve is set to. The case-sensitivity of bound attributes and parameters is used in TagHelperMatchingConventions. Is it expected that TagHelperMatchingConventions.SatisfiesBoundAttributeWithParameter(...) will match @bind-Value:GET, or does that not ever happen because of other code paths?

[DustinCampbell]

What's the point of setting BoundAttributeDescriptor.CaseSensitive if we just always do a case-insensitive comparison during lowering? Should we be checking CaseSensitive here? If not, it'd be good to add a comment why.

FWIW, there's a private helper in TagHelperMatchingConventions that retrieves the correct StringComparison based on BoundAttributeDescriptor.CaseSensitive. If that we're made public, this could be something like this.

foreach (var attribute in componentTagHelper.BoundAttributes)
{
    var comparison = attribute.GetComparison();

    if (string.Equals(valueAttributeName, attribute.Name, comparison))
    {
        valueAttribute = 
    }
    if (string.Equals(changeAttributeName, attribute.Name, comparison))
    {
        changeAttribute = attribute;
    }

    if (string.Equals(expressionAttributeName, attribute.Name, comparison))
    {
        expressionAttribute = attribute;
    }
}

Also, this algorithm walks forwards and compares every attribute name three times. That could be improved by walking backwards and using three bools to track when valueAttribute, changeAttribute, and expressionAttribute are set. When a bool is set, we can avoid that particular string comparison. When all three are set, we can break out of the loop.

[jjonescz]

FWIW, there's a private helper in TagHelperMatchingConventions that retrieves the correct StringComparison based on BoundAttributeDescriptor.CaseSensitive. If that we're made public, this could be something like this.

Yeah, sounds good, I will do that, thanks.

Also, this algorithm walks forwards and compares every attribute name three times. That could be improved by walking backwards and using three bools to track when valueAttribute, changeAttribute, and expressionAttribute are set. When a bool is set, we can avoid that particular string comparison. When all three are set, we can break out of the loop.

I can do that as well, thanks. Although I think bools aren't necessary - we can check whether the variables are null.

[DustinCampbell]

Although I think bools aren't necessary - we can check whether the variables are null.

Oh yes, you're right. I had misread the code above and my eyes were seeing changedAttributeName and changedAttribute as the same.

[jjonescz]

The BoundAttributeParameterDescriptor.CaseSensitive will match whatever value BoundAttributeDescriptor.CaseSensitve is set to.

Yes but the BoundAttributeDescriptor which is the parent of the BoundAttributeParameterDescriptor is the descriptor for @bind-... (created by BindTagHelperDescriptorProvider) which is case sensitive (and we want it to stay that way) - it's separate from the BoundAttributeDescriptor created for the component parameter (created by ComponentTagHelperDescriptorProvider) which is now case insensitive.

Is it expected that TagHelperMatchingConventions.SatisfiesBoundAttributeWithParameter(...) will match @bind-Value:GET

So no, that should not match and there's a test for that: ImplicitStringConversion_ParameterCasing_Bind_03.

[DustinCampbell]

The BoundAttributeParameterDescriptor.CaseSensitive will match whatever value BoundAttributeDescriptor.CaseSensitve is set to.

Yes but the BoundAttributeDescriptor which is the parent of the BoundAttributeParameterDescriptor is the descriptor for @bind-... (created by BindTagHelperDescriptorProvider) which is case sensitive (and we want it to stay that way) - it's separate from the BoundAttributeDescriptor created for the component parameter (created by ComponentTagHelperDescriptorProvider) which is now case insensitive.

Is it expected that TagHelperMatchingConventions.SatisfiesBoundAttributeWithParameter(...) will match @bind-Value:GET

So no, that should not match and there's a test for that: ImplicitStringConversion_ParameterCasing_Bind_03.

Thanks for verifying that there're tests to validate the behavior. I'm still confused by how this is supposed to work from the implementation, but if there're tests validating the correct behavior, I'm good with it.

[jjonescz]

@chsienki for another look, thanks