-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Reject postfix ops already in Parser #14677
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
Conversation
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.
There was a problem hiding this 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) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
.
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. |
Okey, I see. The clarity is indeed better with enums. |
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