Skip to content

SR-13098 Diagnostic improvement: encountering an unexpected statement at type scope #32984

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.

Sign up for GitHub

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

Closed
wants to merge 10 commits into from
Closed

SR-13098 Diagnostic improvement: encountering an unexpected statement at type scope #32984

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
7da4d8c
Added note and logic for IsProbablyStatement.
dfsweeney Jul 18, 2020
b849800
Replaced one line in unit tests that was inadvertently removed.
dfsweeney Jul 19, 2020
baf14e6
Changed logic for IsProbablyFuncDecl to require left-parenthesis or l…
dfsweeney Jul 19, 2020
e484569
Reverted to a simple fail-fast on identifier followed by period.
dfsweeney Jul 20, 2020
9501414
Added parseExpr call to try to parse over identifier-period statement…
dfsweeney Jul 22, 2020
92bb52c
Returned a parser error when we find the IdentifierPeriod condition. …
dfsweeney Jul 23, 2020
608ac92
Removed an expected-error from a test to ensure it passes because I c…
dfsweeney Jul 24, 2020
8a56bb8
First pass at testing for probable functions.
dfsweeney Jul 26, 2020
1be6f2c
Converted the test for a property reference at type-level to a probe …
dfsweeney Aug 6, 2020
100c8fe
Fixed a column error in `test/Parse/diagnostic_missing_func_keyword.s…
dfsweeney Sep 6, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 40 additions & 2 deletions lib/Parse/ParseDecl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
#include "swift/AST/LazyResolver.h"
#include "swift/AST/DebuggerClient.h"
#include "swift/AST/DiagnosticsParse.h"
#include "swift/AST/DiagnosticSuppression.h"
#include "swift/AST/Initializer.h"
#include "swift/AST/Module.h"
#include "swift/AST/ParameterList.h"
Expand Down Expand Up @@ -3926,9 +3927,46 @@ Parser::parseDecl(ParseDeclOptions Flags,
break;
}

const bool IsProbablyFuncDecl =
Tok.isIdentifierOrUnderscore() || Tok.isAnyOperator();
bool IsProbablyFuncDecl = false;

if (Tok.isIdentifierOrUnderscore() || Tok.isAnyOperator()) {

// This is a probe, so backtrack when we're done
// and don't emit diagnostics.
BacktrackingScope backtrackingScope(*this);
DiagnosticSuppression diagnosticSuppression(Diags);

// TODO make sure we can't get these from the environment
SourceLoc staticLoc;
StaticSpellingKind staticSpelling = StaticSpellingKind::None;
DeclAttributes attributes;
bool hasFuncKeyword = false;

ParserResult<FuncDecl> parserResult =
parseDeclFunc(staticLoc,
staticSpelling,
Flags,
attributes,
hasFuncKeyword);

// it looks like we always get a good parse result.
// But if there are no parentheses
// the left and right parens will be at the beginning of
// the expression and will be equal so we use
// this to reject degenerate declarations.

if (parserResult.isNonNull()) {
auto parameterList = parserResult.get()->getParameters();
if (parameterList->getLParenLoc() != parameterList->getRParenLoc()) {
IsProbablyFuncDecl = true;
}
}
Comment on lines +3952 to +3963
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't quite follow the reasoning here. Why is the degenerate case rejected? Also, if the parser always succeeds, there must be something wrong with it... Not every syntactic form is a valid function declaration.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, this is confusing. parseDeclFunc returns a non-null parse result even when it does not completely parse a function. It's pretty liberal in what it will accept. That's part of why this was happening originally--it thought the statement was a function then tried to parse it and sent out a lot of confusing diagnostics.

The situation we are guarding against is a statement at the type level when there should be a function. parseDeclFunc is returning a non-null ParserResult<FuncDecl> in that situation and the only indicator (empirically) is that the left and right paren locations are identical in the ParserResult<FuncDecl>.

I did not re-run all this through the debugger today. But as I remember it the parentheses are not the sensitive point--there are a couple of things that can happen in parseFuncDecl and still return a ParserResult<FuncDecl>. The location of the parens is just the differentiator the situation where we expect a function decl and can't parse one. You might also get a null parserRequest in other situations.

Maybe parseFuncDecl should be tighter in what it accepts and return null when it does not get a full declaration. I'm not sure right how if that change would break anything else though. The code is not that intricate but the effects could be large.

I'm sort of assuming that the normal clients of parseFuncDecl want to get as much information as possible out of it, while for this specific problem we want to inhibit some of those situations in the probe to skip to our diagnostic.

Does that make sense?

// backtrackingScope and diagnostic suppression are both RAII
// when they go out of scope, Tok returns to the beginning of
// the identifier, and diagnostics are unsuppressed.
Comment on lines +3964 to +3966
Copy link
Contributor

Choose a reason for hiding this comment

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

We generally try to avoid comments which explain what is going on (which can be understood by reading the code) and try to focus on the why, unless something is particularly complicated.

I would avoid this comment and similar comments above and below.

// This is a probe, so backtrack when we're done
// and don't emit diagnostics.

// if probe succeeded, then do it for real

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with you that the specific comment you’re marking here (about RAII variables) is unnecessary, but I actually think the comments about probing are helpful “why” comments. (Maybe they could be shortened slightly—“This is a probe, so we will always backtrack after we’re done.” is definitely a helpful “why” comment.)

}

// if probe succeeded, then do it for real
if (IsProbablyFuncDecl) {

DescriptiveDeclKind DescriptiveKind;
Expand Down
12 changes: 12 additions & 0 deletions test/Parse/diagnostic_call_at_type_level.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
// RUN: %target-typecheck-verify-swift

struct Bar { // expected-note {{in declaration of 'Bar'}}
var fisr = 0x5F3759DF
var a: Int

// ensure that the id.id pattern is not interpreted as a function
a.foo = 345 // expected-error {{expected declaration}}
// ensure that the id.id pattern generating an expected declaration
// diagnostic does not block further diagnostics.
fisr.bar = 345
}
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. We can move this test into the diagnostic_missing_func_keyword file, since it's very much related.
  2. You could give the struct a more descriptive name like SR13098.

4 changes: 2 additions & 2 deletions test/Parse/diagnostic_missing_func_keyword.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,10 +34,10 @@ struct Bar {
}

_: Int = 42 // expected-error {{expected 'var' keyword in property declaration}} {{3-3=var }}
// expected-error @-1 {{property declaration does not bind any variables}}
// expected-error @-1 {{property declaration does not bind any variables}}

(light, dark) = (100, 200)// expected-error {{expected 'var' keyword in property declaration}} {{3-3=var }}

a, b: Int // expected-error {{expected 'var' keyword in property declaration}} {{3-3=var }}
}

Expand Down
7 changes: 0 additions & 7 deletions test/Parse/line-directive.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,13 +24,6 @@ x x // expected-error{{consecutive statements}} {{2-2=;}}

// rdar://19582475
public struct S { // expected-note{{in declaration of 'S'}}
// expected-error@+8{{expected 'func' keyword in operator function declaration}}
// expected-error@+7{{operator '/' declared in type 'S' must be 'static'}}
// expected-error@+6{{expected '(' in argument list of function declaration}}
// expected-error@+5{{operators must have one or two arguments}}
// expected-error@+4{{member operator '/()' must have at least one argument of type 'S'}}
// expected-error@+3{{expected '{' in body of function declaration}}
// expected-error@+2{{consecutive declarations on a line must be separated by ';}}
// expected-error@+1{{expected declaration}}
/ ###line 25 "line-directive.swift"
}
Expand Down