Skip to content

[libSyntax] Fix a crash that happened when parsing an invalid type #18022

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
merged 1 commit into from
Jul 19, 2018

Conversation

ahoppen
Copy link
Member

@ahoppen ahoppen commented Jul 17, 2018

When parsing an invalid type like [Stri?ng], the syntax parser would crash because it was trying to build an OptionalType but the second entry on the stack was no TypeSyntax but a TokenSyntax.

Fix this by checking if the second next node on the parsing stack is a TypeSyntax and if not, just push the nodes back onto the stack so that they can later be gathered into an unknown node.

@ahoppen ahoppen requested review from rintaro and nkcsgexi July 17, 2018 23:17
@ahoppen
Copy link
Member Author

ahoppen commented Jul 17, 2018

@swift-ci Please smoke test

Copy link
Member

@rintaro rintaro left a comment

Choose a reason for hiding this comment

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

Thank you! LGTM modulo comments.

@@ -1181,10 +1181,17 @@ Parser::parseTypeOptional(TypeRepr *base) {
llvm::Optional<TypeSyntax> SyntaxNode;
if (SyntaxContext->isEnabled()) {
OptionalTypeSyntaxBuilder Builder(Context.getSyntaxArena());
Copy link
Member

Choose a reason for hiding this comment

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

Could you move this into if branch? The order shouldn't matter here.

SyntaxNode.emplace(Builder.build());
auto QuestionMark = SyntaxContext->popToken();
auto WrappedType = SyntaxContext->popIf<TypeSyntax>();
if (WrappedType.hasValue()) {
Copy link
Member

Choose a reason for hiding this comment

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

if (auto WrappedType = SyntaxContext->popIf<TypeSyntax>()) {

should work.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh that's great. I always thought that the if (auto …) syntax only worked for pointers.

@ahoppen ahoppen force-pushed the invalid-type-fix branch from cf075d6 to 53dfecd Compare July 18, 2018 20:10
@ahoppen
Copy link
Member Author

ahoppen commented Jul 18, 2018

@swift-ci Please smoke test

Copy link
Contributor

@nkcsgexi nkcsgexi left a comment

Choose a reason for hiding this comment

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

LGTM!

@ahoppen ahoppen merged commit 6568feb into swiftlang:master Jul 19, 2018
@ahoppen ahoppen deleted the invalid-type-fix branch July 19, 2018 16:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants