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

Tracking issue for future-incompatibility lint deprecated_cfg_attr_crate_type_name #91632

Closed
3 tasks done
bjorn3 opened this issue Dec 7, 2021 · 27 comments
Closed
3 tasks done
Labels
A-attributes Area: Attributes (`#[…]`, `#![…]`) A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. C-future-incompatibility Category: Future-incompatibility lints C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-lang Relevant to the language team, which will review and decide on the PR/issue.

Comments

@bjorn3
Copy link
Member

bjorn3 commented Dec 7, 2021

What is this lint about

The deprecated_cfg_attr_crate_type_name lint detects uses of the #![cfg_attr(..., crate_type = "...")] and #![cfg_attr(..., crate_name = "...")] attributes to conditionally specify the crate type and name in the source code. For example:

#![cfg_attr(debug_assertions, crate_type = "lib")]

Explanation

The #![crate_type] and #![crate_name] attributes require a hack in the compiler to be able to change the used crate type and crate name after macros have been expanded. Neither attribute works in combination with Cargo as it explicitly passes --crate-type and --crate-name on the commandline. These values must match the value used in the source code to prevent an error.

How to fix this warning/error

To fix the warning use --crate-type on the commandline when running rustc instead of #![cfg_attr(..., crate_type = "...")] and --crate-name instead of #![cfg_attr(..., crate_name = "...")].

We are interested to hear about any/all use cases for conditional #![crate_type] and #![crate_name] attributes.

Current status

@est31
Copy link
Member

est31 commented Jul 26, 2022

I've filed a PR to make this deny-by-default: #99784

compiler-errors added a commit to compiler-errors/rust that referenced this issue Aug 27, 2022
…, r=Mark-Simulacrum

Make forward compatibility lint deprecated_cfg_attr_crate_type_name deny by default

Turns the forward compatibility lint added by rust-lang#83744 to deprecate `cfg_attr` usage with `#![crate_type]` and `#![crate_name]` attributes into deny by default. Copying the example from rust-lang#83744:

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

Over 8 months have passed since rust-lang#83744 was merged so I'd say this gives ample time for people to have been warned, so we can make the warning stronger. No usage was found via grep.app except for one, which was in an unmaintained code base that didn't seem to be used in the open source eco system. The crater run conducted in rust-lang#83744 also didn't show up anything.

cc rust-lang#91632 - tracking issue for the lint
JohnTitor added a commit to JohnTitor/rust that referenced this issue Aug 27, 2022
…, r=Mark-Simulacrum

Make forward compatibility lint deprecated_cfg_attr_crate_type_name deny by default

Turns the forward compatibility lint added by rust-lang#83744 to deprecate `cfg_attr` usage with `#![crate_type]` and `#![crate_name]` attributes into deny by default. Copying the example from rust-lang#83744:

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

Over 8 months have passed since rust-lang#83744 was merged so I'd say this gives ample time for people to have been warned, so we can make the warning stronger. No usage was found via grep.app except for one, which was in an unmaintained code base that didn't seem to be used in the open source eco system. The crater run conducted in rust-lang#83744 also didn't show up anything.

cc rust-lang#91632 - tracking issue for the lint
@est31
Copy link
Member

est31 commented Mar 21, 2023

Since the grep.app seach conducted in the original PR there have been two new crates using it, despite the lint:

wasmer:

#![cfg_attr(feature = "js", crate_type = "cdylib")]
#![cfg_attr(feature = "js", crate_type = "rlib")]

azul:

#![cfg_attr(feature ="cdylib", crate_type = "cdylib")]
#![cfg_attr(feature ="staticlib", crate_type = "staticlib")]
#![cfg_attr(feature ="rlib", crate_type = "rlib")]

Both uses were added in 2021, after #83744 was opened and before it was merged. Also both only modify crate_type. As @bjorn3 has asked above about use cases for crate_type within cfg_attr(), it might be interesting to think about alternatives one can suggest to the maintainers of these libraries.

Maybe one could extend cargo to support cargo:rustc-crate-type in build.rs scripts? Alternatively, one could maybe suggest adding a crate that has crate-type set in Cargo.toml, and then include!()s the lib.rs?

@bjorn3
Copy link
Member Author

bjorn3 commented Mar 21, 2023

Wouldn't cargo build --crate-type cdylib suffice in both cases?

@est31
Copy link
Member

est31 commented Mar 21, 2023

It should actually, good point. Maybe the wasm-pack equivalent of it for wasmer, but yes. Also as you've wrote earlier, #![crate_type = "..."] is totally ignored if --crate-type is present (only then though, if it's not present it's not ignored, but it seems that cargo always sets it?? not 100% sure). Ideally one would interact with downstream though before proposing a PR to remove.

@ia0
Copy link
Contributor

ia0 commented Jun 14, 2023

Wouldn't cargo build --crate-type cdylib suffice in both cases?

This doesn't work for me. My understanding is that --crate-type is only a rustc flag, you can't use it with cargo build. I would really like it to work with cargo build though. Is this planned?

@bjorn3
Copy link
Member Author

bjorn3 commented Jun 14, 2023

Right, --crate-type is a flag for cargo rustc, not cargo build. My bad. cargo rustc should work in most cases that cargo build does. The main limitation is that it requires a single target within a single package rather than allowing multiple packages and targets.

@ia0
Copy link
Contributor

ia0 commented Jun 15, 2023

Oh that's very interesting, I didn't know that. I always assumed cargo rustc was just a wrapper around a single invocation of rustc requiring you to compile dependencies separately and pass them on the command line. That's good to know, that should solve my issue. I think there's almost no cases where cargo build is useful to me then since I always use a single package (I never use workspaces) and always compile for a single target.

@est31
Copy link
Member

est31 commented Aug 5, 2023

Nice, seems the azul usage has been removed in the meantime: fschutt/azul@6096a1b#diff-2486c782348527d5e8a121166f399ed1449c5960b8079859b573cbc5f8b184c2L3

Now only the usage in wasmer is remaining. It has been reduced to a single attr:

https://github.com/wasmerio/wasmer/blob/ccbe870770604ecb61f2d8e0fa298504381f6784/lib/api/src/lib.rs#L29

I wonder whether it would be possible to enable cdylib on the api crate unconditionally?

@nyurik
Copy link
Contributor

nyurik commented Sep 5, 2023

Note that the docs still suggest this attribute

@bjorn3
Copy link
Member Author

bjorn3 commented Sep 5, 2023

Using plain #![crate_name = "..."] and #![crate_type = "..."] is fine. Using them behind a #![cfg_attr(...)] triggers this lint.

@nyurik
Copy link
Contributor

nyurik commented Sep 5, 2023

Is there a recommended way for feature-controlled crate-type, e.g. with a build.rs? A crate with optional cdylib type would also need to have a feature-gated panic handler and possibly other things, so it would still have to have a cdylib feature in Cargo.toml. So one would have to run this -- a probable source of bugs?

RUSTFLAGS='--crate-type cdylib' cargo build --features cdylib

P.S. Good point @bjorn3, thx.

@bjorn3
Copy link
Member Author

bjorn3 commented Sep 5, 2023

cargo rustc accepts --crate-type to override the crate type. A build script can't change the crate type and #![crate_type] doesn't work as youbl did intend with cargo anyway. Rustc requires that --crate-type and #![crate_type] match up if both are specified afaik and cargo always passes --crate-type.

@nyurik
Copy link
Contributor

nyurik commented Sep 5, 2023

thx, i think i will have to:

  • create a new sub crate mylib-cdylib/ in the same workspace/repo
  • add a new non-enabled-by-default cdylib feature to the main lib's /Cargo.toml
  • make mylib-cdylib crate depend on mylib with path = "..", features = ["cdylib"]
  • will add the #![crate_type = "cdylib"] to the mylib-cdylib/src/lib.rs
  • will add any other extra code required to compile a proper cdylib to the mylib-cdylib as needed

@bjorn3
Copy link
Member Author

bjorn3 commented Sep 5, 2023

With cargo rustc --crate-type cdylib --features cdylib I think you don't need a separate mylib-cdylib crate.

@traviscross
Copy link
Contributor

@rustbot labels +I-lang-nominated

Let's discuss whether to make this a hard error in Rust 2024.

@rustbot rustbot added the I-lang-nominated Nominated for discussion during a lang team meeting. label Nov 15, 2023
@traviscross traviscross removed the I-lang-nominated Nominated for discussion during a lang team meeting. label Nov 15, 2023
@delta4chat
Copy link

delta4chat commented Feb 29, 2024

The problem is that sometimes dylib, cdylib, or staticlib are not available (and cause compilation errors).
The only way to resolve this problem, that is, make rlib the default option, and to enable dylib under certain conditions, e.g. with #![no_std]:

error: "#[panic_handler]" function required, but not found

error: language item required, but not found: "eh_personality"
  |
  = note: this can occur when a binary crate with "#![no_std]" is compiled for a target where "eh_personality" is defined in the standard library
  = help: you may be able to compile for a target that doesn't need "eh_personality", specify a target with "--target" or in ".cargo/config"

So try doing something like this in lib.rs:

#![cfg_attr(feature="std", crate_type = "cdylib")]

Only attempt to compile cdylib library format if the std feature is available.

@delta4chat
Copy link

delta4chat commented Feb 29, 2024

  1. because build.rs cannot change rustc --crate-type compile flags, and no equivalence in Cargo.toml.
  2. [profiles] in Cargo.toml does not working due to no effect for crate-type.
  3. this also not the correct method Because it can't use features as a condition.:
[target.x86_64-<triple>]
rustc-link-lib = ["foo"]
rustc-link-search = ["/path/to/foo"]
rustc-flags = "-L /some/path"
rustc-cfg = ['key="value"']
rustc-env = {key = "value"}
rustc-cdylib-link-arg = ["…"]
metadata_key1 = "value"
metadata_key2 = "value"

Or, the only way I think, that is just write my own make script and use cargo rustc to handle this situation?
Or can someone tell me a better alternative? (And without include! macro or other complex and non-intuitive solutions?)

@ia0
Copy link
Contributor

ia0 commented Feb 29, 2024

Or, the only way I think, that is just write my own make script and use cargo rustc to handle this situation?

That's what was suggested in #91632 (comment) and what I'm doing since then. And I didn't have any issue with it so far, so would definitely recommend if that's something your project would permit.

@delta4chat
Copy link

and what I'm doing

Yes, this solution is actually similar to a make script, except that it's written in Rust. Rust calls Rust to compile Rust hahaha.

So if we envision an elegant way to handle this situation without using this "make script" approach, it might be to add this logic to Cargo.toml, such as "Conditionally Set Rustc Compilation Flags Based On Features".

@fmease fmease added the A-attributes Area: Attributes (`#[…]`, `#![…]`) label Mar 1, 2024
@est31
Copy link
Member

est31 commented Mar 2, 2024

I don't think there is any "elegant" solution to this problem, but there is a bunch of solutions:

  • You can specify the crate type manually using cargo rustc. Usually you require specific crate types when integrating the project somewhere, you can swap out cargo build there with cargo rustc --crate-type. You can also add an alias if you do manual invocation.
  • You can specify multiple crates each with their own #![crate_type], and then include!() the central crate. Often it's enough to not even just include, just reexporting the API of that other crate is enough, not 100% sure about that though.

I think it's fine to live without an "elegant" solution because the use cases are pretty niche.

@delta4chat
Copy link

delta4chat commented Mar 2, 2024

I don't think there is any "elegant" solution

  1. Yes, that's why it's important to use a bash script or Makefile to do this (because Cargo can't do it). It also means that Cargo can't cover all use cases.

I think it's fine to live without an "elegant" solution because the use cases are pretty niche.

  1. so no-std & dylib are pretty niche?

  1. The issue is that even if you use panic="abort" in Cargo.toml, then Rustc will still requires libunwind and eh_personality: no_std + liballoc can not compile with os based toolchain caused by eh_personality required #106864 / Unable to build 'no_std' crate into dylib #104159 / Using -Z build-std-features=panic_immediate_abort on Linux results in invalid executable. #97602 / Error "cannot find -lunwind" when compiling static mips-unknown-linux-musl  #120655

  2. also this causes most of Rust programs needs dynamic linking, even you specify +crt-static: aarch64-unknown-linux-musl use -lunwind, but my embedded system has no unwind #118400
    4.1. this also breaks cross-rs/android: Target aarch64-linux-android link failure: unwind. cross-rs/cross#1222

  3. but -Z build-std-features=panic_immediate_abort is still unstable/nightly, can not use it for stable Rust. Stabilizing panic_immediate_abort #115022

5.1. Since "not all use cases require panic support", I think the Rust compiler should completely disable "eh_personality" by default if #![no_std] specified. or opt-in / opt-out (by flag)

@traviscross traviscross added T-lang Relevant to the language team, which will review and decide on the PR/issue. C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC labels Jul 13, 2024
@est31
Copy link
Member

est31 commented Aug 27, 2024

I've filed a PR now to make the lint a hard error: #129670

There has been enough time for users to move to alternatives discussed in this thread: It's been two years since my earlier PR to make the deprecation lint warn by default has been merged: #99784 .

@fmease fmease changed the title Tracking Issue for deprecated_cfg_attr_crate_type_name lint Tracking issue for future-incompatible lint deprecated_cfg_attr_crate_type_name Sep 14, 2024
@fmease fmease added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Sep 14, 2024
@fmease fmease changed the title Tracking issue for future-incompatible lint deprecated_cfg_attr_crate_type_name Tracking issue for future-incompatibility lint deprecated_cfg_attr_crate_type_name Sep 14, 2024
@fmease fmease added the A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. label Sep 14, 2024
bors added a commit to rust-lang-ci/rust that referenced this issue Oct 6, 2024
…, r=Urgau

Make deprecated_cfg_attr_crate_type_name a hard error

Turns the forward compatibility lint added by rust-lang#83744 into a hard error, so now, while the `#![crate_name]` and `#![crate_type]` attributes are still allowed in raw form, they are now forbidden to be nested inside a `#![cfg_attr()]` attribute.

The following will now be an error:

```Rust
#![cfg_attr(foo, crate_name = "foobar")]
#![cfg_attr(foo, crate_type = "bin")]
```

This code will continue working and is not deprecated:

```Rust
#![crate_name = "foobar"]
#![crate_type = "lib"]
```

The reasoning for this is explained in rust-lang#83744: it allows us to not have to cfg-expand in order to determine the crate's type and name.

As of filing the PR, exactly two years have passed since rust-lang#99784 has been merged, which has turned the lint's default warning level into an error, so there has been ample time to move off the now-forbidden syntax.

cc rust-lang#91632 - tracking issue for the lint
@est31
Copy link
Member

est31 commented Oct 8, 2024

Closing as #129670 has been merged. It's on the path to being a hard error in the 1.83.0 release of Rust.

@est31 est31 closed this as completed Oct 8, 2024
@traviscross traviscross added the relnotes Marks issues that should be documented in the release notes of the next release. label Oct 8, 2024
@traviscross traviscross removed the relnotes Marks issues that should be documented in the release notes of the next release. label Oct 8, 2024
@est31
Copy link
Member

est31 commented Oct 17, 2024

I've tried to work on the followup refactor to #129670 to remove mutable state from Session but found that it's already been done in #114622. With that PR, what is written in the explanation section of this issue is obsolete: there is no mutable state in Session any more. I'm wondering now about further justifications for the change, not from a "should we halt this now" point of view, but from a "how to explain it" view.

directory for the incr comp session dir

In #83744 @bjorn3 writes:

[I]f 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.

general ordering concerns

With this change it's easier to determine the crate name earlier in the compilation, which might be useful for structuring the state. All one needs is to peek into the provided source file, with no other state needed. Obtaining the crate name and crate type are very dependency-free operations now.

@bjorn3
Copy link
Member Author

bjorn3 commented Oct 17, 2024

I think it would now be possible to revert #114622 in a way that doesn't re-introduce the global mutable state in TyCtxt. This would also allow

let crate_name = find_crate_name(sess, &[]);
let crate_types = collect_crate_types(sess, &[]);
to avoid recomputing the crate name and type. And the same for
let crate_name = sess
.opts
.crate_name
.clone()
.or_else(|| rustc_attr::find_crate_name(attrs).map(|n| n.to_string()));
(this function doesn't handle #![crate_type] correctly at all right now.) And
let t_outputs = rustc_interface::util::build_output_filenames(attrs, sess);
let id = rustc_session::output::find_crate_name(sess, attrs);
and
let id = rustc_session::output::find_crate_name(sess, attrs);
can also avoid recomputing the crate name.

@fmease
Copy link
Member

fmease commented Oct 17, 2024

This would also allow

let crate_name = find_crate_name(sess, &[]);
let crate_types = collect_crate_types(sess, &[]);
to avoid recomputing the crate name and type.

Incidentally my open PR #127581 does away with recomputing the crate name inside fs.rs.

@est31
Copy link
Member

est31 commented Oct 17, 2024

@bjorn3 @fmease thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-attributes Area: Attributes (`#[…]`, `#![…]`) A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. C-future-incompatibility Category: Future-incompatibility lints C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
Archived in project
Development

No branches or pull requests

8 participants