[SR-13098] Diagnostic improvement: encountering an unexpected statement at type scope

[typesanitizer]

Previous ID	SR-13098
Radar	rdar://problem/64947436
Original Reporter	@typesanitizer
Type	Improvement

Additional Detail from JIRA

Votes	0
Component/s	Compiler
Labels	Improvement, Parser
Assignee	dfsweeney (JIRA)
Priority	Medium

md5: 8298353d748ac718f859d5f973f05863

Issue Description:

Consider the following minimal example:

struct T {
    var x: Int
}
struct S {
    var t: T
    t.x = 3
    // error: expected 'func' keyword in instance method declaration
    // error: expected '(' in argument list of function declaration
    // error: consecutive declarations on a line must be separated by ';'
    // error: expected declaration
    // error: expected '{' in body of function declaration
    // error: invalid redeclaration of 't()'
}

A more realistic version can be seen here: https://developer.apple.com/forums/thread/651253

The cascade of errors is quite intimidating. We should:

1. Avoid the cascade of errors, hopefully providing only one or two errors.
2. Provide a more useful diagnostic. For example, we might want to point out that assignments are not permitted at class scope, they must be inside a function. We might also want to say that we expected a property or a function declaration here.

[typesanitizer]

@swift-ci create

[swift-ci]

Comment by Daniel Sweeney (JIRA)

Taking a look at this one if that's OK.

[typesanitizer]

Sure thing. Please feel free to ask for help as you need it. 🙂

[swift-ci]

Comment by Daniel Sweeney (JIRA)

trying some variations and reading the code:

struct Test_SR_13098 {
{{ var text = "Hello, World!" // Ok, no diagnostic obviously}}
{{ x = 3 // Expected 'var' keyword in property declaration}}
{{ whatever : Int = 34 // Expected 'var' keyword in property declaration}}
{{ self.x = 3 // Expected declaration}}
{{ somebody.text = "ok" // cluster of 5 diagnostics}}

The first few variations work OK. It's the property access with an identifier at the beginning that triggers the problem.

Parser::parseDecl in ParseDecl.cpp has:

const bool IsProbablyVarDecl =
{{ Tok.isIdentifierOrUnderscore() &&}}
{{ peekToken().isAny(tok::colon, tok::equal, tok::comma);}}

which is probably what's triggering the diagnostics we get, with another predicate someplace to get the behavior with self.

choices:

1. Add a case to the IsProbablyVarDecl test looking for a tok::period in the peekToken(), and possibly another peek forward to ensure that we have tok::period then identifier. That should generate the right diagnostic. That is simplest but might have some unforeseen consequences.

2. Get more sophisticated here, and check whether we can parse a statement at this point. This is after we drop through the valid cases in a declaration list (like let, var, func, etc.) So if we can parse a statement here we could do a diagnostic like "Statement found at top-level of struct" instead of "Expected ... declaration". I'm not sure if "Statement found" is a better diagnostic though. The "expected declaration" is reasonably clear to me, although I am looking at the code of the parser right now so am biased.

3. suggest something else than a var--if people put the id.id syntax here it's not unreasonable that they want it to do something, probably declare a var, but maybe they expect something else.

Thinking about the consequences to 1. above now.

[swift-ci]

Comment by Daniel Sweeney (JIRA)

Adding tok::period to the IsProbablyVarDecl peekToken call clears up some of these, and produces the expected 'var' keyword in property declaration diagnostic, but I still get type annotation missing in pattern, consecutive declarations on a line must be separated by ';', and expected declaration. The first of those is at the beginning of the expression with a dot (somebody above) and the second and third are at the dot (.text above). Previously it was assuming that it should be a function declaration and trying to fix that. Now it's trying to fix it as a var declaration, but the period is not really appropriate there.

I guess option 4. above would be to handle this situation separately without piggybacking on the IsProbablyVarDecl above. If we have a period assume it's a method or property reference in the wrong place and do a separate diagnostic for that. (I'm keeping notes here so I have some record of what I'm thinking. theindigamer (JIRA User) if you have suggestions or thoughts let me know but no obligation.

[typesanitizer]

Looking at the debugger, the IsProbablyFuncDecl case is being considered. The problem is that we don't have a function here. The heuristic for considering a function is very lax – we simply check if the next token is an identifier or an operator. I think we should at least check if it is immediately followed by an opening paren. If not, it is unlikely to be a function or operator decl where someone just forgot the keyword in front. So we would at least skip some of the diagnostics related to the function declaration.

Ideally, we would try parsing as a statement and if that succeeds, give the user an extra hint that we found a statement even though we were expecting a declaration. I don't know how easy that would be to implement though (and if it might lead to degradation of other diagnostics).

That said, I did try something like the following:

diff --git a/include/swift/AST/DiagnosticsParse.def b/include/swift/AST/DiagnosticsParse.def
index 1a79bcc0da3..3257d72a53a 100644
--- a/include/swift/AST/DiagnosticsParse.def
+++ b/include/swift/AST/DiagnosticsParse.def
@@ -219,2 +219,3 @@ ERROR(disallowed_var_multiple_getset,none,
       " getters/setters", ())
+NOTE(found_unexpected_stmt,none, "found a unexpected statement here", ())
 
diff --git a/lib/Parse/ParseDecl.cpp b/lib/Parse/ParseDecl.cpp
index 8c2b0a42e0c..9c9c876273a 100644
--- a/lib/Parse/ParseDecl.cpp
+++ b/lib/Parse/ParseDecl.cpp
@@ -3926,3 +3926,4 @@ Parser::parseDecl(ParseDeclOptions Flags,
       const bool IsProbablyFuncDecl =
-          Tok.isIdentifierOrUnderscore() || Tok.isAnyOperator();
+          (Tok.isIdentifierOrUnderscore() || Tok.isAnyOperator())
+          && peekToken().is(tok::l_paren);
 
@@ -3954,2 +3955,11 @@ Parser::parseDecl(ParseDeclOptions Flags,
       }
+
+      auto startLoc = Tok.getLoc();
+      auto startPosition = this->getParserPosition();
+      ParserResult<Stmt> tryParseAsStmt = this->parseStmt();
+      if (tryParseAsStmt.isParseError())
+        this->backtrackToPosition(startPosition);
+      else
+        diagnose(startLoc, diag::found_unexpected_stmt);
     }
 

However, this ends up (a) printing a diagnostic (undesirable, given that we might not actually have a statement) and (b) crashing with the following signature:

Assertion failed: (InitialLoc != P.Tok.getLoc() && "parser did not make progress, this can result in infinite loop"), function ~AssertParserMadeProgressBeforeLeavingScopeRAII, file /Users/varun/foss-swift/swift/include/swift/Parse/Parser.h, line 253.

I don't know why it didn't consume anything or why it failed to parse as a statement.

I don't have much experience working on the parser, so I'm somewhat unsure if it is possible to implement what I'm looking for without major changes...

[swift-ci]

Comment by Daniel Sweeney (JIRA)

Thanks Varun!

I was thinking about trying to parse a statement like you did above. I think declarations are statements in the grammar, so it's possible that parseStmt is trying to parse as a declaration and mutually recursing there. There might need to be a different SyntaxParsingContext or something to signal we're parsing speculatively or something--I'm not sure really. But that would add complication to the parser, so probably the wrong way to go. I'm not super familiar with the parser either but there's already a fair amount of state moving around in there.

I think catching and diagnosing this condition in the default: case of the switch statement in ParseDecl, without calling out to another function, is probably the way to go. But we need to catch it unambiguously and make sure we don't get extra diagnostics. I think some of the diagnostics I'm getting are generated somewhere else in the parser, but I'm going to track that later on today.

[swift-ci]

Comment by Daniel Sweeney (JIRA)

I put a pull request through just now at #32984 I left it as a draft--I hope you can see it.

I created a new case for the identifier.identifier case because the existing IsProbably? were trying to do a substitution, whereas this situation is not fixable. The extra diagnostics disappear if you handle the situation independently and end with a break.

Right now this is just a note but I can convert it to an error obviously. Also I think I dropped a line from one of the other test cases--I have to fix that.

I'm trying to think of other situations where this occurs. In this case we have the identifier.identifier syntax but there must be other situations where the existing code does not identify a valid declaration (with, I think, the beginning-of-line or after-semicolon beginning of context, then one of the `func`, `var`, etc. keywords.) that drop into the default case and generate complicated diagnostics.