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

Reject postfix ops already in Parser #14677

Merged
merged 3 commits into from Mar 14, 2022
Merged

Conversation

odersky
Copy link
Contributor

@odersky odersky commented Mar 12, 2022

Parse a postfix op only if language.postfixOps is imported. This helps avoiding
non-sensical parses and confusing error messages. Previously we always parsed,
but issued errors in Typer if postfix ops were not enabled.

Also: drop language.postfixOps as a default option in build and test

Fixes #14564

Parse a postfix op only if language.postfixOps is imported. This helps avoiding
non-sensical parses. Previously we always parsed, but issued errors in Typer if
postfix ops were not enabled.
Copy link
Contributor

@pikinier20 pikinier20 left a comment

Choose a reason for hiding this comment

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

This is the change nearly nobody asked for but everybody needed. Personally, I encountered the non-sensical message about postfix ops many times.

I've got only one thought/comment about these changes. Besides that, LGTM.

infixOps(t, in.canStartExprTokens, prefixExpr, location,
isType = false,
isOperator = !(location.inArgs && followingIsVararg()),
maybePostfix = true)
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder whether just changing there the value of maybePostfix to in.postfixOpsEnabled wouldn't do the same as all these parser changes.

I mean, this is the only place where we set ParseKind.Expr so instead of checking if the postfix is enabled in infixOps, we can do it here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, since even with postfixOpsEnabled we do not allow postfix ops in patterns and types.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, but for these cases the maybePostfix parameter was already by default false.

@odersky odersky merged commit b505b08 into scala:main Mar 14, 2022
@odersky odersky deleted the fix-14564 branch March 14, 2022 11:11
@odersky
Copy link
Contributor Author

odersky commented Mar 14, 2022

Yes, true, but I chose to do a three-way enumeration instead of two booleans for clarity. It's not a big deal either way, I agree.

@pikinier20
Copy link
Contributor

Okey, I see. The clarity is indeed better with enums.

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.

Sequence arg in interpolation is rejected
4 participants