Extensions: fix lowering of implicit conversion on receiver in pattern-based scenarios

[jcouv]

This is a follow-up on #78480, which didn't properly handle the lowering of some more complex implicit conversions (implicit span or implicit tuple conversions).

Relates to test plan #76130

[jcouv]

📝 I was debugging in this area as part of this PR and noticed we're doing some captures

[jcouv]

📝 the problem was that this produces a BoundConversion. That's okay (can be handled by the emit layer) for simple conversions, but for more complex conversions we need to produce lowered nodes (see MakeConversionNode).

[jcouv]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 2 pipeline(s).

[AlekseyTs]

markAsChecked: true

I do not see any corresponding changes in binding, therefore, I am not certain whether we really can pass true here. The contract is, we can pass true only if this conversion was actually checked during binding. This is achieved by calling CreateConversion helper and actually attempting to create BoundConversion node (there are plenty examples for this). In addition, it is preferred to not drop the created BoundConversion node, but preserve it in the tree and use it during rewrite in order to actually produce its lowered form. There is a small set of cases when we do not preserve the node given too much complexity of the bound node structure, but we still "check" conversion during binding for those cases. #Closed

[jcouv]

Whenever initial binding produces the nodes for an extension property access, we go through ResolveExtensionMemberAccessIfResultIsNonMethod/GetExtensionMemberAccess which does CheckAndConvertExtensionReceiver on the receiver.
So I believe markAsChecked: true is correct.
For the broader issue of not recreating conversions during lowering, let's discuss offline.

[AlekseyTs]

I would like to suggest the following:

1. Instead of assuming that each caller performed binding in a certain way, we should request each consumer of ConvertReceiverForExtensionMemberIfNeeded to assert that by passing true to markAsChecked parameter (which I suggest to add).
2. For each call site of ConvertReceiverForExtensionMemberIfNeeded let's try to add a test that makes the fact that ```CheckAndConvertExtensionReceiver `` was called observable. Perhaps by observing a diagnostic that otherwise wouldn't be reported.

[AlekseyTs]

@checked: false

This looks suspicious. #Closed

[jcouv]

Only implicit conversions are allowed on the receiver of the extension member. Implicit conversions don't care about checked/unchecked (only certain operators and explicit conversions care)

[AlekseyTs]

Consider capturing this line of thinking in a comment.

[AlekseyTs]

this.ConvertReceiverForExtensionMemberIfNeeded(memberSymbol, rewrittenReceiver);

Is there a test that would fail for this code path without the change? #Closed

[jcouv]

I was not able to find one.
We need:

1. a conversion that is acceptable for the extension parameter: those are identity, boxing, implicit reference, implicit span, or implicit tuple (see IsValidExtensionMethodThisArgConversion)
2. the conversion needs to be non-trivial (ie. needs lowering): we're left with implicit span or implicit tuple
3. the receiver type of the property should not be a struct (see logic in CheckPropertyValueKind involving RequiresVariableReceiver): we're left with no options

The ExtensionMemberLookup_ObjectInitializer_Conversion_02 test illustrates the ROS scenario. I'll add a couple similar tests

[AlekseyTs]

Done with review pass (commit 2) #Closed

[jcouv]

@dotnet/roslyn-compiler for second review. Thanks

[AlekseyTs]

@jcouv It looks like ExtensionMemberLookup_PatternBased_Deconstruct_Conversion_03 test has DEBUG vs. RELEASE behavior difference

[AlekseyTs]

// (1,41): error CS0131: The left-hand side of an assignment must be a variable, property or indexer

I think we should capture a design question about an error like this with respect to extension members. If we are extending a struct by value, it feels like the requirement for the receiver to be a variable is meaningless. The property wouldn't have access to the original location even if the receiver was a variable. Therefore, we could lift the requirement in this case.

Alternatively, we could say that for a struct it is allowed to assign only to ref-extending properties. This scenario still be an error, but the error will be different. We should also keep an eye on an unconstrained generic type parameter case.

We also might want to be consistent with "Add" invocations in collection initializers and collection expressions. Do we require the receiver to be a variable?

Separately, we might want to consider to "see through" implicit span conversions for the purpose of determining what is a variable. If I am not mistaken, resulting spans refer to the same location as the converted variable, therefore could be considered a variable.

[333fred]

At the very least, the error is seemingly nonsense: this is a property. An extension one.

[jcouv]

At the very least, the error is seemingly nonsense: this is a property. An extension one.

I agree the message is bad, but that's an existing problem:

        src = """
new S().Property = 42;

struct S
{
    public int Property { set { } }
}
""";

        comp = CreateCompilation(src, targetFramework: TargetFramework.Net90);
        comp.VerifyEmitDiagnostics(
            // (1,1): error CS0131: The left-hand side of an assignment must be a variable, property or indexer
            // new S().Property = 42;
            Diagnostic(ErrorCode.ERR_AssgLvalueExpected, "new S().Property").WithLocation(1, 1));

[AlekseyTs]

comp.VerifyEmitDiagnostics();

It looks like we are getting expected diagnostics in RELEASE build. #Closed

[AlekseyTs]

LGTM (commit 3)

[jcouv]

@dotnet/roslyn-compiler for second review. Thanks