Skip to content

[DiagnosticsQoI] SR-3599: Better recovery for decls with consecutive identifiers #6948

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

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
7 changes: 7 additions & 0 deletions include/swift/Parse/Parser.h
Original file line number Diff line number Diff line change
Expand Up @@ -462,6 +462,9 @@ class Parser {

/// Parse an #endif.
bool parseEndIfDirective(SourceLoc &Loc);

/// True if Tok is the second consecutive identifier in a variable decl.
bool isSecondVarIdentifier();

public:
InFlightDiagnostic diagnose(SourceLoc Loc, Diagnostic Diag) {
Expand Down Expand Up @@ -489,6 +492,10 @@ class Parser {
}

void diagnoseRedefinition(ValueDecl *Prev, ValueDecl *New);

/// Add a fix-it to remove the space in consecutive identifiers.
/// Add a camel-cased option if it is different than the first option.
void diagnoseConsecutiveIDs(Token First, Token Second);

/// \brief Check whether the current token starts with '<'.
bool startsWithLess(Token Tok) {
Expand Down
78 changes: 55 additions & 23 deletions lib/Parse/ParseDecl.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2586,6 +2586,22 @@ parseIdentifierDeclName(Parser &P, Identifier &Result, SourceLoc &L,
ResyncP1, Diagnostic(ID, Args...));
}

/// Add a fix-it to remove the space in consecutive identifiers.
/// Add a camel-cased option if it is different than the first option.
void Parser::diagnoseConsecutiveIDs(Token First, Token Second) {
auto Joined = (First.getText() + Second.getText()).str();
SourceRange Range(First.getLoc(), Second.getLoc());
diagnose(Second.getLoc(), diag::join_identifiers).fixItReplace(Range, Joined);

SmallString<8> Scratch;
auto SentenceCased = camel_case::toSentencecase(Tok.getText(), Scratch);
auto CamelCased = (First.getText() + SentenceCased).str();
if (Joined != CamelCased) {
diagnose(Second.getLoc(), diag::join_identifiers_camel_case)
.fixItReplace(Range, CamelCased);
}
}

/// Parse a Decl item in decl list.
static ParserStatus parseDeclItem(Parser &P,
bool &PreviousHadSemi,
Expand All @@ -2603,9 +2619,20 @@ static ParserStatus parseDeclItem(Parser &P,
// If the previous declaration didn't have a semicolon and this new
// declaration doesn't start a line, complain.
if (!PreviousHadSemi && !P.Tok.isAtStartOfLine() && !P.Tok.is(tok::unknown)) {
auto endOfPrevious = P.getEndOfPreviousLoc();
P.diagnose(endOfPrevious, diag::declaration_same_line_without_semi)
.fixItInsert(endOfPrevious, ";");
// Add a fix-it to remove the space in consecutive identifiers
// in a property or enum case decl.
auto PreviousTok = P.L->getTokenAt(P.PreviousLoc);
if (P.Tok.is(tok::identifier) && PreviousTok.is(tok::identifier)) {
auto enumContext = P.CurDeclContext->getAsEnumOrEnumExtensionContext();
auto TypeName = enumContext ? "enum case" : "property";
P.diagnose(P.Tok.getLoc(), diag::repeated_identifier, TypeName);
P.diagnoseConsecutiveIDs(PreviousTok, P.Tok);
P.skipUntilDeclRBrace(tok::semi, tok::pound_endif);
} else {
auto endOfPrevious = P.getEndOfPreviousLoc();
P.diagnose(endOfPrevious, diag::declaration_same_line_without_semi)
.fixItInsert(endOfPrevious, ";");
}
}

ParserStatus Status = P.parseDecl(Options, handler);
Expand Down Expand Up @@ -4534,26 +4561,7 @@ Parser::parseDeclFunc(SourceLoc StaticLoc, StaticSpellingKind StaticSpelling,
// space or newline accidentally.
if (Tok.isIdentifierOrUnderscore() && SimpleName.str().back() != '<') {
diagnose(Tok.getLoc(), diag::repeated_identifier, "function");

SourceRange DoubleIdentifierRange(NameLoc, Tok.getLoc());

// Provide two fix-its: a direct concatenation of the two identifiers
// and a camel-cased version.

auto DirectConcatenation = NameTok.getText().str() + Tok.getText().str();

diagnose(Tok.getLoc(), diag::join_identifiers)
.fixItReplace(DoubleIdentifierRange, DirectConcatenation);

SmallString<8> CapitalizedScratch;
auto Capitalized = camel_case::toSentencecase(Tok.getText(),
CapitalizedScratch);
auto CamelCaseConcatenation = NameTok.getText().str() + Capitalized.str();

if (DirectConcatenation != CamelCaseConcatenation)
diagnose(Tok.getLoc(), diag::join_identifiers_camel_case)
.fixItReplace(DoubleIdentifierRange, CamelCaseConcatenation);

diagnoseConsecutiveIDs(NameTok, Tok);
consumeToken();
}
}
Expand Down Expand Up @@ -4814,6 +4822,14 @@ ParserResult<EnumDecl> Parser::parseDeclEnum(ParseDeclOptions Flags,
ED->setGenericParams(GenericParams);
}

// Add a fix-it to remove the space in consecutive identifiers
// in an enum decl.
if (Tok.is(tok::identifier)) {
diagnose(Tok.getLoc(), diag::repeated_identifier, "enum");
diagnoseConsecutiveIDs(L->getTokenAt(EnumNameLoc), Tok);
consumeToken();
}

SourceLoc LBLoc, RBLoc;
if (parseToken(tok::l_brace, LBLoc, diag::expected_lbrace_enum)) {
LBLoc = PreviousLoc;
Expand Down Expand Up @@ -5068,6 +5084,14 @@ ParserResult<StructDecl> Parser::parseDeclStruct(ParseDeclOptions Flags,
SD->setGenericParams(GenericParams);
}

// Add a fix-it to remove the space in consecutive identifiers
// in a struct decl.
if (Tok.is(tok::identifier)) {
diagnose(Tok.getLoc(), diag::repeated_identifier, "struct");
diagnoseConsecutiveIDs(L->getTokenAt(StructNameLoc), Tok);
consumeToken();
}

SourceLoc LBLoc, RBLoc;
if (parseToken(tok::l_brace, LBLoc, diag::expected_lbrace_struct)) {
LBLoc = PreviousLoc;
Expand Down Expand Up @@ -5151,6 +5175,14 @@ ParserResult<ClassDecl> Parser::parseDeclClass(SourceLoc ClassLoc,
CD->setGenericParams(GenericParams);
}

// Add a fix-it to remove the space in consecutive identifiers
// in a class decl.
if (Tok.is(tok::identifier)) {
diagnose(Tok.getLoc(), diag::repeated_identifier, "class");
diagnoseConsecutiveIDs(L->getTokenAt(ClassNameLoc), Tok);
consumeToken();
}

SourceLoc LBLoc, RBLoc;
if (parseToken(tok::l_brace, LBLoc, diag::expected_lbrace_class)) {
LBLoc = PreviousLoc;
Expand Down
17 changes: 13 additions & 4 deletions lib/Parse/ParseStmt.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -283,10 +283,19 @@ ParserStatus Parser::parseBraceItems(SmallVectorImpl<ASTNode> &Entries,
// If the previous statement didn't have a semicolon and this new
// statement doesn't start a line, complain.
if (!PreviousHadSemi && !Tok.isAtStartOfLine()) {
SourceLoc EndOfPreviousLoc = getEndOfPreviousLoc();
diagnose(EndOfPreviousLoc, diag::statement_same_line_without_semi)
.fixItInsert(EndOfPreviousLoc, ";");
// FIXME: Add semicolon to the AST?
// Add a fix-it to remove the space in consecutive identifiers
// in a variable decl.
if (isSecondVarIdentifier()) {
diagnose(Tok.getLoc(), diag::repeated_identifier, "variable");
auto Previous = L->getTokenAt(PreviousLoc);
diagnoseConsecutiveIDs(Previous, Tok);
skipUntilDeclRBrace(tok::semi, tok::pound_endif);
} else {
SourceLoc EndOfPreviousLoc = getEndOfPreviousLoc();
diagnose(EndOfPreviousLoc, diag::statement_same_line_without_semi)
.fixItInsert(EndOfPreviousLoc, ";");
// FIXME: Add semicolon to the AST?
}
}

ParserPosition BeginParserPosition;
Expand Down
21 changes: 21 additions & 0 deletions lib/Parse/Parser.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -737,6 +737,27 @@ void Parser::diagnoseRedefinition(ValueDecl *Prev, ValueDecl *New) {
Prev->getName());
}

/// True if Tok is the second consecutive identifier in a variable decl.
bool Parser::isSecondVarIdentifier() {
auto PreviousTok = L->getTokenAt(PreviousLoc);
if (Tok.isNot(tok::identifier) || PreviousTok.isNot(tok::identifier)) {
return false;
}
auto LineStart = L->getLocForStartOfLine(SourceMgr, Tok.getLoc());
auto FirstTok = L->getTokenAt(LineStart);
Lexer::State FirstTokState = L->getStateForBeginningOfToken(FirstTok);
auto StartPos = ParserPosition(FirstTokState, LineStart);
auto RestorePos = getParserPosition();
backtrackToPosition(StartPos);
while (Tok.isNot(tok::kw_let) && Tok.isNot(tok::kw_var)
&& Tok.getLoc() != PreviousTok.getLoc()) {
consumeToken();
}
bool IsConsecutiveLoc = peekToken().getLoc() == PreviousTok.getLoc();
restoreParserPosition(RestorePos);
return IsConsecutiveLoc;
}

struct ParserUnit::Implementation {
LangOptions LangOpts;
SearchPathOptions SearchPathOpts;
Expand Down
52 changes: 52 additions & 0 deletions test/Parse/recovery.swift
Original file line number Diff line number Diff line change
Expand Up @@ -242,6 +242,58 @@ class NoBracesClass2 // expected-error {{expected '{' in class}}
protocol NoBracesProtocol2 // expected-error {{expected '{' in protocol type}}
extension NoBracesStruct2 // expected-error {{expected '{' in extension}}

//===--- Recovery for multiple identifiers in decls

enum EE EE {}
// expected-error @-1 {{found an unexpected second identifier in enum declaration; is there an accidental break?}}
// expected-note @-2 {{join the identifiers together}} {{6-11=EEEE}}

struct SS SS {}
// expected-error @-1 {{found an unexpected second identifier in struct declaration; is there an accidental break?}}
// expected-note @-2 {{join the identifiers together}} {{8-13=SSSS}}

class CC CC {}
// expected-error @-1 {{found an unexpected second identifier in class declaration; is there an accidental break?}}
// expected-note @-2 {{join the identifiers together}} {{7-12=CCCC}}
// expected-note @-3 {{did you mean 'CC'}}

enum EEE {

case a a
// expected-error @-1 {{found an unexpected second identifier in enum case declaration; is there an accidental break?}}
// expected-note @-2 {{join the identifiers together}} {{8-11=aa}}
// expected-note @-3 {{join the identifiers together with camel-case}} {{8-11=aA}}

case b
}

struct AA {

private var a b = 0
// expected-error @-1 {{found an unexpected second identifier in property declaration; is there an accidental break?}}
// expected-note @-2 {{join the identifiers together}} {{15-18=ab}}
// expected-note @-3 {{join the identifiers together with camel-case}} {{15-18=aB}}
// expected-error @-4 {{type annotation missing in pattern}}

func f() {
var c d = 5
// expected-error @-1 {{found an unexpected second identifier in variable declaration; is there an accidental break?}}
// expected-note @-2 {{join the identifiers together}} {{9-12=cd}}
// expected-note @-3 {{join the identifiers together with camel-case}} {{9-12=cD}}
// expected-error @-4 {{type annotation missing in pattern}}

let _ = 0
}
}

let e f = 5
// expected-error @-1 {{found an unexpected second identifier in variable declaration; is there an accidental break?}}
// expected-note @-2 {{join the identifiers together}} {{5-8=ef}}
// expected-note @-3 {{join the identifiers together with camel-case}} {{5-8=eF}}
// expected-error @-4 {{type annotation missing in pattern}}
// expected-note @-5 * {{did you mean 'e'?}}


//===--- Recovery for parse errors in types.

struct ErrorTypeInVarDecl1 {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
// See https://swift.org/LICENSE.txt for license information
// See https://swift.org/CONTRIBUTORS.txt for the list of Swift project authors

// RUN: not --crash %target-swift-frontend %s -emit-ir
// RUN: not %target-swift-frontend %s -emit-ir
// REQUIRES: asserts
protocol a:RangeReplaceableCollection
class a
Expand Down