Skip to content

Replace visibility test with reachability test in dead code detection#119552

Merged
bors merged 9 commits intorust-lang:masterfrom
krtab:dead_code_priv_mod_pub_field
Mar 23, 2024
Merged

Replace visibility test with reachability test in dead code detection#119552
bors merged 9 commits intorust-lang:masterfrom
krtab:dead_code_priv_mod_pub_field

Conversation

@krtab
Copy link
Contributor

@krtab krtab commented Jan 3, 2024

Fixes #119545

Also included is a fix for an error now flagged by the lint

@rustbot
Copy link
Collaborator

rustbot commented Jan 3, 2024

r? @cjgillot

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

@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. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Jan 3, 2024
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@krtab
Copy link
Contributor Author

krtab commented Jan 4, 2024

Hm, my bad it took me some time to figure out why things were compiling on my side but not in the CI. I'll fix all the previously undetected dead code and you can review then.

@rustbot author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 4, 2024
@krtab krtab force-pushed the dead_code_priv_mod_pub_field branch from 3e34b88 to e6d8c78 Compare January 4, 2024 15:54
@rust-log-analyzer

This comment has been minimized.

@krtab krtab force-pushed the dead_code_priv_mod_pub_field branch from e6d8c78 to 4521396 Compare January 4, 2024 16:34
@rustbot
Copy link
Collaborator

rustbot commented Jan 4, 2024

Some changes occurred in compiler/rustc_codegen_gcc

cc @antoyo, @GuillaumeGomez

@krtab krtab force-pushed the dead_code_priv_mod_pub_field branch from 4521396 to 549f08f Compare January 4, 2024 16:49
@rust-log-analyzer

This comment has been minimized.

@krtab krtab force-pushed the dead_code_priv_mod_pub_field branch from 549f08f to 73be48a Compare January 5, 2024 15:25
@rustbot
Copy link
Collaborator

rustbot commented Jan 5, 2024

The Miri subtree was changed

cc @rust-lang/miri

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

@rust-log-analyzer

This comment has been minimized.

Copy link
Member

@flip1995 flip1995 left a comment

Choose a reason for hiding this comment

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

Commit fb75654 is kinda a 🥷 fix for Clippy and doesn't really belong in this PR. I definitely appreciate the fix, but would prefer to have it in a Clippy PR. What's the motivation in including it in this PR?

@krtab
Copy link
Contributor Author

krtab commented Jan 5, 2024

Commit fb75654 is kinda a 🥷 fix for Clippy and doesn't really belong in this PR. I definitely appreciate the fix, but would prefer to have it in a Clippy PR. What's the motivation in including it in this PR?

I hesitated and discussed with other contributors to Rust. My issue with having it in a clippy specific PR is that I don't have a good solution to pass the CI in this one. I can delete or mark as allow(dead_code) the unused fields here and make the more substantial fix in clippy itself, but then this requires a correct ordering between this PR merge and the update of the in-tree clippy.
Having the fix here avoids having to PR to synchronize.

@flip1995
Copy link
Member

flip1995 commented Jan 5, 2024

Ah, was the self.allow_one_hash_in_raw_strings field unused? Well then, let's keep it in this PR. Thanks for the explanation!

@krtab krtab force-pushed the dead_code_priv_mod_pub_field branch from 73be48a to 5be1860 Compare January 5, 2024 17:17
@rustbot
Copy link
Collaborator

rustbot commented Jan 5, 2024

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

@rustbot rustbot added the O-unix Operating system: Unix-like label Jan 5, 2024
@rust-log-analyzer

This comment has been minimized.

@krtab
Copy link
Contributor Author

krtab commented Jan 8, 2024

Well it seems to finally pass the CI! Didn't expect it to end-up being a +120/-51 PR 😅
I expect that there will be breakage during bors try.

Anyway @rustbot review

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

krtab commented Mar 22, 2024

@rustbot author

@rustbot rustbot removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 22, 2024
@Noratrieb
Copy link
Member

the failure may be spurious
@bors retry

@Noratrieb
Copy link
Member

oh wait no you modified CI
@bors r
if you revert that I can retry

@Noratrieb
Copy link
Member

@bors r-

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@saethlin
Copy link
Member

saethlin commented Mar 22, 2024

Suggestion (or argue that my suggestion is a bad idea), then I'll re-apply approval.

@saethlin
Copy link
Member

@bors r=cjgillot,saethlin

@bors
Copy link
Collaborator

bors commented Mar 22, 2024

📌 Commit 843e79f has been approved by cjgillot,saethlin

It is now in the queue for this repository.

@bors
Copy link
Collaborator

bors commented Mar 22, 2024

⌛ Testing commit 843e79f with merge 1cb1dca...

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Collaborator

bors commented Mar 22, 2024

💔 Test failed - checks-actions

@krtab
Copy link
Contributor Author

krtab commented Mar 22, 2024

@rustbot author

I need to find a way to compile src/bootstrap locally with the stage1 compiler to get rid of all errors befors re pushing.

The dead_code lint was previously eroneously missing this dead code.
Since this lint bug has been fixed, the unused field need to be removed
or marked as `#[allow(dead_code)]`.

Given that this struct is deserialized without #[serde(deny_unknown_fields)]
it is ok to simply delete the never read fields.
@krtab
Copy link
Contributor Author

krtab commented Mar 22, 2024

@rustbot review

@saethlin
Copy link
Member

@bors r=cjgillot,saethlin

@bors
Copy link
Collaborator

bors commented Mar 22, 2024

📌 Commit 7342cc4 has been approved by cjgillot,saethlin

It is now in the queue for this repository.

@workingjubilee
Copy link
Member

higher!
@bors p=2

@bors
Copy link
Collaborator

bors commented Mar 23, 2024

⌛ Testing commit 7342cc4 with merge c308726...

@bors
Copy link
Collaborator

bors commented Mar 23, 2024

☀️ Test successful - checks-actions
Approved by: cjgillot,saethlin
Pushing c308726 to master...

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (c308726): comparison URL.

Overall result: ❌ regressions - ACTION NEEDED

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

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)
2.1% [1.0%, 3.2%] 4
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 2.1% [1.0%, 3.2%] 4

Max RSS (memory usage)

Results

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)
2.8% [2.1%, 3.7%] 4
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 2.8% [2.1%, 3.7%] 4

Cycles

Results

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)
2.6% [1.4%, 3.7%] 3
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 2.6% [1.4%, 3.7%] 3

Binary size

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

Bootstrap: 671.121s -> 671.167s (0.01%)
Artifact size: 315.04 MiB -> 315.03 MiB (-0.00%)

@krtab
Copy link
Contributor Author

krtab commented Mar 23, 2024

Thanks everyone, especially @cjgillot and @saethlin :)

@Kobzol
Copy link
Member

Kobzol commented Mar 23, 2024

More warnings are now emitted for ripgrep (analyzed here).

@rustbot label: +perf-regression-triaged

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 merged-by-bors This PR was explicitly merged by bors. O-SGX Target: SGX O-unix Operating system: Unix-like O-wasi Operating system: Wasi, Webassembly System Interface perf-regression Performance regression. perf-regression-triaged The performance regression has been triaged. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. 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. T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

No warning for unread pub struct field in private module