-
Notifications
You must be signed in to change notification settings - Fork 13.4k
Refactor the two-phase check for impls and impl items #141407
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
Conversation
b19a615
to
faab5c6
Compare
@rustbot ready |
CI looks broken |
@rustbot ready |
This comment has been minimized.
This comment has been minimized.
Thanks, this looks better now. |
Reminder, once the PR becomes ready for a review, use |
1cf061b
to
e50396e
Compare
This comment has been minimized.
This comment has been minimized.
@rustbot ready |
e50396e
to
f83ecd8
Compare
This comment has been minimized.
This comment has been minimized.
☀️ Try build successful - checks-actions |
@bors r=petrochenkov |
@petrochenkov update with tests |
@bors r+ |
Rollup of 8 pull requests Successful merges: - #133823 (Use `cfg_attr_trace` in AST with a placeholder attribute for accurate suggestion) - #141004 (Report text_direction_codepoint_in_literal when parsing) - #141407 (Refactor the two-phase check for impls and impl items) - #141430 (remove `visit_clobber` and move `DummyAstNode` to `rustc_expand`) - #141507 (atomic_load intrinsic: use const generic parameter for ordering) - #141538 (implement `va_arg` for x86_64 systemv) - #141669 (float: Replace some approximate assertions with exact) - #141747 (rustdoc: display doc(cfg(false)) properly) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of #141407 - mu001999-contrib:dead-code/refactor, r=petrochenkov Refactor the two-phase check for impls and impl items Refactor the two-phase dead code check to make the logic clearer and simpler: 1. adding assoc fn and impl into `unsolved_items` directly during the initial construction of the worklist 2. converge the logic of checking whether assoc fn and impl are used to `item_should_be_checked`, and the item is considered used only when its corresponding trait and Self adt are used This PR only refactors as much as possible to avoid affecting the original functions. However, due to the adjustment of the order of checks, the test results are slightly different, but overall, there is no regression problem Fixes #127911 Fixes #128839 Extracted from #128637. r? petrochenkov try-job: dist-aarch64-linux
@rust-timer build 1d94b88 (trying perf. due to #141753) |
This comment has been minimized.
This comment has been minimized.
Finished benchmarking commit (1d94b88): comparison URL. Overall result: ❌ regressions - please read the text belowBenchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf. Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @bors rollup=never Instruction countThis is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.
Max RSS (memory usage)Results (primary 2.4%, secondary 0.6%)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.
CyclesResults (primary 1.1%, secondary 0.3%)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.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: missing data |
This seems to have caused most of the regressions in #141753. |
Oh, this might be expected. While this PR does more, I'm not sure if the performance impact is within normal range. |
Rollup of 8 pull requests Successful merges: - rust-lang/rust#133823 (Use `cfg_attr_trace` in AST with a placeholder attribute for accurate suggestion) - rust-lang/rust#141004 (Report text_direction_codepoint_in_literal when parsing) - rust-lang/rust#141407 (Refactor the two-phase check for impls and impl items) - rust-lang/rust#141430 (remove `visit_clobber` and move `DummyAstNode` to `rustc_expand`) - rust-lang/rust#141507 (atomic_load intrinsic: use const generic parameter for ordering) - rust-lang/rust#141538 (implement `va_arg` for x86_64 systemv) - rust-lang/rust#141669 (float: Replace some approximate assertions with exact) - rust-lang/rust#141747 (rustdoc: display doc(cfg(false)) properly) r? `@ghost` `@rustbot` modify labels: rollup
It's nothing terrible, if it was expected, and this fixes some issues, I think it's fine (unless you have ideas on how to fix the perf. :) ) |
Bisects to rust-lang/rust#141407. warning: struct `Struct` is never constructed --> test_suite/tests/test_gen.rs:803:16 | 803 | pub struct Struct { | ^^^^^^ | = note: `#[warn(dead_code)]` on by default warning: function `vec_first_element` is never used --> test_suite/tests/test_gen.rs:885:4 | 885 | fn vec_first_element<T, S>(vec: &[T], serializer: S) -> StdResult<S::Ok, S::Error> | ^^^^^^^^^^^^^^^^^ warning: struct `S` is never constructed --> test_suite/tests/regression/issue2415.rs:5:12 | 5 | pub struct S; | ^ | = note: `#[warn(dead_code)]` on by default
Bisects to rust-lang/rust#141407. warning: function `unindent` is never used --> src/unindent.rs:3:8 | 3 | pub fn unindent(s: &str) -> String { | ^^^^^^^^ | = note: `#[warn(dead_code)]` on by default warning: function `unindent_bytes` is never used --> src/unindent.rs:10:8 | 10 | pub fn unindent_bytes(s: &[u8]) -> Vec<u8> { | ^^^^^^^^^^^^^^ warning: trait `Unindent` is never used --> src/unindent.rs:53:11 | 53 | pub trait Unindent { | ^^^^^^^^
Question for this: In Wasmtime I've bisected a new warning on nightly to this PR where a new dead code warning is firing for a method. The problem is that when removing the method the crate no longer compiles so I'm wondering if it might be a false positive. I've used Unfortunately the |
@alexcrichton What it essentially does is simply propagate the unused status to the corresponding impl and impl items by checking whether traits and trait items, as well as ADTs, have been used. Considering your command has features option, I guess this method might be feature-dependent? For example, it might only be unused under certain features, in other case directly deleting it would be problematic—you'd need to use cfg conditional compilation for this method instead. You could first verify whether this dead code appears across all features. |
Alas while it may be simple for you I fear I don't understand the ramifications of such a change, so in the meantime I dug in a bit more with In any case, no, this is not feature dependent, but additionally I don't think there's anything that needs to change. |
@alexcrichton I will have a look |
Refactor the two-phase dead code check to make the logic clearer and simpler:
unsolved_items
directly during the initial construction of the worklistitem_should_be_checked
, and the item is considered used only when its corresponding trait and Self adt are usedThis PR only refactors as much as possible to avoid affecting the original functions. However, due to the adjustment of the order of checks, the test results are slightly different, but overall, there is no regression problem
Fixes #127911
Fixes #128839
Extracted from #128637.
r? petrochenkov
try-job: dist-aarch64-linux