Skip to content

[Parse] Minor code improvement in function-type parsing #5721

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

Merged
merged 3 commits into from
Jan 13, 2017
Merged

[Parse] Minor code improvement in function-type parsing #5721

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ae495c2
[Parse] Simplify FunctionType parsing
rintaro Nov 8, 2016
e4c18ba
[Parse] Add fix-it for 'rethrows' in function type
rintaro Nov 11, 2016
2a2da1f
[Parse] Add FIXME for bad recovery for incomplete function type
rintaro Nov 14, 2016
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
5 changes: 3 additions & 2 deletions include/swift/AST/DiagnosticsParse.def
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -619,8 +619,9 @@ ERROR(expected_type_function_result,PointsToFirstBadToken,
"expected type for function result", ())
ERROR(generic_non_function,PointsToFirstBadToken,
"only syntactic function types can be generic", ())
ERROR(rethrowing_function_type,PointsToFirstBadToken,
"only function declarations may be marked 'rethrows'", ())
ERROR(rethrowing_function_type,none,
"only function declarations may be marked 'rethrows'; "
"did you mean 'throws'?", ())
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't make sense to me. The fact that rethrows isn't allowed in a function type (which I didn't realize until seeing this PR) seems like an unnecessary and surprising limitation. I would guess users will expect to be able to write rethrows function types, so suggesting did you mean 'throws'? will just be confusing.

Copy link
Member Author

@rintaro rintaro Nov 22, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Hmm. Maybe, rejecting rethrows is just a bug.

From the language reference

Function types that can throw an error must be marked with the throws keyword, and function types that can rethrow an error must be marked with the rethrows keyword.

GRAMMAR OF A FUNCTION TYPE

function-type → attributes­opt ­function-type-argument-clause ­throws ­opt­ ->­ type­
function-type → attributes­opt ­function-type-argument-clause ­rethrows ­-> ­type­

Copy link
Contributor

Choose a reason for hiding this comment

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

Summoning @rjmccall. A rethrows function type is useful, but it also makes the language more complicated, and I thought we decided not to do it.

ERROR(throws_in_wrong_position,none,
"'throws' may only occur before '->'", ())
ERROR(rethrows_in_wrong_position,none,
Expand Down
3 changes: 2 additions & 1 deletion lib/Parse/ParseExpr.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2217,7 +2217,8 @@ parseClosureSignatureIfPresent(SmallVectorImpl<CaptureListEntry> &captureList,
throwsLoc = consumeToken();
} else if (Tok.is(tok::kw_rethrows)) {
throwsLoc = consumeToken();
diagnose(throwsLoc, diag::rethrowing_function_type);
diagnose(throwsLoc, diag::rethrowing_function_type)
.fixItReplace(throwsLoc, "throws");
}

// Parse the optional explicit return type.
Expand Down
58 changes: 21 additions & 37 deletions lib/Parse/ParseType.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -184,6 +184,7 @@ ParserResult<TypeRepr> Parser::parseType() {
///
/// type-function:
/// type-composition '->' type
/// type-composition 'throws' '->' type
///
ParserResult<TypeRepr> Parser::parseType(Diag<> MessageID,
bool HandleCodeCompletion) {
Expand All @@ -203,62 +204,45 @@ ParserResult<TypeRepr> Parser::parseType(Diag<> MessageID,
parseTypeSimpleOrComposition(MessageID, HandleCodeCompletion);
if (ty.hasCodeCompletion())
return makeParserCodeCompletionResult<TypeRepr>();

if (ty.isNull())
return nullptr;
auto tyR = ty.get();

// Parse a throws specifier. 'throw' is probably a typo for 'throws',
// but in local contexts we could just be at the end of a statement,
// so we need to check for the arrow.
ParserPosition beforeThrowsPos;
// Parse a throws specifier.
// Don't consume 'throws', if the next token is not '->', so we can emit a
// more useful diagnostic when parsing a function decl.
SourceLoc throwsLoc;
bool rethrows = false;
if (Tok.isAny(tok::kw_throws, tok::kw_rethrows) ||
(Tok.is(tok::kw_throw) && peekToken().is(tok::arrow))) {
if (Tok.is(tok::kw_throw)) {
diagnose(Tok.getLoc(), diag::throw_in_function_type)
if (Tok.isAny(tok::kw_throws, tok::kw_rethrows, tok::kw_throw) &&
peekToken().is(tok::arrow)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is definitely a behavior change for the recovery in a partial line:

let fn: () throws

Can you add tests for this case, and see if it's a net benefit?

Copy link
Member Author

@rintaro rintaro Nov 11, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

I'm not sure. What you mention is Don't consume 'throws', if next token is not '->'?
Swift 3.0.1 and this PR both emits:

test.swift:1:11: error: consecutive statements on a line must be separated by ';'
let fn: () throws
          ^
          ;
test.swift:1:12: error: expected expression
let fn: () throws
           ^

Actually, this behavior is from the original code:
https://github.com/apple/swift/blob/71364d0d/lib/Parse/ParseType.cpp#L222-L224

if (Tok.isNot(tok::kw_throws)) {
// 'rethrows' is only allowed on function declarations for now.
// 'throw' is probably a typo for 'throws'.
Diag<> DiagID = Tok.is(tok::kw_rethrows) ?
diag::rethrowing_function_type : diag::throw_in_function_type;
diagnose(Tok.getLoc(), DiagID)
.fixItReplace(Tok.getLoc(), "throws");
}

beforeThrowsPos = getParserPosition();
rethrows = Tok.is(tok::kw_rethrows);
throwsLoc = consumeToken();
}

// Handle type-function if we have an arrow.
SourceLoc arrowLoc;
if (consumeIf(tok::arrow, arrowLoc)) {
if (Tok.is(tok::arrow)) {
// Handle type-function if we have an arrow.
SourceLoc arrowLoc = consumeToken();
ParserResult<TypeRepr> SecondHalf =
parseType(diag::expected_type_function_result);
if (SecondHalf.hasCodeCompletion())
return makeParserCodeCompletionResult<TypeRepr>();
if (SecondHalf.isNull())
return nullptr;
if (rethrows) {
// 'rethrows' is only allowed on function declarations for now.
diagnose(throwsLoc, diag::rethrowing_function_type);
}
auto fnTy = new (Context) FunctionTypeRepr(generics, ty.get(),
throwsLoc,
arrowLoc,
SecondHalf.get());
return makeParserResult(applyAttributeToType(fnTy, inoutLoc, attrs));
} else if (throwsLoc.isValid()) {
// Don't consume 'throws', so we can emit a more useful diagnostic when
// parsing a function decl.
restoreParserPosition(beforeThrowsPos);
}

// Only function types may be generic.
if (generics) {
tyR = new (Context) FunctionTypeRepr(generics, tyR, throwsLoc, arrowLoc,
SecondHalf.get());
} else if (generics) {
// Only function types may be generic.
auto brackets = generics->getSourceRange();
diagnose(brackets.Start, diag::generic_non_function);
}

if (ty.isNonNull() && !ty.hasCodeCompletion()) {
ty = makeParserResult(applyAttributeToType(ty.get(), inoutLoc, attrs));
}
return ty;
return makeParserResult(applyAttributeToType(tyR, inoutLoc, attrs));
}

ParserResult<TypeRepr> Parser::parseTypeForInheritance(
Expand Down
7 changes: 7 additions & 0 deletions test/Parse/errors.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,13 @@ func postRethrows2(_ f: () throws -> Int) -> rethrows Int { // expected-error{{'
return try f()
}

func incompleteThrowType() {
// FIXME: Bad recovery for incomplete function type.
let _: () throws
// expected-error @-1 {{consecutive statements on a line must be separated by ';'}}
// expected-error @-2 {{expected expression}}
}

// rdar://21328447
func fixitThrow0() throw {} // expected-error{{expected throwing specifier; did you mean 'throws'?}} {{20-25=throws}}
func fixitThrow1() throw -> Int {} // expected-error{{expected throwing specifier; did you mean 'throws'?}} {{20-25=throws}}
Expand Down
6 changes: 3 additions & 3 deletions test/decl/func/rethrows.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,9 @@
/** Basics *******************************************************************/

// Function types can't be rethrows right now.
let r1 = {() rethrows -> Int in 0} // expected-error {{only function declarations may be marked 'rethrows'}}
let r2 : () rethrows -> Int = { 0 } // expected-error {{only function declarations may be marked 'rethrows'}}
let r3 : Optional<() rethrows -> ()> = nil // expected-error {{only function declarations may be marked 'rethrows'}}
let r1 = {() rethrows -> Int in 0} // expected-error {{only function declarations may be marked 'rethrows'; did you mean 'throws'?}} {{14-22=throws}}
let r2 : () rethrows -> Int = { 0 } // expected-error {{only function declarations may be marked 'rethrows'; did you mean 'throws'?}} {{13-21=throws}}
let r3 : Optional<() rethrows -> ()> = nil // expected-error {{only function declarations may be marked 'rethrows'; did you mean 'throws'?}} {{22-30=throws}}

func f1(_ f: () throws -> ()) rethrows { try f() }
func f2(_ f: () -> ()) rethrows { f() } // expected-error {{'rethrows' function must take a throwing function argument}}
Expand Down