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

syntax: allow negative integer literal expression to be interpolated as pattern #42886

Merged
merged 1 commit into from
Jun 29, 2017

Conversation

durka
Copy link
Contributor

@durka durka commented Jun 24, 2017

Fixes #42820.

r? @jseyfried

@@ -1661,6 +1661,8 @@ impl<'a> Parser<'a> {

/// matches '-' lit | lit
pub fn parse_pat_literal_maybe_minus(&mut self) -> PResult<'a, P<Expr>> {
maybe_whole_expr!(self);
Copy link
Contributor

Choose a reason for hiding this comment

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

This line permits much more that just negative integer literals, now PatKind::Lit and PatKind::Range can contain arbitrary expressions.
I'm not sure that later compilation stages are prepared to deal with them, but there's a chance that it will "just work" and passing an arbitrary expression through expr matcher will be equivalent to passing it through a constant.

There are two ways to proceed, I think:

  • Conservative way: add a check to ast_validation ensuring that only paths, literals and literals with minus are accepted in PatKind::Lit and PatKind::Range.
  • Non-conservative way: extend the language and permit passing arbitrary expressions to literal/range patterns through expr matchers, make sure it doesn't cause ICEs etc, make @rust-lang/lang aware of this change.

In any case tests for passing arbitrary expressions are required.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed it causes ICEs. Conservative approach sounds good to me for this patch.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should the check be in ast_validation or right here in parse_pat_literal_maybe_minus?

Copy link
Contributor

Choose a reason for hiding this comment

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

These kinds of things are usually checked in ast_validation because AST fragments can be produced by procedural macros as well (and parser checks won't help in this case).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ack.

Copy link
Contributor Author

@durka durka Jun 26, 2017

Choose a reason for hiding this comment

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

@petrochenkov OK, not quite sure what to do. I added the check to ast_validation but I am hitting this ICE in a lint before it even gets there.

Should I just remove the ICE, letting any expression through that lint and have validation catch it later, or add the validation check here (does procedural macro generated code go through lints?) (also then we'd have a lint emitting an error -- is that bad?)?

@Mark-Simulacrum Mark-Simulacrum added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Jun 24, 2017
@durka
Copy link
Contributor Author

durka commented Jun 27, 2017

I added the check to ast_validation and a test that 1+1 doesn't slip through. Just to make sure the user doesn't miss the message, it prints the error twice and an unrelated one from typeck as well (but seriously wtf is going on).

match expr.node {
ExprKind::Lit(_) => {}
ExprKind::Unary(UnOp::Neg, ref inner)
if match inner.node { ExprKind::Lit(_) => true, _ => false } => {}
Copy link
Contributor

Choose a reason for hiding this comment

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

ExprKind::Path is permitted as well, Travis fails due to this.

@durka
Copy link
Contributor Author

durka commented Jun 27, 2017 via email

Pos = 1,
Neg = -1,
Arith = 1 + 1, //~ ERROR arbitrary expressions aren't allowed in patterns
//~^ ERROR arbitrary expressions aren't allowed in patterns
Copy link
Member

Choose a reason for hiding this comment

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

cc @arielb1 -- you wanted to get pinged when I find duplicated error messages.

Copy link
Contributor

@arielb1 arielb1 Jul 30, 2017

Choose a reason for hiding this comment

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

Nah this is caused by there being 2 uses of $value in the macro.

@durka
Copy link
Contributor Author

durka commented Jun 28, 2017 via email

@jseyfried
Copy link
Contributor

Reviewed, r=me unless anyone objects.
Eventually, I think we should consider the negative to be part of the literal (cc @eddyb).

@petrochenkov
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Jun 29, 2017

📌 Commit 0dfd9c3 has been approved by petrochenkov

@arielb1
Copy link
Contributor

arielb1 commented Jun 29, 2017

@bors rollup

arielb1 pushed a commit to arielb1/rust that referenced this pull request Jun 29, 2017
syntax: allow negative integer literal expression to be interpolated as pattern

Fixes rust-lang#42820.

r? @jseyfried
bors added a commit that referenced this pull request Jun 29, 2017
Rollup of 12 pull requests

- Successful merges: #42219, #42831, #42832, #42884, #42886, #42901, #42919, #42920, #42946, #42953, #42955, #42958
- Failed merges:
@bors bors merged commit 0dfd9c3 into rust-lang:master Jun 29, 2017
arielb1 added a commit to arielb1/rust that referenced this pull request Aug 13, 2017
Since rust-lang#42886, macros can create "nonstandard" PatKind::Lit patterns,
that contain path expressions instead of the usual literal expr. These
can cause trouble, including ICEs.

We *could* map these nonstandard patterns to PatKind::Path patterns
during HIR lowering, but that would be much effort for little gain, and
I think is too risky for beta. So let's just forbid them during AST
validation.

Fixes rust-lang#43250.
bors added a commit that referenced this pull request Aug 14, 2017
ast_validation: forbid "nonstandard" literal patterns

Since #42886, macros can create "nonstandard" PatKind::Lit patterns,
that contain path expressions instead of the usual literal expr. These
can cause trouble, including ICEs.

We *could* map these nonstandard patterns to PatKind::Path patterns
during HIR lowering, but that would be much effort for little gain, and
I think is too risky for beta. So let's just forbid them during AST
validation.

Fixes #43250.

beta-nominating because regression.
r? @eddyb
alexcrichton pushed a commit to alexcrichton/rust that referenced this pull request Aug 23, 2017
Since rust-lang#42886, macros can create "nonstandard" PatKind::Lit patterns,
that contain path expressions instead of the usual literal expr. These
can cause trouble, including ICEs.

We *could* map these nonstandard patterns to PatKind::Path patterns
during HIR lowering, but that would be much effort for little gain, and
I think is too risky for beta. So let's just forbid them during AST
validation.

Fixes rust-lang#43250.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants