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

The vec![..] macro can be used in pattern position with bad diagnostics #61933

Closed
czipperz opened this issue Jun 18, 2019 · 7 comments · Fixed by #63399 or #81080
Closed

The vec![..] macro can be used in pattern position with bad diagnostics #61933

czipperz opened this issue Jun 18, 2019 · 7 comments · Fixed by #63399 or #81080
Assignees
Labels
A-diagnostics Area: Messages for errors, warnings, and lints A-macros Area: All kinds of macros (custom derive, macro_rules!, proc macros, ..) A-slice-patterns Area: Slice patterns, https://github.com/rust-lang/rust/issues/23121 D-newcomer-roadblock Diagnostics: Confusing error or lint; hard to understand for new users. D-papercut Diagnostics: An error or lint that needs small tweaks. E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@czipperz
Copy link
Contributor

czipperz commented Jun 18, 2019

fn main() {
    match Some(vec![3]) {
        Some(vec![x]) => (),
        _ => (),
    }
}

Gives error message:

error: unexpected `(` after qualified path
 --> src/main.rs:3:14
  |
3 |         Some(vec![x]) => (),
  |              ^^^^^^^
  |              |
  |              unexpected `(` after qualified path
  |              in this macro invocation
  |
  = note: this warning originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info)

But this is clearly wrong as there is no ( in the span pointed to.

@jonas-schievink jonas-schievink added A-diagnostics Area: Messages for errors, warnings, and lints C-bug Category: This is a bug. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jun 18, 2019
@estebank
Copy link
Contributor

This is caused by macro expansion:

$ rustc +nightly file3.rs -Zexternal-macro-backtrace
error: unexpected `(` after qualified path
 --> <::alloc::macros::vec macros>:3:23
  |
1 | / ( $ elem : expr ; $ n : expr ) => (
2 | | $ crate :: vec :: from_elem ( $ elem , $ n ) ) ; ( $ ( $ x : expr ) , * ) => (
3 | | < [ _ ] > :: into_vec ( box [ $ ( $ x ) , * ] ) ) ; ( $ ( $ x : expr , ) * )
  | |                       ^ unexpected `(` after qualified path
4 | | => ( $ crate :: vec ! [ $ ( $ x ) , * ] )
  | |_________________________________________- in this expansion of `vec!`
  |
 ::: file3.rs:3:14
  |
3 |           Some(vec![x]) => (),
  |                -------
  |                |
  |                in this macro invocation
  |                in this macro invocation

error: aborting due to previous error

You cannot pattern match on vectors, but you can pattern match on slices (and even rely on match ergonomics to make it look like you're matching on an array).

@estebank estebank added A-macros Area: All kinds of macros (custom derive, macro_rules!, proc macros, ..) A-slice-patterns Area: Slice patterns, https://github.com/rust-lang/rust/issues/23121 and removed C-bug Category: This is a bug. labels Jun 18, 2019
@czipperz
Copy link
Contributor Author

I think it would be nice if this was special cased since it's easily fixable.

Centril added a commit to Centril/rust that referenced this issue Aug 9, 2019
More explicit diagnostic when using a `vec![]` in a pattern

```
error: unexpected `(` after qualified path
  --> $DIR/vec-macro-in-pattern.rs:3:14
   |
LL |         Some(vec![x]) => (),
   |              ^^^^^^^
   |              |
   |              unexpected `(` after qualified path
   |              in this macro invocation
   |              use a slice pattern here instead
   |
   = help: for more information, see https://doc.rust-lang.org/edition-guide/rust-2018/slice-patterns.html
   = note: this warning originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info)
```

Fix rust-lang#61933.
Centril added a commit to Centril/rust that referenced this issue Aug 10, 2019
More explicit diagnostic when using a `vec![]` in a pattern

```
error: unexpected `(` after qualified path
  --> $DIR/vec-macro-in-pattern.rs:3:14
   |
LL |         Some(vec![x]) => (),
   |              ^^^^^^^
   |              |
   |              unexpected `(` after qualified path
   |              in this macro invocation
   |              use a slice pattern here instead
   |
   = help: for more information, see https://doc.rust-lang.org/edition-guide/rust-2018/slice-patterns.html
   = note: this warning originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info)
```

Fix rust-lang#61933.
Centril added a commit to Centril/rust that referenced this issue Aug 10, 2019
More explicit diagnostic when using a `vec![]` in a pattern

```
error: unexpected `(` after qualified path
  --> $DIR/vec-macro-in-pattern.rs:3:14
   |
LL |         Some(vec![x]) => (),
   |              ^^^^^^^
   |              |
   |              unexpected `(` after qualified path
   |              in this macro invocation
   |              use a slice pattern here instead
   |
   = help: for more information, see https://doc.rust-lang.org/edition-guide/rust-2018/slice-patterns.html
   = note: this warning originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info)
```

Fix rust-lang#61933.
@oli-obk oli-obk changed the title Match diagnostic unexpected ( The vec![..] macro can be used in pattern position with bad diagnostics Dec 18, 2020
@oli-obk oli-obk reopened this Dec 18, 2020
@oli-obk
Copy link
Contributor

oli-obk commented Dec 18, 2020

I reopened this issue (with a title update), because #80080 will reintroduce the issue (with an even odder diagnostic). While trying to come up with a way to fix this issue again with the new system, I came up with the following situation:

Add a way to mark macro_rules declarations with an attribute that declares what their returned AST node type is. Then we mark macro_rules! vec as only producing expressions and add a check during expansion that handles this and reports errors if used wrongly. This will always produce the right error, and we can then also re-add a suggestion if vec! specifically has been used in pattern position.

@oli-obk
Copy link
Contributor

oli-obk commented Dec 18, 2020

I did some more experimentation, and we could also get away with just changing the vec! macro to enforce it only being used in expression position:

https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=43c7b63851e0b5858a60f191543a49ad

macro_rules! vec {
    () => (
        vec!(@force_expr_internal_do_not_use@: $crate::vec::Vec::new())
    );
    ($elem:expr; $n:expr) => (
        vec!(@force_expr_internal_do_not_use@: $crate::vec::from_elem($elem, $n))
    );
    ($($x:expr),+ $(,)?) => (
        vec!(@force_expr_internal_do_not_use@: <[_]>::into_vec(box [$($x),+]))
    );
    (@force_expr_internal_do_not_use@: $e:expr) => ($e);
}

This gives us

error: arbitrary expressions aren't allowed in patterns
  --> src/main.rs:11:48
   |
11 |         vec!(@force_expr_internal_do_not_use@: <[_]>::into_vec(box [$($x),+]))
   |                                                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
...
18 |         vec![43] => {},
   |         -------- in this macro invocation
   |
   = note: this error originates in a macro (in Nightly builds, run with -Z macro-backtrace for more info)

though line 15 is probably not going to get shown since the macro is from liballoc

@estebank
Copy link
Contributor

I think that's a reasonable solution. Do you want to cut a PR for it and deleting the now unused code, @oli-obk?

@estebank estebank added D-newcomer-roadblock Diagnostics: Confusing error or lint; hard to understand for new users. D-papercut Diagnostics: An error or lint that needs small tweaks. labels Dec 19, 2020
@oli-obk oli-obk added E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. labels Dec 19, 2020
@oli-obk
Copy link
Contributor

oli-obk commented Dec 19, 2020

I'll write up instructions and hope that someone will pick it up as a good first issue over the holidays. If not, I'll do it before #80080 gets merged.

  1. write @rustbot claim in this issue to assign it to yourself
  2. cherry pick 5e0401f to remove the old code
  3. change the vec! macro in https://github.com/rust-lang/rust/blob/master/library/alloc/src/macros.rs#L41-L51 to the one shown above
  4. leave some documentation on the force_expr_internal_do_not_use macro arm explaining that it is to force the ast node type to an expression, thus improving the diagnostics if used in type or pattern position
  5. Run ./x.py test src/tests/ui --bless which auto-updates all .stderr files and update all //~ ERROR and other comments that need updating (likely you'll be able to remove some of them entirely because we now only get a single error
  6. open a PR (also do that if you get stuck or anything doesn't work) and write r? @oli-obk in the main message so it gets assigned to me

@bugadani
Copy link
Contributor

@rustbot claim

nnethercote added a commit to nnethercote/rust that referenced this issue Jun 25, 2024
This was added (with a different name) to improve an error message. It
is no longer needed -- removing it changes the error message, but overall
I think the new message is no worse:
- the mention of `#` in the first line is a little worse,
- but the extra context makes it very clear what the problem is, perhaps
  even clearer than the old message,
- and the removal of the note about the `expr` fragment (an internal
  detail of `__rust_force_expr`) is an improvement.

Overall I think the error is quite clear and still far better than the
old message that prompted rust-lang#61933, which didn't even mention patterns.

The motivation for this is rust-lang#124141, which will cause pasted
metavariables to be tokenized and reparsed instead of the AST node being
cached. This change in behaviour occasionally has a non-zero perf cost,
and `__rust_force_expr` causes the tokenize/reparse step to occur twice.
Removing `__rust_force_expr` greatly reduces the extra overhead for the
`deep-vector` benchmark.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Jun 26, 2024
…r=oli-obk

Remove `__rust_force_expr`.

This was added (with a different name) to improve an error message. It is no longer needed -- removing it changes the error message, but overall I think the new message is no worse:
- the mention of `#` in the first line is a little worse,
- but the extra context makes it very clear what the problem is, perhaps even clearer than the old message,
- and the removal of the note about the `expr` fragment (an internal detail of `__rust_force_expr`) is an improvement.

Overall I think the error is quite clear and still far better than the old message that prompted rust-lang#61933, which didn't even mention patterns.

The motivation for this is rust-lang#124141, which will cause pasted metavariables to be tokenized and reparsed instead of the AST node being cached. This change in behaviour occasionally has a non-zero perf cost, and `__rust_force_expr` causes the tokenize/reparse step to occur twice. Removing `__rust_force_expr` greatly reduces the extra overhead for the `deep-vector` benchmark.

r? `@oli-obk`
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Jun 26, 2024
…r=oli-obk

Remove `__rust_force_expr`.

This was added (with a different name) to improve an error message. It is no longer needed -- removing it changes the error message, but overall I think the new message is no worse:
- the mention of `#` in the first line is a little worse,
- but the extra context makes it very clear what the problem is, perhaps even clearer than the old message,
- and the removal of the note about the `expr` fragment (an internal detail of `__rust_force_expr`) is an improvement.

Overall I think the error is quite clear and still far better than the old message that prompted rust-lang#61933, which didn't even mention patterns.

The motivation for this is rust-lang#124141, which will cause pasted metavariables to be tokenized and reparsed instead of the AST node being cached. This change in behaviour occasionally has a non-zero perf cost, and `__rust_force_expr` causes the tokenize/reparse step to occur twice. Removing `__rust_force_expr` greatly reduces the extra overhead for the `deep-vector` benchmark.

r? ``@oli-obk``
jhpratt added a commit to jhpratt/rust that referenced this issue Jun 27, 2024
…r=oli-obk

Remove `__rust_force_expr`.

This was added (with a different name) to improve an error message. It is no longer needed -- removing it changes the error message, but overall I think the new message is no worse:
- the mention of `#` in the first line is a little worse,
- but the extra context makes it very clear what the problem is, perhaps even clearer than the old message,
- and the removal of the note about the `expr` fragment (an internal detail of `__rust_force_expr`) is an improvement.

Overall I think the error is quite clear and still far better than the old message that prompted rust-lang#61933, which didn't even mention patterns.

The motivation for this is rust-lang#124141, which will cause pasted metavariables to be tokenized and reparsed instead of the AST node being cached. This change in behaviour occasionally has a non-zero perf cost, and `__rust_force_expr` causes the tokenize/reparse step to occur twice. Removing `__rust_force_expr` greatly reduces the extra overhead for the `deep-vector` benchmark.

r? ```@oli-obk```
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Jun 27, 2024
Rollup merge of rust-lang#126929 - nnethercote:rm-__rust_force_expr, r=oli-obk

Remove `__rust_force_expr`.

This was added (with a different name) to improve an error message. It is no longer needed -- removing it changes the error message, but overall I think the new message is no worse:
- the mention of `#` in the first line is a little worse,
- but the extra context makes it very clear what the problem is, perhaps even clearer than the old message,
- and the removal of the note about the `expr` fragment (an internal detail of `__rust_force_expr`) is an improvement.

Overall I think the error is quite clear and still far better than the old message that prompted rust-lang#61933, which didn't even mention patterns.

The motivation for this is rust-lang#124141, which will cause pasted metavariables to be tokenized and reparsed instead of the AST node being cached. This change in behaviour occasionally has a non-zero perf cost, and `__rust_force_expr` causes the tokenize/reparse step to occur twice. Removing `__rust_force_expr` greatly reduces the extra overhead for the `deep-vector` benchmark.

r? ```@oli-obk```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-diagnostics Area: Messages for errors, warnings, and lints A-macros Area: All kinds of macros (custom derive, macro_rules!, proc macros, ..) A-slice-patterns Area: Slice patterns, https://github.com/rust-lang/rust/issues/23121 D-newcomer-roadblock Diagnostics: Confusing error or lint; hard to understand for new users. D-papercut Diagnostics: An error or lint that needs small tweaks. E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
5 participants