Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix 'else without if' parsing - better error msg #28115

Merged

Conversation

wachulski
Copy link
Contributor

@wachulski wachulski commented Jun 26, 2018

[Note: there are merge commits in this PR, so it should not be squashed. -- jcouv]

Instead of a general syntax error } expected message we want
sth like Syntax error 'if' expected.

Fixes #27866

@wachulski wachulski requested a review from a team as a code owner June 26, 2018 11:22
@wachulski wachulski changed the title Fix 'else without if' parsing - better error msg WIP: Fix 'else without if' parsing - better error msg Jun 26, 2018
// Semantic model has a special case here that we match: if the underlying syntax is missing, don't create a conversion expression,
// and instead directly return the operand, which will be a BoundBadExpression. When we generate a node for the BoundBadExpression,
// the resulting IOperation will also have a null Type.
Debug.Assert(boundOperand.Kind == BoundKind.BadExpression ||
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I need your help, please.

Trying to copy the idea from 'try-catch-finally' parsing I created missing if (true) {} for existing else clause. Then, I got the runtime error of failing an assert.

Could you suggest what I should do with this?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which test failed with an assert?

From the CI log, I see only one test that failed (IIfstatementWithStatementMissing). I think that test expected some parsing diagnostics which just need to be updated.


In reply to: 198106984 [](ancestors = 198106984)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was live debugging session in VS. Repro code as in the image attached to the description of this PR.

Anyway, I'll try what @CyrusNajmabadi suggests first.

@jcouv jcouv added Area-Compilers Community The pull request was submitted by a contributor who is not a Microsoft employee. labels Jun 26, 2018
SyntaxToken.CreateMissing(SyntaxKind.OpenBraceToken, null, null),
default,
SyntaxToken.CreateMissing(SyntaxKind.CloseBraceToken, null, null));
}
Copy link
Member

@jcouv jcouv Jun 26, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add some parsing tests to demonstrate the shape of the parse tree after the change. Look at StatementParsingTests.ParsePrivate as an example.

Note that the M method will print out the correct code, but you first have to uncomment #define PARSING_TESTS_DUMP in src\Compilers\CSharp\Test\Syntax\Parsing\ParsingTests.cs. #Closed

@jcouv jcouv self-assigned this Jun 26, 2018
@jcouv jcouv added this to the 16.0 milestone Jun 26, 2018
@CyrusNajmabadi
Copy link
Member

CyrusNajmabadi commented Jun 26, 2018

@jcouv Can you help shephard this in? #Closed

@jcouv
Copy link
Member

jcouv commented Jun 26, 2018

@CyrusNajmabadi Yes, that's what I'm doing. #Closed

elseClause = this.AddErrorToFirstToken(elseClause, ErrorCode.ERR_SyntaxError, SyntaxFacts.GetText(SyntaxKind.IfKeyword));

// synthesize missing tokens for "if (true) { }":
@if = SyntaxToken.CreateMissing(SyntaxKind.IfKeyword, null, null);
Copy link
Member

@CyrusNajmabadi CyrusNajmabadi Jun 26, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this seems redundant. You could just always call @if = this.EatToken(SyntaxKind.IfKeyword); above. It will do the right thing if you are/aren't on an 'if' keyword. This also means you won't need to manually add the 'ERR_SyntaxError' to the 'elseClause'.

You can do the same for all the rest of the required bits of the else. #Closed

statement = _syntaxFactory.Block(
SyntaxToken.CreateMissing(SyntaxKind.OpenBraceToken, null, null),
default,
SyntaxToken.CreateMissing(SyntaxKind.CloseBraceToken, null, null));
Copy link
Member

@CyrusNajmabadi CyrusNajmabadi Jun 26, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would not make a block here. Remember that if-statements don't require blocks for the if/else sections. Instead, just call ParseExpressionStatement. Also, add .ElseKeyword to IsInvalidSubExpression. #Closed

openParen = SyntaxToken.CreateMissing(SyntaxKind.OpenParenToken, null, null);
condition = this._syntaxFactory.LiteralExpression(
SyntaxKind.TrueLiteralExpression,
SyntaxToken.CreateMissing(SyntaxKind.TrueKeyword, null, null));
Copy link
Member

@CyrusNajmabadi CyrusNajmabadi Jun 26, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't make a true-expression. just call ParseExpression(). #Closed

Copy link
Member

@CyrusNajmabadi CyrusNajmabadi Jun 26, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Basically, just keep the parsing logic the same as it was before except for the point that you'll call statement = this.ParseEmbeddedStatement();. There, you want to just check if you're on an 'else' keyword. In that case, you don't want to call ParseEmbeddedStatement (since you now support 'else' starting a statement). so i would expect:

           var @if = this.EatToken(SyntaxKind.IfKeyword);
           var openParen = this.EatToken(SyntaxKind.OpenParenToken);
           var condition = this.ParseExpressionCore();
           var closeParen = this.EatToken(SyntaxKind.CloseParenToken);
           // comment this line appropriately.
           var statement = this.CurrentToken.Kind == SyntaxKind.ElseKeyword ? this.ParseExpressionStatement() : this.ParseEmbeddedStatement();
           var elseClause = ParseElseClauseOpt();
``` #Closed

Copy link
Contributor Author

@wachulski wachulski Jun 27, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks guys! That helped a lot. Although I see it generates many more syntax error entries (like missing (, ) etc.) but I also see this is a common approach here. At least syntax error, missing if comes first in such series and therefore should put people on good track of finding the root cause. #Closed

Copy link
Member

@CyrusNajmabadi CyrusNajmabadi Jun 27, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yup yup! #Closed

@wachulski wachulski force-pushed the fix/27866/else-without-if-error-message branch from 948fe8d to e693854 Compare June 28, 2018 08:15
@wachulski wachulski changed the title WIP: Fix 'else without if' parsing - better error msg Fix 'else without if' parsing - better error msg Jun 28, 2018
@chborl
Copy link
Contributor

chborl commented Jun 28, 2018

@dotnet-bot retest windows_debug_vs-integration_prtest please #Closed

var statement = this.ParseEmbeddedStatement();
var elseClause = ParseElseClauseOpt();

// when 'if' is missing, then we parse an expression statement in a stadard way (which adds missing syntax properly)
Copy link
Member

@333fred 333fred Jun 28, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

stadard [](start = 80, length = 7)

stadard [](start = 80, length = 7)

Typo: stadard -> standard #Closed

@333fred
Copy link
Member

333fred commented Jun 28, 2018

@wachulski could you please add some IOperation tests for similar scenarios to your syntax tests? You'll want to add them in src/Compilers/CSharp/Test/Semantic/IOperation/IOperationTests_IIfStatement.cs. To create the test method:

  • Copy/paste an existing test method and rename. Make sure you use a standard IOperation test and not a dataflow test, like https://github.com/dotnet/roslyn/blob/master/src/Compilers/CSharp/Test/Semantic/IOperation/IOperationTests_IIfStatement.cs#L15.
  • Modify the test source. You'll need a full program, not just a one-liner like syntax tests. My suggestion is just a simple method with a body of the if/else/else or other structure.
  • Surround the section you want to test with /*<bind>*/ ... /*</bind>*/. I suggest using the entire method body.
  • The VerifyOperationTreeForTest method has a generic parameter, which is the type of SyntaxNode surrounded by the bind tags. If you surround the whole method body, this will be BlockSyntax.
  • Run the test a couple of times. The first time will likely fail due to missing diagnostics, copy those over. The second time it will fail because the expected operation tree is incorrect, copy over the actual operation tree.

Feel free to ping me if you run into any issues. I'd guess you're very likely to run into binding issues, and when those are resolved I'd guess you're likely to run into IOperation factory issues :). My gut feeling is that, for this case, IOperation should probably produce an IInvalidOperation, with things in the block being children of that block. #Closed

N(SyntaxKind.OpenBraceToken);
N(SyntaxKind.CloseBraceToken);
}
N(SyntaxKind.ElseClause);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ElseClause [](start = 29, length = 10)

The second else { } isn't captured, since this code parses as two statement. You may need to wrap the whole thing in a block { ... } or use UsingTree instead of UsingStatement.

@jcouv
Copy link
Member

jcouv commented Jun 28, 2018

@wachulski Aside from Fred's suggestion, I have a small test nit. Otherwise things look good. Thanks

I'm going to retarget this PR to the features/compiler branch, because master is currently meant for 15.8 which is close to shipping (so the bug bar is high). #Closed

@jcouv jcouv changed the base branch from master to features/compiler June 28, 2018 20:38
@wachulski wachulski force-pushed the fix/27866/else-without-if-error-message branch from e693854 to f605c4d Compare June 28, 2018 21:05
[Fact]
public void ParseElseKeywordPlacedAsIfEmbeddedStatement()
{
UsingStatement("if (a) else {}",
Copy link
Member

@CyrusNajmabadi CyrusNajmabadi Jun 28, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we have an "else { } else { }" as well? #Closed

Copy link
Contributor Author

@wachulski wachulski Jun 29, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is ParseSubsequentElseWithoutPrecedingIfStatement already with its UsingStatement("{ if (a) { } else { } else { } }". Do you mean UsingStatement("{ if (a) else { } else { } }"? #Closed

Copy link
Member

@CyrusNajmabadi CyrusNajmabadi Jun 29, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope. just else { } else { } :) #Closed

Copy link
Contributor Author

@wachulski wachulski Jun 29, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here you go: b17e8b7 #Closed

@wachulski wachulski force-pushed the fix/27866/else-without-if-error-message branch from f605c4d to 1ef941a Compare June 29, 2018 08:34
@wachulski
Copy link
Contributor Author

wachulski commented Jun 29, 2018

I've been wondering whether IOperation tests that output sth like (Syntax: '/*<bind>*/e ... }') are OK, that is there are only my 2 new added UTs that produce such fragment containing '/*<bind>*/ #Closed

@333fred
Copy link
Member

333fred commented Jun 29, 2018

@wachulski if I understand your question correctly, yes it's expected that some of the nodes in the new IOperation nodes will have Syntax in the output that starts with the /*<bind>*/ trivia. We trim the Syntax when dumping the IOperation nodes, so you're only seeing a small part of the actual syntax associated with the node. #Closed

/*</bind>*/ }
}
";
string expectedOperationTree = @"
Copy link
Member

@jcouv jcouv Jul 3, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

expectedOperationTree [](start = 19, length = 21)

Why aren't the second else and its block appearing in the output? #Closed

Copy link
Contributor Author

@wachulski wachulski Aug 9, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried to bind 2 operation statements. In 3016d83 I wrapped them in a block statement. #Closed

}
else
{
Op();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Op(); [](start = 12, length = 5)

I would have expected the invocation of Op() to appear in the output. Do you know why it's missing?

Copy link
Member

@jcouv jcouv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done with review pass (iteration 2).

@jcouv
Copy link
Member

jcouv commented Jul 11, 2018

@wachulski I didn't hear back from you on a couple of remaining questions. Thanks #Closed

Copy link
Member

@jcouv jcouv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM Thanks (iteration 3)
I'll let @AlekseyTs and @333fred comment on whether this warrants some control-flow graph (CFG) tests too.

@333fred
Copy link
Member

333fred commented Oct 29, 2018

@wachulski sorry for the delay here. I meant to take a look a month ago, and it slipped off my radar. I'm looking now. #Closed

Copy link
Member

@333fred 333fred left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM (commit 3). @CyrusNajmabadi, did you have any other comments?

@333fred 333fred closed this Oct 29, 2018
@333fred 333fred reopened this Oct 29, 2018
@333fred
Copy link
Member

333fred commented Oct 29, 2018

(Reopened to make sure that status checks were up to date) #Closed

@333fred
Copy link
Member

333fred commented Oct 29, 2018

@wachulski, can you please retarget this to master? #Closed

@@ -6613,6 +6613,7 @@ private StatementSyntax ParseStatementNoDeclaration(bool allowAnyExpression)
case SyntaxKind.GotoKeyword:
return this.ParseGotoStatement();
case SyntaxKind.IfKeyword:
case SyntaxKind.ElseKeyword:
Copy link
Member

@CyrusNajmabadi CyrusNajmabadi Oct 29, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there should probably be a comment here explaining that this is to handle error cases. #Closed


// when 'if' is missing, then we parse an expression statement in a standard way (which adds missing syntax properly)
// parsing embedded one expects non-null statement to be present
var statement = this.CurrentToken.Kind == SyntaxKind.ElseKeyword ? this.ParseExpressionStatement() : this.ParseEmbeddedStatement();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this seems to be strange (and indicates to me a change in behavior. specifically if you had if (a) else. Was it intentional? I think what you were trying to do was something more like:

var firstTokenIsElse = this.currentToken == .ElseKeyword;
var @if = this.EatToken(SyntaxKind.IfKeyword);
var openParen = this.EatToken(SyntaxKind.OpenParenToken);
var condition = this.ParseExpressionCore();
var closeParen = this.EatToken(SyntaxKind.CloseParenToken);
var statement = firstTokenIsElse ? ...

?

Copy link
Member

@CyrusNajmabadi CyrusNajmabadi Oct 29, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note: if you do want the current form, then the comment needs to be fixed up Because it applies to both when an 'if' is missing, and when tehre is no statement following the close-paren. #Closed

Copy link
Contributor Author

@wachulski wachulski Oct 31, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I followed your #28115 (comment) but most likely missed the point in the comment. I'll change the code to the what you proposed last. #Closed

@gafter
Copy link
Member

gafter commented Oct 29, 2018

I think this needs to be retargeted to master before being integrated, as we are no longer using the features/compiler branch. #Closed

…out-if-error-message

* origin/master: (1712 commits)
  User-defined operator checks should ignore differences in tuple member names (dotnet#30774)
  Attempted fix for correctly reporting error CS1069 when using implicit namespaces (dotnet#30244)
  Invert the binding order of InitializerExpressions and CreationExpressions (dotnet#30805)
  Use Arcade bootstrapping scripts (dotnet#30498)
  Ensure that the compilers produce double.NaN values in IEEE canonical form. (dotnet#30587)
  Remove properties set in BeforeCommonTargets.targets
  Fix publishing of dependent projects
  Contributing page: reference Unix build instructions
  Delete 0
  Propagate values from AbstractProject to VisualStudioProjectCreationInfo
  Fix publishing nuget info of dev16.0.x-vs-deps branch
  Revert "Add a SetProperty API for CPS to passing msbuild properties"
  Validate generic arguments in `using static` directives (dotnet#30737)
  Correct 15.9 publish data
  Enable test.
  Do not inject attribute types into .Net modules.
  Add a SetProperty API for CPS to passing msbuild properties
  Revert "add beta2 suffix to dev16 branch"
  Fix references
  Remove commit sha from package versions
  ...
@wachulski wachulski requested a review from a team as a code owner October 31, 2018 12:44
@wachulski wachulski changed the base branch from features/compiler to master October 31, 2018 14:03
@wachulski
Copy link
Contributor Author

wachulski commented Oct 31, 2018

@gafter Retargeted #Closed

@gafter
Copy link
Member

gafter commented Oct 31, 2018

It appears there are a couple of failing unit tests. #Closed

@wachulski
Copy link
Contributor Author

wachulski commented Nov 1, 2018 via email

Copy link
Contributor Author

@wachulski wachulski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Failing tests should be fixed now. I'm worried about the output for one edge case, details below.

@@ -3021,6 +3021,18 @@ public void ParseSubsequentElseWithoutPrecedingIfStatement()
public void ParseElseKeywordPlacedAsIfEmbeddedStatement()
{
UsingStatement("if (a) else {}",
// (1,8): error CS1003: Syntax error, 'if' expected
Copy link
Contributor Author

@wachulski wachulski Nov 6, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO interesting (potentially misleading). I understand the original issue was about lacking missing if indication for misplaced elses. This one however for "if (a) else {}" outputs // (1,8): error CS1003: Syntax error, 'if' expected as the first diagnostic. Isn't this sth unwanted?

This was added by following the last suggestion by @CyrusNajmabadi #Closed

Copy link
Member

@CyrusNajmabadi CyrusNajmabadi Nov 6, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could customize the error message here. EatToken takes an optional error message. If we see an 'else' we could just call EatToken(SyntaxKind.IfToken, specialId); where 'specialId' is the resource id for "'else' unexpected" or something like that. #Closed

Copy link
Member

@CyrusNajmabadi CyrusNajmabadi Nov 6, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or: "'else' cannot start a statement"

#Closed

Copy link
Contributor Author

@wachulski wachulski Nov 7, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I'll add the message #Closed

Copy link
Contributor

@Neme12 Neme12 Nov 7, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is wrong with this error message? It sounds pretty clear to me. #Closed

Copy link
Contributor

@Neme12 Neme12 Nov 7, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah I see, the example is if (a) else { }? That is indeed odd (even though it makes sense). I would expect something like "missing statement". #Closed

…out-if-error-message

* origin/master: (174 commits)
  Add Compilers filter for Roslyn (dotnet#30880)
  Remove NonNullTypes context and other unnecessary information stored in TypeSymbolWithAnnotations. (dotnet#30913)
  Update BoundCall method based on receiver nullability (dotnet#31000)
  Make nullabiulity inference associative and commutative (dotnet#30990)
  Async-streams: minimal test for IOperation and CFG. Improve diagnostics (dotnet#30363)
  Add src.
  Add test.
  Remove dead code
  Update the build status table
  Script for generating our build status tables
  Add parsing tests to compiler benchmarks (dotnet#31033)
  Fix Edit and Continue in CPS projects
  Add comment
  Sorting.
  Save work
  Address PR feedback
  only produce optprof data on an official build
  Add bunch of exclusions to dead code analysis for special methods
  Disable WinRT tests on Linux (dotnet#31026)
  Change prerelease version to beta2 (dotnet#31017)
  ...

# Conflicts:
#	src/Compilers/CSharp/Portable/CSharpResources.resx
#	src/Compilers/CSharp/Portable/Errors/ErrorCode.cs
@wachulski
Copy link
Contributor Author

wachulski commented Nov 12, 2018

Ok, I changed the message. Now it's 'else' cannot start a statement. I'm still not convinced (i.e. maybe the previous version 'if' expected was actually better), as the original issue was about this:

while(true)
{
    if (false)
    {

    }
}
else {

}

Expected Behavior:
Error says something regarding else without a connected if
Actual Behavior:
CS 1513 } expected

What's your thought?

Copy link
Member

@333fred 333fred left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM (commit 8)

Copy link
Member

@jcouv jcouv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM Thanks (iteration 8)

@jcouv jcouv merged commit 8176685 into dotnet:master Nov 12, 2018
@jcouv
Copy link
Member

jcouv commented Nov 12, 2018

Merged. Thanks @wachulski

@jcouv jcouv modified the milestones: 16.0, 16.0.P3 Nov 12, 2018
@jcouv
Copy link
Member

jcouv commented Nov 12, 2018

Note: this will first show up in dev16 preview3.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Compilers Community The pull request was submitted by a contributor who is not a Microsoft employee.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants