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

parser warning "macro is experimental" interacts badly with module-level cfg #104633

Closed
RalfJung opened this issue Nov 20, 2022 · 18 comments · Fixed by #110141
Closed

parser warning "macro is experimental" interacts badly with module-level cfg #104633

RalfJung opened this issue Nov 20, 2022 · 18 comments · Fixed by #110141
Labels
A-attributes Area: #[attributes(..)] A-parser Area: The parsing of Rust source code to an AST. A-stability Area: issues related to #[stable] and #[unstable] attributes themselves. T-lang Relevant to the language team, which will review and decide on the PR/issue.

Comments

@RalfJung
Copy link
Member

RalfJung commented Nov 20, 2022

Given code like this

#![feature(decl_macro)]
#![cfg(someflagthatisnotset)]

pub macro mymacro() {}

what I expect should happen is that this works fine, since the feature flag is set so the parser should accept parsing the pub macro, and also the file is anyway empty after cfg expansion. However, I get a warning instead:

warning: `macro` is experimental
 --> src/lib.rs:4:1
  |
4 | pub macro mymacro() {}
  | ^^^^^^^^^^^^^^^^^^^^^^
  |
  = note: [see issue #39412 <https://github.com/rust-lang/rust/issues/39412>](https://github.com/rust-lang/rust/issues/39412) for more information
  = help: [add `#![feature(decl_macro)]`](https://play.rust-lang.org/?version=nightly&mode=debug&edition=2021#) to the crate attributes to enable
  = warning: unstable syntax can change at any point in the future, causing a hard error!
  = note: for more information, [see issue #65860 <https://github.com/rust-lang/rust/issues/65860>](https://github.com/rust-lang/rust/issues/65860)

It looks like the cfg does disable the feature flag, but does not disable parsing of the rest of the file. That is a problem, since it means it is impossible to cfg-out an entire file that contains experimental syntax. I thought I could fix this by ordering the cfg after the feature, but as the example shows that does not help.

This probably started happening with #99935.
Cc @CAD97 @petrochenkov

@CAD97
Copy link
Contributor

CAD97 commented Nov 20, 2022

Two bits:

  • Doesn't #![feature] have to be at the crate root?
  • You can #[cfg] out parsing of a module, but it has to be on the mod statement.

#[cfg] are always processed after parsing, so I believe the compat warning is correct here. At least, doing this same thing with properly gated syntax causes an error. For example,

#![cfg(FALSE)]

fn test() {
    do yeet;
}

is a hard error for the unstable/experimental syntax. The unstable syntaxes that are only warnings when cfgd out is more an unfortunate happenstance, since this makes them at least somewhat syntax-stable. The ones uplifted to warnings by #99935 were ones crater said were essentially not being used at the time it was originally tested.

@RalfJung
Copy link
Member Author

RalfJung commented Nov 20, 2022

Doesn't #![feature] have to be at the crate root?

Yes. But there are cases where one wants the entire crate to be cfg'd out, and that's where I ran into this. (See #104634 for the work-around. This is related to this cfg.)

You can #[cfg] out parsing of a module, but it has to be on the mod statement.

I assume this only works for mod file; though, not mod inline { ... }?

#[cfg] are always processed after parsing, so I believe the compat warning is correct here. At least, doing this same thing with properly gated syntax causes an error. For example,

The thing is, this should work, since I am setting the feature gate

#![feature(yeet_expr)]
#![cfg(FALSE_DOES_NOT_EXIST_IN_CFG)]

fn test() {
    do yeet;
}

If the parser has to parse all the things that are being cfg out, it should also take into account the feature gates. It is problematic that it happily ignores cfgd feature gates but then complains about other disabled things.

@petrochenkov
Copy link
Contributor

@RalfJung
This was discussed in detail in #65860, you even participated in the discussion.

@RalfJung
Copy link
Member Author

RalfJung commented Nov 20, 2022

I vaguely remember some discussions, but this seems slightly different? I am running in a nightly compiler either way. I am not asking that a stable compiler accept such a file. I don't think there is a stability risk here. The problem is that there is no way to set a feature flag in a way that the parser will always take it into account. Hm, re-reading that old issue it does sound pretty much the same. But I don't think there is a stability risk here -- I am totally fine with the stable compiler emitting an error here.

If the parser goes over the entire file, no matter the cfg, then why can't it take into account all feature, no matter the cfg? The current behavior does not seem coherent. I wouldn't complain if it parsed all things ignoring cfg and applied all feature gates to the parser, ignoring cfg.

@petrochenkov
Copy link
Contributor

why can't it take into account all feature, no matter the cfg?

Ah, this part, it may be doable, yes.

@RalfJung
Copy link
Member Author

RalfJung commented Nov 20, 2022

If a #![cfg] only erased the attributes that come after it, that would also work. But it seems to apply "backwards in code" so there's no place to put a feature that the parser would honor if there's a crate-level cfg disabling things.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Nov 20, 2022
move core::arch into separate file

This works around rust-lang#104633 which otherwise leads to warnings in miri-test-libstd.
@shepmaster
Copy link
Member

I have something similar. In SNAFU, I have a number of crates that are only for testing purposes. I was experimenting with yeet:

Cargo.toml

[package]
name = "repro"
version = "0.1.0"
edition = "2021"

[dependencies]

src/lib.rs

#![cfg(test)]
#![feature(yeet_expr)]

fn x() -> Result<(), i32> {
    do yeet 0;
}

cargo nightly test isn't happy:

error[E0658]: `do yeet` expression is experimental
 --> src/lib.rs:5:5
  |
5 |     do yeet 0;
  |     ^^^^^^^^^
  |
  = note: see issue #96373 <https://github.com/rust-lang/rust/issues/96373> for more information
  = help: add `#![feature(yeet_expr)]` to the crate attributes to enable

Is this the same case, or should I open a new issue?

@CAD97
Copy link
Contributor

CAD97 commented Nov 24, 2022

It's the same issue. I agree that it's reasonable to apply at least the #![feature] which occur before a #![cfg].

If they were outer attributes,

#[feature(...)]
#[cfg(...)]
crate {
}

the #[feature] attribute would be logically evaluated before the #[cfg] attribute is evaluated, and thus the #[cfg] shouldn't suppress the effect of the #[feature].

@RalfJung
Copy link
Member Author

@CAD97 yes that would resolve the issue for me. :)

@RalfJung
Copy link
Member Author

RalfJung commented Nov 25, 2022

I guess that would technically be a breaking change since this will currently compile on stable

#![feautre(...)]
#![cfg(notathing)]

However, this generates an empty crate, so... doesn't seem terribly likely? The cfg can be ordered up to make this stable-compatible. (And of course then the parser error do apply, and that is indeed their entire point.)

thomcc pushed a commit to tcdi/postgrestd that referenced this issue Feb 10, 2023
move core::arch into separate file

This works around rust-lang/rust#104633 which otherwise leads to warnings in miri-test-libstd.
@petrochenkov
Copy link
Contributor

petrochenkov commented Feb 18, 2023

Fixed in #108221.

@petrochenkov
Copy link
Contributor

It turns out stuff breaks if features are considered for fully unconfigured crates #108221 (comment).

@petrochenkov
Copy link
Contributor

petrochenkov commented Feb 20, 2023

the #[feature] attribute would be logically evaluated before the #[cfg] attribute is evaluated, and thus the #[cfg] shouldn't suppress the effect of the #[feature].

This is not how it currently works in other places in the language, the attribute expansion algorithm always picks cfg and cfg_attr attributes first for expansion, and then proceeds to attribute macros and inert attributes.

Things also break if we change the order to be fully left-to-right (I tried it, it would be nice to have this kind of simplification - #83331).

I guess we could make an exception and change the order for the specific list of "very early" attributes, but I'm not sure it's good idea, maybe we should just leave this papercut alone.

@petrochenkov
Copy link
Contributor

maybe we should just leave this papercut alone

On the other hand it then becomes impossible to fully unconfigure a crate if it directly contains unstable syntax, which also doesn't seem right.

I'll try to implement this left-to-right amendment for a second crater run, let's see what happens.
It's a hack, but these "very early" attributes are already hacky enough due to being required before the main expansion pass.

@RalfJung
Copy link
Member Author

On the other hand it then becomes impossible to fully unconfigure a crate if it directly contains unstable syntax, which also doesn't seem right.

Yeah, that is what prompted this issue. It can be worked around (by moving all unstable syntax our of the lib.rs file) but that is somewhat unsatisfying. Though given backwards compatibility constraints, none of the hacks that make this possible are really satisfying either...

I'll try to implement this left-to-right amendment for a second crater run, let's see what happens.

Sounds plausible. Another option might be to special-case only feature. AFAIK none of the other attributes cause problem with fully unconfiguring a crate?

@petrochenkov
Copy link
Contributor

Next attempt - #110141.

@jyn514 jyn514 added A-attributes Area: #[attributes(..)] A-parser Area: The parsing of Rust source code to an AST. labels Apr 15, 2023
@jyn514 jyn514 added A-stability Area: issues related to #[stable] and #[unstable] attributes themselves. T-lang Relevant to the language team, which will review and decide on the PR/issue. labels Apr 15, 2023
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Jun 9, 2023
…pkin

expand: Change how `#![cfg(FALSE)]` behaves on crate root

Previously it removed all other attributes from the crate root.
Now it removes only attributes below itself (during both regular expansion and pre-configuration).

So it becomes possible to configure some global crate properties even for fully unconfigured crates.

Fixes rust-lang#104633
Part of rust-lang#110082
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this issue Jun 9, 2023
…pkin

expand: Change how `#![cfg(FALSE)]` behaves on crate root

Previously it removed all other attributes from the crate root.
Now it removes only attributes below itself (during both regular expansion and pre-configuration).

So it becomes possible to configure some global crate properties even for fully unconfigured crates.

Fixes rust-lang#104633
Part of rust-lang#110082
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this issue Jun 9, 2023
…pkin

expand: Change how `#![cfg(FALSE)]` behaves on crate root

Previously it removed all other attributes from the crate root.
Now it removes only attributes below itself (during both regular expansion and pre-configuration).

So it becomes possible to configure some global crate properties even for fully unconfigured crates.

Fixes rust-lang#104633
Part of rust-lang#110082
@bors bors closed this as completed in 2baebad Jun 10, 2023
@RalfJung
Copy link
Member Author

Hm, this had some odd fallout in miri-test-libstd, not sure what is going on there
https://github.com/rust-lang/miri-test-libstd/actions/runs/5233119090/jobs/9448362774

@RalfJung
Copy link
Member Author

RalfJung commented Jun 11, 2023

Okay it's mostly resolved by reordering as one would expect: #112535.

But one strange lint remains:

warning: unknown lint: `fuzzy_provenance_casts`
   --> std_miri_test/../library/std/src/lib.rs:610:1
    |
610 | #[allow(dead_code, unused_attributes, fuzzy_provenance_casts)]
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = note: the `fuzzy_provenance_casts` lint is unstable
    = note: see issue #95228 <https://github.com/rust-lang/rust/issues/95228> for more information
    = help: add `#![feature(strict_provenance)]` to the crate attributes to enable
    = note: `#[warn(unknown_lints)]` on by default

@petrochenkov looks like it is processing that allow(...) deep inside the crate (it is attached to a mod) but not processing the feature(strict_provenance) at the crate root?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-attributes Area: #[attributes(..)] A-parser Area: The parsing of Rust source code to an AST. A-stability Area: issues related to #[stable] and #[unstable] attributes themselves. T-lang Relevant to the language 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