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.

Sign up for GitHub

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

Jump to bottom

Detect unused struct impls pub trait #121752

Merged
merged 2 commits into from Mar 11, 2024
Merged

Detect unused struct impls pub trait #121752

merged 2 commits into from Mar 11, 2024

Conversation

mu001999
Copy link
Contributor

Fixes #47851

@rustbot
Copy link
Collaborator

rustbot commented Feb 28, 2024

r? @wesleywiser

rustbot has assigned @wesleywiser.
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. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Feb 28, 2024
@mu001999
Copy link
Contributor Author

r? @shepmaster

@rustbot rustbot assigned shepmaster and unassigned wesleywiser Feb 28, 2024
@mu001999 mu001999 mentioned this pull request Feb 28, 2024
Merged
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@shepmaster
Copy link
Member

I'll give this a look-through, but I'm definitely not qualified to give it a final sign-off 🙂.

r? rust-lang/compiler

@rustbot rustbot assigned pnkfelix and unassigned shepmaster Feb 28, 2024
@mu001999 mu001999 changed the title Detect unused struct impls pub trait WIP: Detect unused struct impls pub trait Feb 29, 2024
@mu001999 mu001999 mentioned this pull request Feb 29, 2024
Merged
@rust-log-analyzer

This comment has been minimized.

Sign in to view
jhpratt added a commit to jhpratt/rust that referenced this pull request Feb 29, 2024
@jhpratt
Rollup merge of rust-lang#121779 - mu001999:clean, r=Nilstrieb
f8b3fbc 
Remove unused diagnostic struct

Detected by rust-lang#121752
jhpratt added a commit to jhpratt/rust that referenced this pull request Feb 29, 2024
@jhpratt
Rollup merge of rust-lang#121779 - mu001999:clean, r=Nilstrieb
698cec8 
Remove unused diagnostic struct

Detected by rust-lang#121752
@rustbot
Copy link
Collaborator

rustbot commented Feb 29, 2024

Some changes occurred in compiler/rustc_codegen_gcc

cc @antoyo, @GuillaumeGomez

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rustbot
Copy link
Collaborator

rustbot commented Feb 29, 2024

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Feb 29, 2024
@rust-timer
Unrolled build for rust-lang#121779
092558c 
Rollup merge of rust-lang#121779 - mu001999:clean, r=Nilstrieb

Remove unused diagnostic struct

Detected by rust-lang#121752
@mu001999 mu001999 changed the title WIP: Detect unused struct impls pub trait Detect unused struct impls pub trait Feb 29, 2024
@mu001999 mu001999 mentioned this pull request Feb 29, 2024
Closed
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bors
Copy link
Contributor

bors commented Mar 8, 2024

💔 Test failed - checks-actions

@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 8, 2024
@mu001999 mu001999 mentioned this pull request Mar 9, 2024
Merged
@mu001999
Copy link
Contributor Author

mu001999 commented Mar 9, 2024 •
edited

New pending PR:

bors added a commit to rust-lang/rust-analyzer that referenced this pull request Mar 9, 2024
@bors
Auto merge of #16795 - mu001999:master, r=Veykril
8f08bbe 
remove unused struct Snap in lsif

Detected by #121752, see rust-lang/rust#121752 (comment)
@bors
Copy link
Contributor

bors commented Mar 10, 2024

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

@mu001999
Copy link
Contributor Author

@pnkfelix @shepmaster No pending PRs, could you reapprove? Thanks ;)

@shepmaster
Copy link
Member

@bors r=pnkfelix

@bors
Copy link
Contributor

bors commented Mar 11, 2024

📌 Commit c73a7f0 has been approved by pnkfelix

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 Mar 11, 2024
@bors
Copy link
Contributor

bors commented Mar 11, 2024

⌛ Testing commit c73a7f0 with merge c69fda7...

@bors
Copy link
Contributor

bors commented Mar 11, 2024

☀️ Test successful - checks-actions
Approved by: pnkfelix
Pushing c69fda7 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Mar 11, 2024
@bors bors merged commit c69fda7 into rust-lang:master Mar 11, 2024
12 checks passed
@rustbot rustbot added this to the 1.78.0 milestone Mar 11, 2024
@mu001999 mu001999 deleted the dead_code/improve branch March 11, 2024 05:02
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (c69fda7): 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)		 0.4% [0.2%, 0.5%] 6
Regressions ❌
(secondary)		 - - 0
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 - - 0
All ❌✅ (primary) 0.4% [0.2%, 0.5%] 6

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)		 1.9% [1.9%, 1.9%] 1
Regressions ❌
(secondary)		 - - 0
Improvements ✅
(primary)		 -2.0% [-2.4%, -1.1%] 10
Improvements ✅
(secondary)		 -2.8% [-4.0%, -2.3%] 13
All ❌✅ (primary) -1.6% [-2.4%, 1.9%] 11

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: 648.469s -> 647.708s (-0.12%)
Artifact size: 309.94 MiB -> 309.95 MiB (0.00%)

@rustbot rustbot added the perf-regression Performance regression. label Mar 11, 2024
This was referenced Mar 12, 2024
Merged
Merged
@mu001999 mu001999 mentioned this pull request Mar 12, 2024
Open
@pnkfelix
Copy link
Member

pnkfelix commented Mar 12, 2024 •
edited

Visiting for weekly rustc-perf triage.

@mu001999 it looks like the performance regression flagged here is legitimate, even though it is also minor. I.e., the performance stats indicate that the newly added worklist code is the source of a bit of extra time we are spending here.

Do you have any ideas on ways we might attempt to avoid the extra cost?

If not, that is okay; we just need to keep our eyes on how these things add up, and make sure we don't end up slowing down the compiler too much for lints that may not be paying for themselves.

@mu001999
Copy link
Contributor Author

Do you have any ideas on ways we might attempt to avoid the extra cost?

@pnkfelix maybe we can make unsolved impl items as a map from adt def id to impl item ids, and update next-to-solve impl terms if finding a used adt when visiting trait items. Then we don't need traversal all unsolved impl items every time until there are no unsolved items. But I don't know if this will reduce the extra time.

@pnkfelix
Copy link
Member

Yeah that sounds like a potential idea. Its not something you need to take on yourself, I just want to try to get the ideas written down while the code is still fresh in our heads.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@clubby789 clubby789 clubby789 left review comments

Assignees

@pnkfelix pnkfelix

Labels
merged-by-bors This PR was explicitly merged by bors. perf-regression Performance regression. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. 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.
Projects
None yet
Milestone
1.78.0
Development

Successfully merging this pull request may close these issues.

impls should not inhibit the unused_item lint