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

Make deprecated_cfg_attr_crate_type_name a hard error #129670

Merged
merged 1 commit into from
Oct 6, 2024

Conversation

est31
Copy link
Member

@est31 est31 commented Aug 27, 2024

Turns the forward compatibility lint added by #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:

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

This code will continue working and is not deprecated:

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

The reasoning for this is explained in #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 #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 #91632 - tracking issue for the lint

@rustbot
Copy link
Collaborator

rustbot commented Aug 27, 2024

r? @cjgillot

rustbot has assigned @cjgillot.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Aug 27, 2024
@est31 est31 added the relnotes Marks issues that should be documented in the release notes of the next release. label Aug 27, 2024
@est31 est31 added the T-lang Relevant to the language team, which will review and decide on the PR/issue. label Aug 27, 2024
@bors
Copy link
Contributor

bors commented Aug 28, 2024

☔ The latest upstream changes (presumably #129665) made this pull request unmergeable. Please resolve the merge conflicts.

@cjgillot cjgillot added the I-lang-nominated Nominated for discussion during a lang team meeting. label Aug 28, 2024
@est31 est31 force-pushed the cfg_attr_crate_type_name_error branch from ccaaab0 to 23d40df Compare August 28, 2024 21:02
@cjgillot cjgillot added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 14, 2024
@traviscross traviscross removed the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Sep 18, 2024
@traviscross
Copy link
Contributor

traviscross commented Sep 18, 2024

@rfcbot fcp merge

We discussed this is the meeting today and were feeling positively toward it.

@rfcbot
Copy link

rfcbot commented Sep 18, 2024

Team member @traviscross has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns.
See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Sep 18, 2024
@rfcbot
Copy link

rfcbot commented Sep 18, 2024

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot rfcbot removed the proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. label Sep 18, 2024
@nikomatsakis
Copy link
Contributor

nikomatsakis commented Sep 18, 2024

@rfcbot reviewed

This seems like a good simplification, but we should notify crate authors that may be affected with at least a ping. I'd like to know if there is truly no way to support their use case.

@joshtriplett
Copy link
Member

joshtriplett commented Sep 18, 2024

Documenting some supporting rationale:

  1. We're assuming that --crate-type will be sufficient to migrate to.

  2. We expect that if the warned-about (and later errored-about) thing being allowed was an issue for anyone, the affected crates would have raised this upstream.

@nikomatsakis
Copy link
Contributor

nikomatsakis commented Sep 18, 2024

Based on a google code search, I want to give a headsup to

@bjorn3
Copy link
Member

bjorn3 commented Sep 18, 2024

The usage in wasmer seems to be entirely ineffective:

$ cargo +stable build --no-default-features --features "wasmer/js wasmer/std" -p wasmer --target wasm32-unknown-unknown
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.09s
$ ls target/wasm32-unknown-unknown/debug
build  deps  examples  incremental  libwasmer.d  libwasmer.rlib
$ ls target/wasm32-unknown-unknown/debug/deps | grep wasmer
libwasmer-aac61f984f72830e.rlib
libwasmer-aac61f984f72830e.rmeta
libwasmer_types-e9cfd7530a67cd97.rlib
libwasmer_types-e9cfd7530a67cd97.rmeta
wasmer-aac61f984f72830e.d
wasmer_types-e9cfd7530a67cd97.d

No .wasm file is produced that would be expected from a cdylib.

On the other hand even if I remove #![cfg_attr(feature = "js", crate_type = "cdylib")], cargo +stable rustc --no-default-features --features "wasmer/js wasmer/std" -p wasmer --target wasm32-unknown-unknown --crate-type cdylib produces a wasmer.wasm file. The usage of cargo rustc + --crate-type cdylib instead of cargo build is what causes the crate type to be changed to cdylib.

Deny,
"detects usage of `#![cfg_attr(..., crate_type/crate_name = \"...\")]`",
@future_incompatible = FutureIncompatibleInfo {
reason: FutureIncompatibilityReason::FutureReleaseErrorDontReportInDeps,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this has not gone through a "report in deps" stage. Fine for me, but I just wanted to call it out for awareness.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think with the low usage it has in the ecosystem, the step can be skipped.

@traviscross traviscross removed the I-lang-nominated Nominated for discussion during a lang team meeting. label Sep 25, 2024
@est31 est31 force-pushed the cfg_attr_crate_type_name_error branch from 23d40df to 5879827 Compare September 26, 2024 20:58
@rfcbot rfcbot added the finished-final-comment-period The final comment period is finished for this PR / Issue. label Sep 28, 2024
@rfcbot rfcbot removed the final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. label Sep 28, 2024
@rfcbot
Copy link

rfcbot commented Sep 28, 2024

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

@rfcbot rfcbot added the to-announce Announce this issue on triage meeting label Sep 28, 2024
@est31 est31 added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels Sep 30, 2024
@est31 est31 force-pushed the cfg_attr_crate_type_name_error branch from 5879827 to 00ed47b Compare October 5, 2024 02:30
@Urgau
Copy link
Member

Urgau commented Oct 6, 2024

LGTM. Thanks for pushing this forward.

r? @Urgau
@bors r+

@bors
Copy link
Contributor

bors commented Oct 6, 2024

📌 Commit 00ed47b has been approved by Urgau

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 6, 2024
@bors
Copy link
Contributor

bors commented Oct 6, 2024

⌛ Testing commit 00ed47b with merge 8422e27...

@bors
Copy link
Contributor

bors commented Oct 6, 2024

☀️ Test successful - checks-actions
Approved by: Urgau
Pushing 8422e27 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Oct 6, 2024
@bors bors merged commit 8422e27 into rust-lang:master Oct 6, 2024
7 checks passed
@rustbot rustbot added this to the 1.83.0 milestone Oct 6, 2024
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (8422e27): comparison URL.

Overall result: ❌ regressions - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
0.3% [0.3%, 0.3%] 2
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) - - 0

Max RSS (memory usage)

Results (primary 2.4%, secondary -0.4%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
4.3% [2.3%, 6.4%] 2
Regressions ❌
(secondary)
2.2% [2.2%, 2.2%] 1
Improvements ✅
(primary)
-1.5% [-1.5%, -1.5%] 1
Improvements ✅
(secondary)
-1.2% [-1.8%, -0.9%] 3
All ❌✅ (primary) 2.4% [-1.5%, 6.4%] 3

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 773.691s -> 774.004s (0.04%)
Artifact size: 329.52 MiB -> 329.48 MiB (-0.01%)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. merged-by-bors This PR was explicitly merged by bors. relnotes Marks issues that should be documented in the release notes of the next release. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-lang Relevant to the language team, which will review and decide on the PR/issue. to-announce Announce this issue on triage meeting
Projects
None yet
Development

Successfully merging this pull request may close these issues.