rustc_parse: restore public visibility on parse_attribute

[calebcartwright]

Make parse_attribute public as rustfmt is a downstream consumer. Refs #78782 (comment)

r? @petrochenkov

[petrochenkov]

I don't think rustfmt need to report these errors?
It could always pass InnerAttrPolicy::Permitted instead, and keep this constant private.

[calebcartwright]

We only use this parse_attribute function within a special case handling of cfg_if, and in this case immediately following the if kw. AIUI an inner attribute is not allowed here, which I presume is why we were passing false as the arg to the parse_attribute function before, which in turn applied the DEFAULT_INNER_ATTR_FORBIDDEN policy.

Are you saying we should still use the Permitted variant anyway?

https://github.com/rust-lang/rustfmt/blob/003786228d65135b9bae20a260caefc0248eb375/src/syntux/parser.rs#L225-L233

[petrochenkov]

I mean the only effect of setting InnerAttrPolicy::Forbidden is a call to span_err, which results in nothing in rustfmt?
I don't see any error diagnosticss when run rustfmt even on a wildly syntactically incorrect piece of Rust code.
So it shouldn't matter which argument is passed.

[calebcartwright]

I don't see any error diagnosticss when run rustfmt even on a wildly syntactically incorrect piece of Rust code.

Are you speaking in general/categorical terms or specifically within a cfg_if block? By any chance do you have hide_parse_errors enabled?

There are indeed a handful of cases were rustfmt forcibly suppresses the diagnostic detail, but in most cases the diagnostics are emitted (and the cases where we suppress have proven to be an occasional source of confusion for our users)

$ echo "pub struct S" | rustfmt
error: expected `where`, `{`, `(`, or `;` after struct name, found `<eof>`
 --> <stdin>:1:12
  |
1 | pub struct S
  |            ^ expected `where`, `{`, `(`, or `;` after struct name

And current behavior even within a cfg_if block would be for the forbidden inner attr error message to be displayed:

$ rustfmt --version
rustfmt 1.4.24-stable (eb894d5 2020-11-05)
$ cat src/lib.rs 
cfg_if! {
    if #![cfg(any(target_arch = "x86", target_arch = "x86_64"))] {
        println!("x86");
    } else if #[cfg(target_arch = "arm")] {
        println!("ARM");
    } else {
        // Unimplemented architecture:
        println!("unimplemented");
    }
}
$ rustfmt src/lib.rs --check
error: an inner attribute is not permitted in this context
 --> /home/caleb/dev/rustfmt-sandbox/cfg-if-inner/src/lib.rs:2:8
  |
2 |     if #![cfg(any(target_arch = "x86", target_arch = "x86_64"))] {
  |        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  |
  = note: inner attributes, like `#![no_std]`, annotate the item enclosing them, and are usually found at the beginning of source files. Outer attributes, like `#[test]`, annotate the item following them.

However, and perhaps to your point, while an inner attribute here is invalid that doesn't impact our ability to parse the contained items which is really what we're after on the rustfmt side. There's a few other cases of invalid syntax where we're still able to get the info needed for formatting and carry on anyway, so I think it's fair we do the same here (though it would be a slight behavior change).

I am a bit concerned that you aren't seeing any error diagnostics though, so any additional info you could share would be greatly appreciated!

[petrochenkov]

Ok, I used a wrong snipped previously.
I do see errors from the cfg_if! example above.
Why on earth does rustfmt reports them though? It's not its job?

[petrochenkov]

(Anyway, rustfmt is not obliged to report errors and this specific error doesn't affect formatting in any way, so it can be omitted.)

[calebcartwright]

Why on earth does rustfmt reports them though? It's not its job?

Generally speaking rustfmt doesn't suppress parser errors so that it doesn't silently fail and exit without giving users any inclination as to why it was unable to format. Since rustfmt works directly with the AST we're reliant on the parsing calls to succeed so that we have something valid/safe to reformat. However, in cases like this where invalid syntax doesn't impact formatting one way or the other, it really isn't necessary to emit and we can suppress/skip emission; user's will be informed whenever they actually run rustc.

(Anyway, rustfmt is not obliged to report errors and this specific error doesn't affect formatting in any way, so it can be omitted.)

Agreed! Will update momentarily

[calebcartwright]

Went ahead and squashed into a single commit vs adding a separate one for this, as the diff is so tiny

[petrochenkov]

You cannot make this private if parse_attribute is public, so the comment seems unnecessary.

[calebcartwright]

I can remove this if you'd prefer, but I've been trying to be very explicit about calling out instances where rustfmt ends up needing a higher visibility on something within the compiler than what would've been done otherwise.

Naturally the public visibility on the function requires this to be public as well, and in this case the two happen to be in the same file and fairly close together. However, I just think that for anyone looking at this code down the road and wondering "why is this enum public?" that it's beneficial to have the inline comment in context vs. having to do a search to find the comment on the function or reducing the visibility to try to let the compiler tell them.

Let me know how you'd like to proceed

[petrochenkov]

Ok, leaving this at your discretion.

[petrochenkov]

Thanks for the explanation!
@bors r+ rollup

[bors]

📌 Commit 5930a8a has been approved by petrochenkov

[bors]

⌛ Testing commit 5930a8a with merge 361543d...

[bors]

☀️ Test successful - checks-actions
Approved by: petrochenkov
Pushing 361543d to master...