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

Remove OuterAttributes for StructPatternEtCetera #1200

Closed
wants to merge 1 commit into from

Conversation

pushkine
Copy link
Contributor

@pushkine pushkine commented May 2, 2022

Rust in stable and nightly treats outer attributes above .. as whitespace.
Running cargo fmt even removes it.

Snippet:

struct Foo {
	a: u8
}

fn main() {
	let Foo { #[rest_attribute] .. } = Foo { a: 1 };
	let Foo { #[prop_attribute] a  } = Foo { a: 1 };
}

cargo run:
rest_attribute is ignored

error: cannot find attribute `prop_attribute` in this scope
 --> src\main.rs:7:14
  |
7 |     let Foo { #[prop_attribute] a  } = Foo { a: 1 };
  |                 ^^^^^^^^^^^^^^

cargo fmt:
rest_attribute is removed

struct Foo {
    a: u8,
}

fn main() {
    let Foo { .. } = Foo { a: 1 };
    let Foo {
        #[prop_attribute]
        a,
    } = Foo { a: 1 };
}

@ehuss
Copy link
Contributor

ehuss commented May 2, 2022

Thanks for the PR! I don't think this is the correct change, as rustc does parse these attributes. This particular thing is a known issue tracked in rust-lang/rust#81282, where there is more discussion if this should be allowed or not.

@pushkine
Copy link
Contributor Author

pushkine commented May 2, 2022

@ehuss
The fact that they are parsed and silently ignored is a bug, not a feature

Unless this is a "macro" situation, like visibility specifiers that are allowed but ignored in certain contexts?
In that case this should be mentioned, note on the other hand that syn does not parse it

https://astexplorer.net/#/gist/4a0c99b76353c971afe9576405234183/b8c362bf14d52215dd8410294069b10a2a305b18

@ehuss
Copy link
Contributor

ehuss commented May 2, 2022

I don't think this is necessarily a macro situation. It's just a bug, and the lang team needs to make a call as to whether or not attributes should be allowed in that position. I personally would vote that it should be allowed to be consistent with other field patterns (and rust-lang/rust#81282 (comment) gives a motivating example).

@pushkine
Copy link
Contributor Author

pushkine commented May 4, 2022

Is the argument that this is an agreed upon incoming feature that is already parsed by rustc?

I don't think that this is a feature that exists anywhere outside docs though

  • The idea of such a feature was introduced by the docs 4+ years ago, there is no RFC supporting its implementation

  • The compiler does not even define .. as a Node with attributes, there is a bug in rustc_parse skipping attributes silently

    /// A struct or struct variant pattern (e.g., `Variant {x, y, ..}`).
    /// The `bool` is `true` in the presence of a `..`.
    Struct(Option<QSelf>, Path, Vec<PatField>, bool),

https://github.com/rust-lang/rust/blob/21d613b1116eef86d613d5f7f10d8fa5f22e27b3/compiler/rustc_ast/src/ast.rs#L725-L727

@ehuss
Copy link
Contributor

ehuss commented May 4, 2022

I'm not sure I understand your question. Attributes were added in rust-lang/rust#38814. There wasn't any discussion of attributes on the .. rest part. It parses it, but ignores it. I think that was an oversight, so the current behavior is a bug. Either rustc should honor the attributes, or it should reject them. That's a question for the lang team to answer in rust-lang/rust#81282.

@ehuss
Copy link
Contributor

ehuss commented May 20, 2022

I'm going to close. Further followup should be done in rust-lang/rust#81282, and either rustc or the reference can be updated based on the outcome of that issue.

@ehuss ehuss closed this May 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants