- 
                Notifications
    You must be signed in to change notification settings 
- Fork 13.9k
          Use Wunused-crate-dependencies for the compiler
          #137930
        
          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
  
    Use Wunused-crate-dependencies for the compiler
  
  #137930
              Conversation
      
        
              This comment has been minimized.
        
        
      
    
  This comment has been minimized.
9c4840f    to
    fe01d30      
    Compare
  
    
      
        
              This comment has been minimized.
        
        
      
    
  This comment has been minimized.
0ede122    to
    d73f6e5      
    Compare
  
    | ☔ The latest upstream changes (presumably #138267) made this pull request unmergeable. Please resolve the merge conflicts. | 
d73f6e5    to
    8650d11      
    Compare
  
    | ☔ The latest upstream changes (presumably #138310) made this pull request unmergeable. Please resolve the merge conflicts. | 
8650d11    to
    2e213f8      
    Compare
  
    | Could not assign reviewer from:  | 
| Some changes occurred in compiler/rustc_codegen_ssa Some changes occurred in exhaustiveness checking cc @Nadrieril These commits modify the  If this was unintentional then you should revert the changes before this PR is merged. Some changes occurred in compiler/rustc_attr_parsing Some changes occurred in compiler/rustc_sanitizers cc @rust-lang/project-exploit-mitigations, @rcvalle | 
| rust-lang/compiler-team#844 has hit 10 days after seconding without objection, so is approved. And we have established  @bors rollup | 
| #[cfg(test)] | ||
| mod tests; | ||
|  | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why this change and why merge all the modules into a single file? Please at least leave the files separated like they were, and overall I'd prefer if this crate was structured like a normal crate, i.e. with usage tests in tests/.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I saw the commit description, I don't agree that "the rest of the compiler does it" is a good reason: this is a librarified crate, meant to be used outside of the compiler and compiled on its own (on stable even). In an ideal future this crate would be published to crates.io. These tests are meant to play with its public API surface, hence why they were integration tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like having a "dual status" crate like this. I think it should either be a compiler crate and treated like other compiler crates, or be on crates.io (like the ena crate, for example) and then it can do things differently.
Nonetheless, the third commit it not integral and can be removed. Some level of false positive workaround is necessary for the first commit, such as the #![allow(unused_crate_dependencies)] attributes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It still evolves sufficiently in sync with the compiler that moving out-of-tree wouldn't make sense. I expect other librarified parts of the compiler to behave similarly, how do they behave? The only other one I know of is rustc_type_ir tho.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rustc_transmute also has a rustc feature.
rustc_abi, rustc_next_trait_solver, rustc_index, rustc_ast_ir, rustc_transmute, rustc_type_ir all have a nightly feature.
I don't have a sense of whether rustc and nightly are doing the same thing. I wonder if some of these crates could be split into the "base" part (typically shared with rust-analyzer) and the rustc-specific part.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rustc_pattern_analysis could easily be split in that way. Looking at these other crates, I see:
- actual nightly features (e.g. Rc::new_zeroedorExtend::extend_one);
- implementing Encodable/Decodable;
- whole submodules cfged-out;
- internal features like rustc_pass_by_valueorrustc_lint_query_instability(only inrustc_type_ir).
My guess is that we could split most of them if we had the orphan rules escape hatch that RfL wants.
| Commit 1 LGTM. I'll hand the review over to @Nadrieril who has a stronger preference on the test organizations. r? @Nadrieril | 
| ☔ The latest upstream changes (presumably #138464) made this pull request unmergeable. Please resolve the merge conflicts. | 
1fa77ca    to
    f84fd39      
    Compare
  
    | I rebased again, and again removed a couple of new unnecessary dependencies that had crept in. @bors r=jieyouxu,Nadrieril p=1 Bumping priority because this is conflict-prone. | 
| @bors rollup=never | 
…encies, r=jieyouxu,Nadrieril Use `Wunused-crate-dependencies` for the compiler An implementation of rust-lang/compiler-team#844. r? `@jieyouxu`
      
        
              This comment has been minimized.
        
        
      
    
  This comment has been minimized.
| 💔 Test failed - checks-actions | 
It's very useful. There are some false positives involving integration tests in `rustc_pattern_analysis` and `rustc_serialize`. There is also a false positive involving `rustc_driver_impl`'s `rustc_randomized_layouts` feature. And I removed a `rustc_span` mention in a doc comment in `rustc_log` because it wasn't integral to the comment but caused a dev-dependency.
Because (a) the vast majority of compiler tests are unit tests, and (b) this works better with `unused_crate_dependencies`.
f84fd39    to
    e2320b3      
    Compare
  
    | @bors r=jieyouxu,Nadrieril p=1 | 
| @bors p=10 (or else this will get rollup-raced) | 
| ☀️ Test successful - checks-actions | 
| What is this?This is an experimental post-merge analysis report that shows differences in test outcomes between the merged PR and its parent PR.Comparing 70237a8 (parent) -> 4e2b096 (this PR) Test differencesShow 68 test diffs
 Additionally, 2 doctest diffs were found. These are ignored, as they are noisy. Job group index 
 | 
| Finished benchmarking commit (4e2b096): comparison URL. Overall result: no relevant changes - no action needed@rustbot label: -perf-regression Instruction countThis benchmark run did not return any relevant results for this metric. Max RSS (memory usage)This benchmark run did not return any relevant results for this metric. CyclesResults (primary -3.0%)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: 773.52s -> 773.468s (-0.01%) | 
An implementation of rust-lang/compiler-team#844.
r? @jieyouxu