Deprecate crate_type and crate_name nested inside #![cfg_attr]

[bjorn3]

This implements the proposal in #83676 (comment), with a future compatibility lint imposed on usage of crate_type/crate_name inside cfg's.

This is a compromise between removing #![crate_type] and #![crate_name] completely and keeping them as a whole, which requires somewhat of a hack in rustc and is impossible to support by gcc-rust. By only removing #![crate_type] and #![crate_name] nested inside #![cfg_attr] it becomes possible to parse them before a big chunk of the compiler has started.

Replaces #83676

#![crate_type = "lib"] // remains working
#![cfg_attr(foo, crate_type = "bin")] // will stop working

Rationale

As it currently is it is possible to try to access the stable crate id before it is actually set, which will panic. The fact that the Session contains mutable state beyond debugging things also doesn't completely sit well with me. Especially once parallel rustc becomes the default.

I think there is currently also a cyclic dependency where you need to set the stable crate id to be able to load crates, but you need to load crates to expand proc macro attributes that may define #![crate_name] or #![crate_type]. Currently crate level proc macro attributes are unstable or completely unsupported (can't remember which), so this is not a problem, but it may become an issue in the future.

Finally if we want to add incremental compilation to macro expansion or even parsing, we need the StableCrateId to be created together with the Session or even earlier as incremental compilation determines the incremental compilation session dir based on the StableCrateId.

[rust-highfive]

r? @matthewjasper

(rust-highfive has picked a reviewer for you, use r? to override)

[bjorn3]

Support for this was introduced in #25399.

Part of the rationale was: #25347 (comment)

I'm actually kind of surprised to see this bug. I would have expected #![cfg_attr] to be used in order to enable features based on cargo features, e.g. a cargo feature nightly that enables the use of features such that the same lib can be used in the beta and nightly channels.

This is actually not true. #![crate_name] must match the value passed in using --crate-name by cargo.

$ echo '#![crate_name = "foo"] fn main() {}' | rustc - --crate-name bar
error: `--crate-name` and `#[crate_name]` are required to match, but `bar` != `foo`
 --> <anon>:1:1
  |
1 | #![crate_name = "foo"] fn main() {}
  | ^^^^^^^^^^^^^^^^^^^^^^

error: aborting due to previous error

Surprisingly #![crate_type] is even completely ignored when using --crate-type:

$ echo '#![crate_type = "lib"] fn main() {}' | rustc - --crate-type bin
$ ls
rust_out

I thought it merely had to match, just like the crate name.

[joshtriplett]

@bjorn3 To confirm, based on your knowledge of the compiler internals, you believe that if we manage to remove this and require that these directives only appear at the top level if at all, we can substantially simplify the logic to handle them in the Rust compiler? Your description that it's "somewhat of a hack in rustc" sounds like there's simplification to be had, and that simplification would be a reasonable motivation to make this change if it turns out no crates use it.

[bjorn3]

Your description that it's "somewhat of a hack in rustc" sounds like there's simplification to be had

Indeed

and that simplification would be a reasonable motivation to make this change if it turns out no crates use it.

IMHO yes. In addition #![cfg_attr(..., crate_type = "...")] can't be supported by rust-gcc.

[joshtriplett]

The former seems like a sufficient rationale to give this a try. Starting a try run in preparation for a crater run. If the crater run comes back clean, this seems potentially reasonable.

[joshtriplett]

@bors try

[bors]

⌛ Trying commit 8e4eb5f3283e0d02c4d46d144e6146011d1d4e73 with merge 45d8b6e68beee1c9c1ce92ae258a7e02f8f13aa4...

[bors]

☀️ Try build successful - checks-actions
Build commit: 45d8b6e68beee1c9c1ce92ae258a7e02f8f13aa4 (45d8b6e68beee1c9c1ce92ae258a7e02f8f13aa4)

[bjorn3]

@craterbot check

[craterbot]

👌 Experiment pr-83744 created and queued.
🤖 Automatically detected try build 45d8b6e68beee1c9c1ce92ae258a7e02f8f13aa4
⚠️ Try build based on commit 4fa76a4, but latest commit is 8e4eb5f3283e0d02c4d46d144e6146011d1d4e73. Did you forget to make a new try build?
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[craterbot]

🚧 Experiment pr-83744 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[craterbot]

🎉 Experiment pr-83744 is completed!
📊 9 regressed and 17 fixed (154464 total)
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the blacklist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[bjorn3]

All regressions are spurious. They are either network errors or linkers crashing without any output.

[est31]

That's really good, although a crater report might not be best suited to turn up cases where these things may occur, as I expect them to be used mainly outside of cargo built codebases.

I've looke for crate_type and crate_name on google's opensource code search, couldn't find anything that could cause a problem.

I've used searchcode.com to look for any repos on gitlab or bitbucket, but no cfg_attr use showed up. crate_type, crate_name.

Using searchcode.com for github turns up too much repetitive stuff for it to be sifted through reasonably in a manual fashion. Same goes for github's builtin search, no way to narrow it down. Anyone know of a better search engine?

[bjorn3]

grep.app shows exactly one usage: https://github.com/dropbox/divans/blob/23459c22f8a63e0a70ef1b968402ae0b3682168f/src/lib.rs#L21 This also enables two feature gates when the same cfg is used and they use cargo so it shouldn't have any effect.

(I believe grep.app doesn't search all of github, but it does search a large portion in my experience)

[est31]

Interesting, thanks for pointing to grep.app!

I'm a bit familiar with the repo it found, having filed a PR against it in 2018. It has been ignored for 2 years until I closed it. In that same period, nothing else has been commited to the repo's master branch (or any other PR filed/merged). So it's kinda dead I'd say. It might be used internally at dropbox, idk.

So overall the fallout is manageable I think.

[bjorn3]

Can this land as is, or should I turn it into a future compatibility lint?

[Mark-Simulacrum]

Discussed during lang meeting today -- we agreed this doesn't need FCP given just adding a future compatibility warning. I'll take a look at the impl itself shortly and request changes or indicate approval modulo the rebase.

[Mark-Simulacrum]

r=me with the future compat issue filed

[Mark-Simulacrum]

@bors r+ rollup

[bors]

📌 Commit 9b6c510 has been approved by Mark-Simulacrum