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

Improve diagnostic for unexpected ']' while parsing a type. #25898

Merged
merged 3 commits into from Jul 8, 2019

Conversation

Projects
None yet
3 participants
@gregomni
Copy link
Collaborator

commented Jun 30, 2019

Resolves SR-10946.

@gregomni

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 30, 2019

@swift-ci Please smoke test.

@gregomni gregomni requested a review from brentdax Jun 30, 2019

@xedin xedin requested a review from rintaro Jun 30, 2019

@brentdax
Copy link
Collaborator

left a comment

A nice improvement! I have some questions, but the only thing I'm sure I want to see is more test cases.

Show resolved Hide resolved test/Parse/recovery.swift
Show resolved Hide resolved test/Parse/recovery.swift
Show resolved Hide resolved lib/Parse/ParseType.cpp
@gregomni

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 2, 2019

@swift-ci Please smoke test.

consumeToken();
return makeParserErrorResult(new (Context) ErrorTypeRepr(Tok.getLoc()));
}
return result;

This comment has been minimized.

Copy link
@rintaro

rintaro Jul 2, 2019

Member

I don't think this implementation handles type in non decl result position, right? Like param (e.g. func foo(x: Int])) or where clause (e.g. where T == Int]).
Have you tried to do this in parseType() instead of parseDeclResultType()?

This comment has been minimized.

Copy link
@gregomni

gregomni Jul 2, 2019

Author Collaborator

Correct. Putting it in parseType() is a much more invasive change, because a trailing ] would be fine in many cases, the most obvious of which is arrays and dictionaries themselves (parseTypeCollection() calling parseType() to get the element type).

One solution is an additional boolean arg passed to parseType() for whether or not to leave a trailing ] alone or greedily complain. I'm happy to do that, but I'm not sure whether this mistake is common enough to be worth the complexity of passing that context around through the different parseTypeFoo() functions, so I started here.

This comment has been minimized.

Copy link
@gregomni

gregomni Jul 2, 2019

Author Collaborator

Actually, worse than that, parsing types in positions where they are at the end of subscript expressions would be a big problem (e.g. self[key.stringValue as! Key] = value -- from Codable).

This comment has been minimized.

Copy link
@rintaro

rintaro Jul 2, 2019

Member

Agreed. Thanks for explanation :)

@rintaro

This comment has been minimized.

Copy link
Member

commented Jul 2, 2019

Do you have plan for dictionaries?

let a : String: Int]
@gregomni

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 2, 2019

@rintaro Just added a commit with a similar diagnosis for an unexpected ':' while parsing types, in order to handle dictionaries.

@swift-ci Please smoke test.

@gregomni gregomni requested a review from brentdax Jul 2, 2019

@gregomni

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 2, 2019

Weird linux error, trying again.

@swift-ci Please smoke test linux.

@gregomni

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 5, 2019

@brentdax Anything further you'd like to see here?

@gregomni gregomni merged commit 483fc05 into apple:master Jul 8, 2019

2 checks passed

Swift Test Linux Platform (smoke test)
Details
Swift Test OS X Platform (smoke test)
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.