syntax: allow negative integer literal expression to be interpolated as pattern #42886
Conversation
src/libsyntax/parse/parser.rs
Outdated
| @@ -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); | |||
There was a problem hiding this comment.
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_validationensuring that only paths, literals and literals with minus are accepted inPatKind::LitandPatKind::Range. - Non-conservative way: extend the language and permit passing arbitrary expressions to literal/range patterns through
exprmatchers, 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.
There was a problem hiding this comment.
Indeed it causes ICEs. Conservative approach sounds good to me for this patch.
There was a problem hiding this comment.
Should the check be in ast_validation or right here in parse_pat_literal_maybe_minus?
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
@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
added
the
S-waiting-on-author
|
I added the check to |
| match expr.node { | ||
| ExprKind::Lit(_) => {} | ||
| ExprKind::Unary(UnOp::Neg, ref inner) | ||
| if match inner.node { ExprKind::Lit(_) => true, _ => false } => {} |
There was a problem hiding this comment.
ExprKind::Path is permitted as well, Travis fails due to this.
|
Some dependency edges must be missing since I was able to rebuild locally.
…On Tue, Jun 27, 2017 at 2:26 PM, Vadim Petrochenkov < ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In src/librustc_passes/ast_validation.rs
<#42886 (comment)>:
> @@ -93,6 +93,17 @@ impl<'a> AstValidator<'a> {
}
}
}
+
+ /// matches '-' lit | lit (cf. parser::Parser::parse_pat_literal_maybe_minus)
+ fn check_expr_within_pat(&self, expr: &Expr) {
+ match expr.node {
+ ExprKind::Lit(_) => {}
+ ExprKind::Unary(UnOp::Neg, ref inner)
+ if match inner.node { ExprKind::Lit(_) => true, _ => false } => {}
ExprKind::Path is permitted as well, Travis fails due to this.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#42886 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAC3n8N2yN4vyFPhl-5ku8OOUBkGTrLZks5sIUlHgaJpZM4OEZep>
.
|
| Pos = 1, | ||
| Neg = -1, | ||
| Arith = 1 + 1, //~ ERROR arbitrary expressions aren't allowed in patterns | ||
| //~^ ERROR arbitrary expressions aren't allowed in patterns |
There was a problem hiding this comment.
cc @arielb1 -- you wanted to get pinged when I find duplicated error messages.
There was a problem hiding this comment.
Nah this is caused by there being 2 uses of $value in the macro.
|
Sorry, I just realized it isn't really a duplicated message. It's generated
twice because the macro generates two patterns containing $value.
…On Wed, Jun 28, 2017 at 2:13 PM, est31 ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In src/test/compile-fail/patkind-litrange-no-expr.rs
<#42886 (comment)>:
> +
+ fn foo(value: i32) -> Option<$name> {
+ match value {
+ $( $value => Some($name::$variant), )* // PatKind::Lit
+ $( $value ... 42 => Some($name::$variant), )* // PatKind::Range
+ _ => None
+ }
+ }
+ }
+}
+
+enum_number!(Change {
+ Pos = 1,
+ Neg = -1,
+ Arith = 1 + 1, //~ ERROR arbitrary expressions aren't allowed in patterns
+ //~^ ERROR arbitrary expressions aren't allowed in patterns
cc @arielb1 <https://github.com/arielb1> -- you wanted to get pinged when
I find duplicated error messages.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#42886 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAC3n_UEGWytfDB6dj4CGADAhhQe0jtqks5sIpe1gaJpZM4OEZep>
.
|
|
Reviewed, r=me unless anyone objects. |
|
@bors r+ |
|
📌 Commit 0dfd9c3 has been approved by |
|
@bors rollup |
syntax: allow negative integer literal expression to be interpolated as pattern Fixes rust-lang#42820. r? @jseyfried
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.
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
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.
Fixes #42820.
r? @jseyfried