Lint unused pub items in binary crates

[mu001999]

View all comments

This PR adds a new unstable flag -Ztreat-pub-as-pub-crate as @Kobzol suggested.
When compiling binary crates with this flag, the seed worklist will only contain the entry fn and won't contain other reachable items. Then we can do the dead code analysis for pub items just like they are pub(crate).

Related zulip thread #general > pub/pub(crate) within a binary is a footgun.

---

Updated:

Adds a new lint unused_pub_items_in_binary (crate-level, default allow for now) instead of the previous unstable flag to lint unused pub items for binary crates.

See more in #149509 (comment).

[rustbot]

r? @jdonszelmann

rustbot has assigned @jdonszelmann.
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

[Noratrieb]

What are the plans for this flag? Being made the default? As just a flag, it's essentially useless, because no one will know about it. So I'd be opposed to just adding it without any plan for making it useful for everyone.

[mu001999]

Maybe this could be a separate lint, I haven't thought too clearly yet

[jdonszelmann]

Yea I do agree with nora here, maybe open a (specific) zulip thread to make a proper plan for this?

[jdonszelmann]

I guess I'd prefer this as a lint (attribute at the crate root) myself

[jdonszelmann]

@rustbot author

[rustbot]

Reminder, once the PR becomes ready for a review, use @rustbot ready.

[Kobzol]

I guess that the use-case that I'm envisioning is to have an opt-in way to treat pub stuff as pub(crate) in binary crates. If it was an allow-by-default lint, which could be manually denied, that would serve that use-case, I think.

[GrantGryczan]

Why would the end goal be for this to be opt-in? I and others want to see #74970 become a default. I wouldn't opt into this, because that would make my codebase less idiomatic, diverging from how everyone else who hasn't opted in uses pub.

The reason this is useful as a default is because it doesn't semantically make sense to distinguish between pub and pub(crate) in binary crates. This creates a footgun because, for example, pub items can't receive dead_code warnings. If it's just opt-in, then that doesn't remove the footgun for anyone who hasn't opted in, and I don't see a benefit to pub and pub(crate) being distinct in binary crates.

The only reason I can see that someone might not want to opt into this is that one might use pub to prevent dead_code warnings as opposed to pub(crate). But I think this intent might be better communicated using #[expect(dead_code)] rather than the implicit behavior of pub. Dead code detection is useful as a default. (The only downside to that is that you then have to remove the #[expect(dead_code)] if you later use it, but I'll leave it to others to decide if that coupling is an important consideration.)

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[mu001999]

@rustbot label -S-blocked +S-waiting-on-t-lang

[traviscross]

Suggested change

	pub UNUSED_PUB_ITEMS_IN_BINARY,
	pub UNUSED_PUB_IN_BINARY,

In keeping with the RFC 0344 guidance, I might call this unused_pub_in_binary.

• Lints that apply to arbitrary items (like the stability lints) should just mention what they check for: use deprecated rather than deprecated_items. This keeps lint names short. (Again, think "allow lint-name items".)

View changes since the review

[steffahn]

Assuming this is something like “dead_code but marking pub doesn’t count as usage”, I wonder if the name should be related to the name of the dead_code lint, like… public_dead_code_in_binary or something 🤔

[GrantGryczan]

The name "unused pub" makes me think the pub keyword is unused, not the item it's on.

[traviscross]

We talked about this in the lang meeting today. Let's do this under the name unused_pub_in_binary.

@rfcbot fcp merge lang

[rust-rfcbot]

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

• @joshtriplett
• @nikomatsakis
• @scottmcm
• @tmandry
• @traviscross

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.

[joshtriplett]

@rfcbot reviewed

Looking forward to this, thank you @mu001999!

[nikomatsakis]

@rfcbot reviewed

[rust-rfcbot]

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

[tmandry]

Can you add a test to verify that this doesn't activate on #[no_mangle] items? There are use cases for an a public #[no_mangle] function being called from a library, and the existing dead code lint doesn't activate on these functions.

[joshtriplett]

Also, @mu001999, could you please confirm that this is in the unused group so that allow(unused) disables it?

[mu001999]

Can you add a test to verify that this doesn't activate on #[no_mangle] items? There are use cases for an a public #[no_mangle] function being called from a library, and the existing dead code lint doesn't activate on these functions.

@tmandry Sure, I will add tests for such cases. Thanks for the reminder :)

[mu001999]

Also, @mu001999, could you please confirm that this is in the unused group so that allow(unused) disables it?

@joshtriplett Nop, this lint is not in the unused group. This is crate_level_only for now, so this will be conflict with unused. And the default level is Allow right now.

If we add this into the unused group, we need to make it not crate_level_only at least. This change will require more work.

[GrantGryczan]

If this isn't warn by default, my and Noratrieb's prior concerns remain unaddressed.

[mu001999]

If this isn't warn by default, my and Noratrieb's prior concerns remain unaddressed.

Yes, but I hope we can add the implementation of this lint firstly. And then someone could use it, I believe this has been better than the unstable flag.

Making it warn-by-default will lead to a lot of noise for this PR (like bless many tests). So I'd like to make it warn-by-default in a separate PR in the future.