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

proc-macro param attrs dropping first attrs in impl fns #64682

Closed
bbqsrc opened this issue Sep 22, 2019 · 6 comments · Fixed by #64894
Closed

proc-macro param attrs dropping first attrs in impl fns #64682

bbqsrc opened this issue Sep 22, 2019 · 6 comments · Fixed by #64894
Assignees
Labels
A-attributes Area: Attributes (`#[…]`, `#![…]`) A-macros Area: All kinds of macros (custom derive, macro_rules!, proc macros, ..) C-bug Category: This is a bug. P-high High priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@bbqsrc
Copy link

bbqsrc commented Sep 22, 2019

I have been able to reproduce #64282.

If an fn inside an impl has a param attr on the first parameter, no matter how many, they are not visible in the TokenStream.

Repro repo: https://github.com/bbqsrc/params-attribute-example

Source:

#[rename_params(send_help)]
fn hello(#[angery(true)] a: i32, #[a2] b: i32, #[what = "how"] c: u32) {}

#[rename_params(send_help)]
impl Foo {
  fn hello(#[angery(true)] a: i32, #[a2] b: i32, #[what = "how"] c: u32) {}
  fn hello2(#[a1] #[a2] a: i32, #[what = "how"] b: i32, #[angery(true)] c: u32) {}
  fn hello_self(#[a1] #[a2] &self, #[a1] #[a2] a: i32, #[what = "how"] b: i32, #[angery(true)] c: u32) {}
}

Printed output:

fn hello(#[angery(true)] a: i32, #[a2] b: i32, #[what = "how"] c: u32) { }
impl Foo {
    fn hello(a: i32, #[a2] b: i32, #[what = "how"] c: u32) { }
    fn hello2(a: i32, #[what = "how"] b: i32, #[angery(true)] c: u32) { }
    fn hello_self(#[a1] #[a2] &self, #[a1] #[a2] a: i32,
                  #[what = "how"] b: i32, #[angery(true)] c: u32) {
    }
}

As you can see, hello and hello2 are missing their attrs on the a param. The addition of self however gives both self and the a param their attrs. Weird.

$ rustc --version
rustc 1.39.0-nightly (ed8b708c1 2019-09-21)
@bbqsrc
Copy link
Author

bbqsrc commented Sep 22, 2019

Ping @Centril

@bbqsrc
Copy link
Author

bbqsrc commented Sep 22, 2019

Also pinging @c410-f3r due to impact on #64010.

@jonas-schievink jonas-schievink added A-macros Area: All kinds of macros (custom derive, macro_rules!, proc macros, ..) C-bug Category: This is a bug. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. I-nominated A-attributes Area: Attributes (`#[…]`, `#![…]`) labels Sep 22, 2019
@c410-f3r
Copy link
Contributor

c410-f3r commented Sep 25, 2019

I'm rather busy this week and I can only take a look into it in this weekend

@pnkfelix
Copy link
Member

triage: P-high. Leaving nominated in hopes to find someone to look into it, since @c410-f3r won't have time for next few days.

@pnkfelix pnkfelix added the P-high High priority label Sep 26, 2019
@pnkfelix
Copy link
Member

assigning to @Centril

@Centril
Copy link
Contributor

Centril commented Sep 29, 2019

Investigation thus far: I don't think the problem is in pretty printing but somewhere else.

#![deny(unused_variables)]

struct Foo;

fn _f0(#[allow(unused_variables)] p0: u8) {}

impl Foo {
    fn _f1(#[allow(unused_variables)] p1: u8) {}
    
    fn _f2(&self, #[allow(unused_variables)] p2: u8) {}
}

results in:

error: unused variable: `p1`
 --> src/lib.rs:8:39
  |
8 |     fn _f1(#[allow(unused_variables)] p1: u8) {}
  |                                       ^^ help: consider prefixing with an underscore: `_p1`
  |
note: lint level defined here
 --> src/lib.rs:1:9
  |
1 | #![deny(unused_variables)]
  |         ^^^^^^^^^^^^^^^^

However: the problem is not in the parser:

pub struct S;

impl S {
    fn _f1(#[allow(lint)] p1: u8) {}
}

results in:

error[E0658]: attributes on function parameters are unstable
 --> src/lib.rs:4:12
  |
4 |     fn _f1(#[allow(lint)] p1: u8) {}
  |            ^^^^^^^^^^^^^^
  |
  = note: for more information, see https://github.com/rust-lang/rust/issues/60406

on stable.

But the problem does appear at least in AST validation:

pub struct S;

impl S {
    fn _f2(
        /// abc  <-- compiles but it should be rejected!
        _p1: u8
    ) {}
}

Actually, it turns out the problem is in the parser, and we have a bug that allows:

trait T {
    fn _f2(#[foo]) {}
}

Centril added a commit to Centril/rust that referenced this issue Sep 29, 2019
syntax: fix dropping of attribute on first param of non-method assocated fn

Fixes rust-lang#64682.

The general idea is that we bake parsing of `self` into `parse_param_general` and then we just use standard list parsing. Overall, this simplifies the parsing and makes it more consistent.

r? @petrochenkov cc @c410-f3r
Centril added a commit to Centril/rust that referenced this issue Sep 29, 2019
syntax: fix dropping of attribute on first param of non-method assocated fn

Fixes rust-lang#64682.

The general idea is that we bake parsing of `self` into `parse_param_general` and then we just use standard list parsing. Overall, this simplifies the parsing and makes it more consistent.

r? @petrochenkov cc @c410-f3r
@bors bors closed this as completed in 8fd03b1 Sep 29, 2019
Mark-Simulacrum pushed a commit to Mark-Simulacrum/rust that referenced this issue Oct 22, 2019
Fuse parsing of `self` into `parse_param_general`.
Mark-Simulacrum pushed a commit to Mark-Simulacrum/rust that referenced this issue Oct 26, 2019
Fuse parsing of `self` into `parse_param_general`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-attributes Area: Attributes (`#[…]`, `#![…]`) A-macros Area: All kinds of macros (custom derive, macro_rules!, proc macros, ..) C-bug Category: This is a bug. P-high High priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants