Adjust Better Betterness rules around method group conversions in order to restore backward compatibility

[AlekseyTs]

Fixes #6560.

@dotnet/roslyn-compiler Please review.

[gafter]

Where can I find a discussion of the proposed revision to the rules? I don't know how to review this without a spec.

[AlekseyTs]

@gafter I guess the title isn't precise. There is no discussion, apart from what I, you and Mads had over e-mail. This fix implements a conservative approach, i.e. new Better Betterness rules do not apply if the winner, according to these rules, uses existing, but erroneous method group conversion. In other words, we revert to native compiler rules in cases like this.

[AlekseyTs]

Test prtest/lin/dbg/unit32 please.

[gafter]

There is a proposal for something like what you describe (#250), but is still in the works for a future version.

I don't think we could take this change without going through the LDM and likely some wordcrafting of the intended rules. Alternatively, we could take #250 as an intentional language change in 1.2.

[AlekseyTs]

@gafter I would still like to get the code change reviewed assuming the stated intent. I will wait for LDM decision before merge.

[AlekseyTs]

@dotnet/roslyn-compiler Please review.

[gafter]

Ok, I will review it with #250 as the spec.

[gafter]

This condition appears to be wrong. (!= should be ==)

I know you didn't touch it, but this makes me very afraid.

[AlekseyTs]

No, the condition is correct. At this point we know that r1 is void, if r2 is not void (hense !=), then r2 is better, which is the right side.

[gafter]

Then the comment just below is wrong?

[AlekseyTs]

The comment is the quote of the applicable rule from the spec as is. It refers to entities from the comment above (also a quote from the spec):

                     // - T1 is either a delegate type D1 or an expression tree type Expression<D1>,
                     //   T2 is either a delegate type D2 or an expression tree type Expression<D2>,
                     //   D1 has a return type S1 and one of the following holds:

[gafter]

The comment below says "D2 is void-returning", but it is inside an if-statement that ensures that D2 does not return void.

[gafter]

(@AlekseyTs explained to me in person that the comments are quotes from the spec, and are not intended to mirror variables in the code; the code typically checks conditions in both directions for each rule)

[AlekseyTs]

@dotnet/roslyn-compiler Please review. LDM gave a green light for the fix.

Here is a "spec" approved by LDM:

In case of a method group conversion for the corresponding argument, if a better conversion target (section 7.5.3.5 Better conversion target), is a delegate type incompatible (section 15.2 Delegate compatibility) with the single best method from the method group (section 6.6 Method group conversions), then neither delegate type is better.

Here are relevant quotes from the language spec:
7.5.3.5 Better conversion target
Given two different types T1 and T2, T1 is a better conversion target than T2 if
...

• T1 is either a delegate type D1 or an expression tree type Expression, T2 is either a delegate type D2 or an expression tree type Expression, D1 has a return type S1 and one of the following holds:
  o D2 is void returning
  o D2 has a return type S2, and S1 is a better conversion target than S2
  ...

15.2 Delegate compatibility
A method or delegate M is compatible with a delegate type D if all of the following are true:

• D and M have the same number of parameters, and each parameter in D has the same ref or out modifiers as the corresponding parameter in M.
• For each value parameter (a parameter with no ref or out modifier), an identity conversion (§6.1.1) or implicit reference conversion (§6.1.6) exists from the parameter type in D to the corresponding parameter type in M.
• For each ref or out parameter, the parameter type in D is the same as the parameter type in M.
• An identity or implicit reference conversion exists from the return type of M to the return type of D.

6.6 Method group conversions
...
• A conversion is considered to exist if the algorithm of §7.6.5.1 produces a single best method M having the same number of parameters as D.
...

[gafter]

LGTM, including spec changes (which either you or I will place into the compiler specification). If you want me to take care of the documentation please assign #6560 to me for verification after this is integrated.

[AlekseyTs]

@dotnet/roslyn-compiler Please review, need a second sign-off.

[AlekseyTs]

Ping. @dotnet/roslyn-compiler Please review, need a second sign-off.

[VSadov]

LGTM