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

[Diagnostics] Improve diagnostic when using == instead of = for defau… #26139

Merged
merged 3 commits into from Jul 18, 2019

Conversation

vguerra
Copy link
Contributor

@vguerra vguerra commented Jul 14, 2019

…lt function argument.

Resolves SR-11006.

@theblixguy
Copy link
Collaborator

cc @rintaro

if (Tok.isBinaryOperator() && Tok.getText() == "==") {
diagnose(Tok,
diag::expected_assignment_instead_of_comparison_operator)
.fixItReplace(Tok.getLoc(), "=");
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible for the compiler to check this earlier and recover by parsing the expression normally?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The check could indeed be taken before l.272 for instance, but just to be sure I understand: in this case "recover" means that the token for '==' should be consumed so that the code can continue parsing the default value and so on?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I was thinking you'd essentially have the compiler just pretend that the user wrote =.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it. Will try this.

Copy link
Member

@rintaro rintaro Jul 15, 2019

Choose a reason for hiding this comment

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

I think this check and recovery should be performed later, like l.385.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

but somehow I need to avoid the parsing logic to get into any of this branches, so a check would be needed before.

Copy link
Member

Choose a reason for hiding this comment

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

It should work as expected if you consume == as if it's = and parse the default value after ==.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

pushed d82dd84 so check is now performed later as @rintaro suggested and compiler is recovering :).

As well, parser recovers from usage of '==' and parses the whole
parameter clause.
SyntaxParsingContext DefaultArgContext(P.SyntaxContext,
SyntaxKind::InitializerClause);
SourceLoc equalLoc = P.consumeToken(tok::equal);
SourceLoc equalLoc = P.consumeToken(assignmentTok);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wonder if consumption of either '=' or '==' should be taken out of here and be performed on caller site?

Copy link
Contributor

Choose a reason for hiding this comment

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

consumeToken also allows not passing a token kind at all, but I still think you might be right. It's weird to consume something when you don't know what it is.

Copy link
Member

@rintaro rintaro Jul 16, 2019

Choose a reason for hiding this comment

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

I'm OK with:

assert(Tok.is(tok::equal) ||
       (Tok.isBinaryOperator() && Tok.getText() == "=="));
consumeToken();

inside parseDefaultArgument(). Like

swift/lib/Parse/ParseDecl.cpp

Lines 6632 to 6634 in 26c4ccc

Parser::parseDeclInit(ParseDeclOptions Flags, DeclAttributes &Attributes) {
assert(Tok.is(tok::kw_init));
SourceLoc ConstructorLoc = consumeToken();

Copy link
Contributor Author

@vguerra vguerra Jul 16, 2019

Choose a reason for hiding this comment

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

thanks for the feedback .. addressed in 2cfe8ad

@rintaro
Copy link
Member

rintaro commented Jul 16, 2019

@swift-ci Please smoke test

@rintaro
Copy link
Member

rintaro commented Jul 17, 2019

@swift-ci Please smoke test Linux platform

@rintaro rintaro merged commit 6d73ed3 into apple:master Jul 18, 2019
@rintaro
Copy link
Member

rintaro commented Jul 18, 2019

Thank you Victor!

@vguerra
Copy link
Contributor Author

vguerra commented Jul 18, 2019

thank you both for the guidance.

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.

None yet

4 participants