Better betterness #484
Better betterness #484
Conversation
This commit adds the notes from https://github.com/dotnet/roslyn/blob/main/docs/specs/CSharp%206/Better%20Betterness.md
Issue dotnet#283 contains notes for how the implementation differed from the proposed better betterness text.
This commit brings in the method group conversion fix that caused dotnet/roslyn#6750 It needs wordsmithing for a number of reasons: - "corresponding argument" isn't defined. - the section references itself. - I think there may have been requirements from the prior commit regarding delegates and expressions that are still needed.
| - An inferred return type `X` exists for `E` in the context of the parameter list of `D` (§11.6.3.12), and an identity conversion exists from `X` to the return type of `D` | ||
| - Either `E` is non-async and `D` has a return type `Y` or `E` is async and `D` has a return type `Task<Y>`, and one of the following holds: | ||
| - The body of `E` is an expression that exactly matches `Y` | ||
| - The body of `E` is a statement block where every return statement returns an expression that exactly matches `Y` |
There was a problem hiding this comment.
Do we need "statement block" rather than just "block"?
There was a problem hiding this comment.
ping @RexJaeschke to check if "statement block" or "block" is better.
From Jon: There are two other occurrences of "statement block" that should be changed. Jon will create an issue for that change.
| - `D₁` and `D₂` have identical parameter lists, and one of the following holds: | ||
| - `D₁` has a return type `Y₁`, and `D₂` has a return type `Y₂`, an inferred return type `X` exists for `E` in the context of that parameter list ([§11.6.3.13](expressions.md#116313-inferred-return-type)), and the conversion from `X` to `Y₁` is better than the conversion from `X` to `Y₂` | ||
| - `E` is async, `D₁` has a return type `Task<Y₁>`, and `D₂` has a return type `Task<Y>`, an inferred return type `Task<X>` exists for `E` in the context of that parameter list ([§11.6.3.13](expressions.md#116313-inferred-return-type)), and the conversion from `X` to `Y₁` is better than the conversion from `X` to `Y₂` | ||
| - `D₁` has a return type `Y`, and `D₂` is void returning |
There was a problem hiding this comment.
In the current Standard we have delegate return type of Y, presumably non-void, being a better conversion than a return type of void. I don’t immediately see this case exists in the revised model, is this intentional? A breaking change?
There was a problem hiding this comment.
I've now found the void case is in the source material Better betterness so I guess this was unintentional?
There was a problem hiding this comment.
This change is intentional, and it was a regression in the original Better Betterness spec. The issue is dotnet/roslyn#6750. There is still work to do here, because I think this loses some other requirements.
There was a problem hiding this comment.
Case that I think is broken by this:
void M(Action action) {}
void M(Func<object> func) {}
void Test()
{
// "xyz" doesn't *exactly match* object, so this should be ambiguous (according to the PR)
M(() => "xyz");
}Case that I think isn't:
void M(Action action) {}
void M(Func<object> func) {}
void Test()
{
// new object() exactly matches object, therefore Func<object> is preferred
M(() => new object());
}
This gets close to what's implemented in C# 6.0. I think this fails as a description in the case of dotnet#499 For 7.3, it would be better to describe that change in the section on Method Group conversion.
|
Adding a comment on the roadmap for much of what I've been reading to reach this point.
|
|
Marking as ready for review, and adding Mads and Neal as reviewers. Consensus from today's meeting was that this is close, and any future updates can be handled in a separate PR (for C# 6) |
standard/expressions.md
Outdated
| - `S₁` is `short` and `S₂` is `ushort`, `uint`, or `ulong` | ||
| - `S₁` is `int` and `S₂` is `uint`, or `ulong` | ||
| - `S₁` is `long` and `S₂` is `ulong` | ||
| - `E` is a delegate creation expression (§11.7.15.6) resulting from a method group conversion and `T₁` is compatible (§19.4) with the single best method from the method group (§10.8) |
There was a problem hiding this comment.
Ping for any updates on this? It would be really good to get this PR done and merged this week, so we can get the Word version out next week.
standard/expressions.md
Outdated
| - `S₁` is `short` and `S₂` is `ushort`, `uint`, or `ulong` | ||
| - `S₁` is `int` and `S₂` is `uint`, or `ulong` | ||
| - `S₁` is `long` and `S₂` is `ulong` | ||
| - `E` is a delegate creation expression (§11.7.15.6) resulting from a method group conversion and `T₁` is compatible (§19.4) with the single best method from the method group (§10.8) |
There was a problem hiding this comment.
Ping for any updates on this? It would be really good to get this PR done and merged this week, so we can get the Word version out next week.
There was a problem hiding this comment.
I'm ready to merge, if you are.
I've pinged Mads offline for a review as well.
There was a problem hiding this comment.
But this still has "delegate creation expression" where I thought we just wanted it to handle method group conversions...
There was a problem hiding this comment.
Gotcha. I misunderstood your earlier comment.
Update coming this morning....
|
I'll merge this on the analysis that it's better than what we had before. This area may still need some edits. |
Fixes #157
Fixes #283
This takes the text from Better betterness and then updates it for compatibility fixes, related to exact matches and method group conversions.
Reviewing each commit in order clarifies what changes came from the roslyn doc, and then those changes that followed after incorporating that initial spec. Commit messages link to dotnet/roslyn#6750 that explains the later changes.