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

Add unused pre-interned symbols tidy check #110437

Closed
wants to merge 2 commits into from

Conversation

DaniPopes
Copy link
Contributor

@DaniPopes DaniPopes commented Apr 17, 2023

Adds a tidy check that checks for unused pre-interned symbols, inspired by the comment in rustc_span::symbol.

This checks for sym::* references, which results in 35 unused symbols, although of these are commonly used even if not explicitly referred to, like Future or CString, with very few being actually unused.

When adding new symbols to the pre-intern list, they must also be added to an ignore list to pass this check if they are not explicitly referenced as sym::* anywhere in the compiler.

I personally don't think it's worth having this extra maintenance burden, but I'm opening this PR anyway for others' opinions on the matter, and for future reference.

@rustbot
Copy link
Collaborator

rustbot commented Apr 17, 2023

r? @ozkanonur

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Apr 17, 2023
@Noratrieb
Copy link
Member

I agree that this tidy check is more effort than it's worth and we shouldn't add it.

@onur-ozkan
Copy link
Member

I personally don't think it's worth having this extra maintenance burden

I agree that this tidy check is more effort than it's worth and we shouldn't add it.

I agree also.

cc @rust-lang/bootstrap

@albertlarsan68
Copy link
Member

This feels like too much effort for something that seems (at a first glance) quite brittle.

@jyn514
Copy link
Member

jyn514 commented Apr 19, 2023

I think t-compiler should make the decision here. Tidy is ultimately for their benefit, I don't think we're in a good position to decide how useful this check is.

That said Nils is on t-compiler, so I'm comfortable closing this.

Separately, @DaniPopes are you interested in making a one time PR that removes the unused symbols with no other changes? That seems to me like a good cleanup and not a maintenance burden.

@jyn514 jyn514 closed this Apr 19, 2023
@onur-ozkan onur-ozkan removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-testsuite Area: The testsuite used to check the correctness of rustc T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants