Deprecate #![crate_type] and #![crate_name] #83676

bjorn3 · 2021-03-30T13:26:31Z

#![crate_type] is implemented using a OnceCell in the Session to allow for setting it after the main source file has already been parsed. This is rather ugly IMO and makes it easy to attempt to access it before it has been set.

#![crate_name] is implemented by threading the determined crate name everywhere after the crate has been parsed up till the moment the TyCtxt is created, at which point the crate_name stored in TyCtxt is used. A local patch which used the same technique as #![crate_type] resulted in "17 files changed, 72 insertions(+), 127 deletions(-)".

Both #![crate_type] and #![crate_name] are useless when using Cargo as they must match the values passed in at the command line. Other build systems are unlikely to use either attribute too.

I propose that #![crate_type] and #![crate_name] will be deprecated and at a future date removed.

rust-highfive · 2021-03-30T13:26:34Z

r? @estebank

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

rust-log-analyzer · 2021-03-30T13:35:00Z

The job x86_64-gnu-llvm-10 failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

   Compiling cc v1.0.60
   Compiling core v0.0.0 (/checkout/library/core)
   Compiling libc v0.2.88
   Compiling std v0.0.0 (/checkout/library/std)
thread 'rustc' panicked at 'compiler/rustc_lint/src/context.rs:209:17: duplicate specification of lint deprecated_crate_info_attrs', /checkout/library/std/src/panic.rs:59:5

error: internal compiler error: unexpected panic

note: the compiler unexpectedly panicked. this is a bug.
note: the compiler unexpectedly panicked. this is a bug.

note: we would appreciate a bug report: https://github.com/rust-lang/rust/issues/new?labels=C-bug%2C+I-ICE%2C+T-compiler&template=ice.md

note: rustc 1.53.0-nightly (d4170586f 2021-03-30) running on x86_64-unknown-linux-gnu

note: compiler flags: -Z macro-backtrace -Z binary-dep-depinfo -Z force-unstable-if-unmarked -C opt-level=3 -C embed-bitcode=no -C codegen-units=1 -C debuginfo=0 -C debug-assertions=on -C overflow-checks=off -C link-args=-Wl,-rpath,$ORIGIN/../lib -C prefer-dynamic -C llvm-args=-import-instr-limit=10 -C embed-bitcode=yes --crate-type lib
note: some of the compiler flags provided by cargo are hidden

query stack during panic:
end of query stack

bjorn3 · 2021-03-30T13:39:16Z

This has previously been discussed a bit in https://rust-lang.zulipchat.com/#narrow/stream/268952-edition-2021/topic/remove.20.23!.5Bcrate_type.5D

jyn514 · 2021-03-30T14:36:32Z

These are used quite a lot for tests, and I also use them for testing rustdoc issues locally. It would be annoying to have to run with --crate-name each time I invoke rustdoc.

$ rg crate_type src/test/ | wc -l
986
$ rg crate_name src/test/ | wc -l
332

nagisa · 2021-03-30T14:37:14Z

This attribute is very extensively used in tests and similar one-off programs that are compiled with rustc directly. I know I type this attribute out at least once every week.

I would like to see what the impact of this change would be by running crater with this lint denied.

Perhaps MCP too, though I guess we can FCP this PR too.

I'm worried all sorts of playground and godbolt links would break once this lint is denied too.

bjorn3 · 2021-03-30T15:34:24Z

As a middle ground could we keep #![crate_type] and deprecate #![crate_name]? Most of the time the crate name is correctly inferred from the file name if --crate-name isn't passed. The accesses of the crate type are more localized than the accesses of the crate name, so it is easier to ensure that the crate type is set before accessing it.

jyn514 · 2021-03-30T15:39:13Z

Can you explain why you want to deprecate these? You've mentioned that the thread-locals feel hacky and that this is unhelpful when using cargo, but not everyone uses cargo and I don't think we should remove language features just because it cleans up the implementation.

bjorn3 · 2021-03-30T16:00:11Z

It is not a thread local, but a field of the Session that is set after Session is already actively being used in case of the crate type. This means that you have to be very careful to not request either property until they have been set on the Session. For example you can't use it from the config callback of rustc_interface at all. The crate name is currently manually passed around everywhere until TyCtxt which is anoying, but with a local patch it would become a field just like the crate type, which requires the same careful handling as for the crate type.

jyn514 · 2021-03-30T18:24:35Z

I marked this as needs-fcp because I think the lang team should weigh in on deprecating any language features.

joshtriplett · 2021-03-30T23:11:29Z

Personally, I would love to see these deprecated, in favor of the command-line options. The command-line options don't require Cargo, they just require passing the values into rustc from whatever is invoking rustc. (Test cases could always store this information in a specially formatted comment, similar to what the Rust testsuite does.)

However, these attributes are part of the language, and while we could consider deprecating them, I don't think we can reasonably remove them except as part of an edition boundary.

@bjorn3 Would any of the potential simplifications you're describing be possible if we need to retain compatibility with previous editions that support these attributes indefinitely? (For instance, assume that cosmetic issues such as messages were not a problem, and we only had to produce a compatible compiled crate.)

If there's still substantial improvement available even given that constraint, I think this might be worth considering. If there isn't, then I don't think this would be worth the churn it would introduce.

bjorn3 · 2021-03-31T05:57:05Z

Would any of the potential simplifications you're describing be possible if we need to retain compatibility with previous editions that support these attributes indefinitely?

No, unfortunately not. Keeping the attributes, requiring them to be equal to the default value when no commandline argument is passed instead of only requiring a match when the commandline arguments are passed would allow for these simplifications though. In addition I wonder if gcc-rs could even support these attributes at all or whether the gcc driver requires to know of it needs to link in advance before invoking the language specific frontend. (cc @philberty) Removing on an edition boundary would help other backends if they choose to not support older editions.

Havvy · 2021-03-31T08:36:39Z

if they choose to not support older editions

As per the definition and intent of editions, you're not a valid Rust compiler unless you actually do support older editions.

philberty · 2021-03-31T08:50:20Z

Would any of the potential simplifications you're describing be possible if we need to retain compatibility with previous editions that support these attributes indefinitely?

No, unfortunately not. Keeping the attributes, requiring them to be equal to the default value when no commandline argument is passed instead of only requiring a match when the commandline arguments are passed would allow for these simplifications though. In addition I wonder if gcc-rs could even support these attributes at all or whether the gcc driver requires to know of it needs to link in advance before invoking the language specific frontend. (cc @philberty) Removing on an edition boundary would help other backends if they choose to not support older editions.

Thanks, @bjorn3 for the heads up. I think there is a method to implement #![crate_name] (by overriding passed options) but no other FE in GCC does this and would likely just cause more problems than it solves. As for #![crate_type] I am currently unaware of any method to actually implement this in gcc-rs. @tschwinge or @dkm do you have any opinions on this?

est31 · 2021-03-31T16:29:47Z

What about deprecating and removing cfg_attr-dependent #[crate_type]? Then it would become extremely easy to parse it very early on.

philberty · 2021-03-31T16:43:26Z

I personally think @est31 proposal of deprecating crate_type is a great move forward, this is a difficult thing for GCC Rust to support in a graceful way. Crate name is something that is much more easily implemented.

I would like to see this change moving forward in Rust.

est31 · 2021-03-31T17:34:53Z

(To clarify, my proposal also extends to #[crate_name]. basically still support them but deprecate&remove cfg_attr(something, crate_name = ...) and cfg_attr(something, crate_type = ...). This makes sure they can be parsed very early on, before all the cfg expansion logic is set up)

joshtriplett · 2021-04-01T06:02:41Z

The PR as originally proposed here isn't something we can merge; removing these would break existing code, and we shouldn't deprecate them if we don't plan to remove them. And it sounds like even dropping these in an edition wouldn't help, even if that were something we wanted to do. So, I'm going to close this PR.

Proposals to make some other change in this area should likely be a separate PR, and would almost certainly need both an FCP and a crater run.

bjorn3 · 2021-04-01T10:18:24Z

What about deprecating and removing cfg_attr-dependent #[crate_type]? Then it would become extremely easy to parse it very early on.

Done in #83744

…e_name, r=Mark-Simulacrum Deprecate crate_type and crate_name nested inside #![cfg_attr] This implements the proposal in rust-lang#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 rust-lang#83676 ```rust #![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.

bjorn3 added 4 commits March 30, 2021 15:00

Deprecate #![crate_type]

7d169a6

Fix file extension for two tests

7057644

Add tests

5f290b6

Remove #![crate_type] and #![crate_name] usage from a few crates

117aa27

rust-highfive assigned estebank Mar 30, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 30, 2021

jyn514 added needs-fcp This change is insta-stable, so needs a completed FCP to proceed. T-lang Relevant to the language team, which will review and decide on the PR/issue. labels Mar 30, 2021

joshtriplett closed this Apr 1, 2021

bjorn3 deleted the deprecate_crate_type_name branch April 1, 2021 08:47

bjorn3 mentioned this pull request Apr 1, 2021

Deprecate crate_type and crate_name nested inside #![cfg_attr] #83744

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deprecate #![crate_type] and #![crate_name] #83676

Deprecate #![crate_type] and #![crate_name] #83676

bjorn3 commented Mar 30, 2021

rust-highfive commented Mar 30, 2021

rust-log-analyzer commented Mar 30, 2021

bjorn3 commented Mar 30, 2021

jyn514 commented Mar 30, 2021

nagisa commented Mar 30, 2021 •

edited

Loading

bjorn3 commented Mar 30, 2021

jyn514 commented Mar 30, 2021

bjorn3 commented Mar 30, 2021

jyn514 commented Mar 30, 2021

joshtriplett commented Mar 30, 2021

bjorn3 commented Mar 31, 2021 •

edited

Loading

Havvy commented Mar 31, 2021

philberty commented Mar 31, 2021

est31 commented Mar 31, 2021

philberty commented Mar 31, 2021

est31 commented Mar 31, 2021

joshtriplett commented Apr 1, 2021 •

edited

Loading

bjorn3 commented Apr 1, 2021

Deprecate #![crate_type] and #![crate_name] #83676

Deprecate #![crate_type] and #![crate_name] #83676

Conversation

bjorn3 commented Mar 30, 2021

rust-highfive commented Mar 30, 2021

rust-log-analyzer commented Mar 30, 2021

bjorn3 commented Mar 30, 2021

jyn514 commented Mar 30, 2021

nagisa commented Mar 30, 2021 • edited Loading

bjorn3 commented Mar 30, 2021

jyn514 commented Mar 30, 2021

bjorn3 commented Mar 30, 2021

jyn514 commented Mar 30, 2021

joshtriplett commented Mar 30, 2021

bjorn3 commented Mar 31, 2021 • edited Loading

Havvy commented Mar 31, 2021

philberty commented Mar 31, 2021

est31 commented Mar 31, 2021

philberty commented Mar 31, 2021

est31 commented Mar 31, 2021

joshtriplett commented Apr 1, 2021 • edited Loading

bjorn3 commented Apr 1, 2021

nagisa commented Mar 30, 2021 •

edited

Loading

bjorn3 commented Mar 31, 2021 •

edited

Loading

joshtriplett commented Apr 1, 2021 •

edited

Loading