Fix parsing an expression-bodied property with async as the type#25704
Fix parsing an expression-bodied property with async as the type#25704jcouv merged 3 commits intodotnet:dev15.7.xfrom
Conversation
|
PDBUsingTests failures don't seem to be related (passed locally). |
|
@jaredpar I see these failures in my other PR as well. Is this a known issue on |
| } | ||
|
|
||
| [Fact] | ||
| [WorkItem(16044, "https://github.com/dotnet/roslyn/issues/16044")] |
There was a problem hiding this comment.
Please see TestPartiallyWrittenConstraintClauseInBaseList4 for a preferred style for writing such tests.
In order to author such a test, temporarily uncomment out the #define line at the top of ParsingTests.cs.
There was a problem hiding this comment.
preferred style for writing such tests.
Didn't know that's preferable.
Done
|
@dotnet/roslyn-compiler Can we please have another review of this community-offered bug fix? |
|
@dotnet/roslyn-infrastructure Is this issue with PDBUsingTests known? |
|
I think it's "known" in the sense of "we just saw it fail in official builds an hour ago too, but don't have a better answer yet". @JieCarolHu can give a further update. |
|
Please add a test for this case: class Frog : I
{
async I.T => null; // incorrect syntax error
}
interface I
{
async T
{
get;
}
}
class async
{
} |
| identifierOrThisOpt != null && | ||
| (typeParameterListOpt != null && typeParameterListOpt.ContainsDiagnostics | ||
| || this.CurrentToken.Kind != SyntaxKind.OpenParenToken && this.CurrentToken.Kind != SyntaxKind.OpenBraceToken) && | ||
| || this.CurrentToken.Kind != SyntaxKind.OpenParenToken && this.CurrentToken.Kind != SyntaxKind.OpenBraceToken && this.CurrentToken.Kind != SyntaxKind.EqualsGreaterThanToken) && |
There was a problem hiding this comment.
this is getting complex enough that i think extracting woudl be valuable. But up to you.
There was a problem hiding this comment.
One benefit of extraction is htat each case can be individually checked, with a nice comment explaining the real language construct this is looking for. I find that that is easier to read, review, and most importantly: maintain. Someone can see those checks and immediately start thinking to themselves "is there a another common case that is missing here?"
There was a problem hiding this comment.
For example, what if i have "async this[...]". Do we have a problem here? you should have "identifierOrThisOpt"... but you'd have an OpenBracketToken not an openbrace.
There was a problem hiding this comment.
Note: if the indexer case works, it woudl still probably be helpful to add tests in this area for 'async' as the return type for all sorts of language constructs (i.e. operators, properties, indexers, etc.).
There was a problem hiding this comment.
I should read this piece to understand why we need to reconsider "async" to be a modifier in the first place. With partial there were no such ambiguities. @CyrusNajmabadi do you have any idea what we're trying to do here?
There was a problem hiding this comment.
nope! hence why i wish this stuff was broken out into specific, documented, cases. Because trying to infer after the fact is super tough in these codeblocks that deal with very ambiguous code.
There was a problem hiding this comment.
Alright, turns out it was doing somersault on ambiguities that we already dealt with for partial. Now it's hugely simplified and gives more accurate results: alrz@6a4d586
I figured the ScanPartial acts as a IsTrueModifier - just gives more details. that's why we can share many parts with ShouldAsyncTreatedAsModifier, however, the later has some specific preference that make these not so identical. I'll open a PR for further discussion when the other two are merged.
| [WorkItem(16044, "https://github.com/dotnet/roslyn/issues/16044")] | ||
| public void AsyncAsType_ExpressionBodiedProperty() | ||
| { | ||
| UsingTree("class a { async b => null; }"); |
There was a problem hiding this comment.
wow, that breaks my brain :)
|
I'm planning to do the same work for async that is being done for partial, to improve diagnostics. If there is no rush, I'll wait for that PR to merge first. |
|
test this please |
|
Consider: Which is preferable, |
|
@jaredpar Please approve for 15.7 ask-mode. Thanks |
|
Not actionable failures. |
|
(It's possible we skip this PR in favor of alrz@6a4d586 which is based on top of #23533) |
|
Test windows_debug_vs-integration_prtest please |
|
Test windows_release_vs-integration_prtest please |
|
CC @jaredpar For approval |
|
approved |
Fixes #16044