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

Prefer suggestion paths which are not doc-hidden #87349

Conversation

In-line
Copy link
Contributor

@In-line In-line commented Jul 21, 2021

There are cases (as clearly shown in test) when compiler will prefer doc-hidden public reexport of item which is available at alternative path.

extern crate serde;

fn main() {
    let x: Option<i32> = 1i32; // This suggests `serde::__private::Some(1i32)`, but should suggest `Some(1i32)`
}

@rust-highfive
Copy link
Collaborator

r? @Mark-Simulacrum

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 21, 2021
@In-line In-line force-pushed the dont-suggest-doc-hidden-variant-for-enum branch from e038dfe to a17c505 Compare July 21, 2021 14:26
@In-line
Copy link
Contributor Author

In-line commented Jul 21, 2021

@Mark-Simulacrum can you or someone else familiar with the code provide mentorship for this issue?

I tried to remove deduplication from rustc_typeck/src/check/demand.rs, but I'm stuck.

@rust-log-analyzer

This comment has been minimized.

@In-line In-line marked this pull request as ready for review July 21, 2021 15:15
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@estebank
Copy link
Contributor

@bors try @rustc-timer queue

@bors
Copy link
Contributor

bors commented Jul 22, 2021

⌛ Trying commit 119967446c2495d8e97030f6f474e3b304def84b with merge a9460823440b911a0920daa5a4e9a348a3e4ae5b...

@estebank
Copy link
Contributor

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jul 22, 2021
@bors
Copy link
Contributor

bors commented Jul 22, 2021

⌛ Trying commit 119967446c2495d8e97030f6f474e3b304def84b with merge e1a100769c9c135c86aee71f4d267bc348b2ba0f...

@bors
Copy link
Contributor

bors commented Jul 22, 2021

☀️ Try build successful - checks-actions
Build commit: e1a100769c9c135c86aee71f4d267bc348b2ba0f (e1a100769c9c135c86aee71f4d267bc348b2ba0f)

@rust-timer
Copy link
Collaborator

Queued e1a100769c9c135c86aee71f4d267bc348b2ba0f with parent 1158367, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (e1a100769c9c135c86aee71f4d267bc348b2ba0f): comparison url.

Summary: This change led to significant regressions 😿 in compiler performance.

  • Very large regression in instruction counts (up to 80.3% on incr-full builds of await-call-tree-debug)

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Benchmarking 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 led to changes in compiler perf.

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 fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf +perf-regression

@rustbot rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Jul 22, 2021
@In-line
Copy link
Contributor Author

In-line commented Jul 23, 2021

Quite interesting results, but they are not suprising because we actually do more work (calling tcx.item_attrs) query. I tried to mitigate some of the constant factor with switching to Vec and reusing allocation and converting is_doc_hidden to query.

@rust-log-analyzer

This comment has been minimized.

@In-line In-line force-pushed the dont-suggest-doc-hidden-variant-for-enum branch from 1c47447 to 7a60b9b Compare July 23, 2021 10:27
@In-line
Copy link
Contributor Author

In-line commented Jul 26, 2021

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Insufficient permissions to issue commands to rust-timer.

@jyn514
Copy link
Member

jyn514 commented Dec 20, 2021

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Dec 20, 2021
@bors
Copy link
Contributor

bors commented Dec 20, 2021

⌛ Trying commit 98c367d7507292c00a8d4f2607cebcada4dddfcd with merge daed0396086cc24257dfa4b20bb5893331da788d...

@bors
Copy link
Contributor

bors commented Dec 20, 2021

☀️ Try build successful - checks-actions
Build commit: daed0396086cc24257dfa4b20bb5893331da788d (daed0396086cc24257dfa4b20bb5893331da788d)

@rust-timer
Copy link
Collaborator

Queued daed0396086cc24257dfa4b20bb5893331da788d with parent e95e084, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (daed0396086cc24257dfa4b20bb5893331da788d): comparison url.

Summary: This change led to small relevant regressions 😿 in compiler performance.

  • Small regression in instruction counts (up to 0.4% on incr-unchanged builds of unify-linearly)

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Benchmarking 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 led to changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf +perf-regression

@In-line In-line force-pushed the dont-suggest-doc-hidden-variant-for-enum branch from 98c367d to 875c9c9 Compare December 21, 2021 22:04
@rust-log-analyzer
Copy link
Collaborator

The job x86_64-gnu-llvm-12 failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

12    = help: items from traits can only be used if the trait is in scope
13 help: the following trait is implemented but not in scope; perhaps add a `use` for it:
14    |
- LL | use unnamed_pub_trait_source::prelude::*; // trait Tr
+ LL | use unnamed_pub_trait_source::m::Tr;
17 
Some tests failed in compiletest suite=ui mode=ui host=x86_64-unknown-linux-gnu target=x86_64-unknown-linux-gnu
18 error: aborting due to previous error

---
To only update this specific test, also pass `--test-args imports/unnamed_pub_trait.rs`

error: 1 errors occurred comparing output.
status: exit status: 1
command: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "/checkout/src/test/ui/imports/unnamed_pub_trait.rs" "-Zthreads=1" "--target=x86_64-unknown-linux-gnu" "--error-format" "json" "--json" "future-incompat" "-Ccodegen-units=1" "-Zui-testing" "-Zdeduplicate-diagnostics=no" "--emit" "metadata" "-C" "prefer-dynamic" "--out-dir" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/imports/unnamed_pub_trait" "-A" "unused" "-Crpath" "-O" "-Cdebuginfo=0" "-Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/imports/unnamed_pub_trait/auxiliary"
------------------------------------------

------------------------------------------
stderr:
stderr:
------------------------------------------
error[E0599]: no method named `method` found for struct `S` in the current scope
   |
   |
LL |     S.method();
   |       ^^^^^^ method not found in `S`
  ::: /checkout/src/test/ui/imports/auxiliary/unnamed_pub_trait_source.rs:7:23
   |
   |
LL |     pub trait Tr { fn method(&self); }
   |                       ------ the method is available for `S` here
   = help: items from traits can only be used if the trait is in scope
help: the following trait is implemented but not in scope; perhaps add a `use` for it:
   |
   |
LL | use unnamed_pub_trait_source::m::Tr;

error: aborting due to previous error

For more information about this error, try `rustc --explain E0599`.
---
test result: FAILED. 12395 passed; 1 failed; 119 ignored; 0 measured; 0 filtered out; finished in 165.04s



command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/compiletest" "--compile-lib-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib" "--run-lib-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib/rustlib/x86_64-unknown-linux-gnu/lib" "--rustc-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "--src-base" "/checkout/src/test/ui" "--build-base" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui" "--stage-id" "stage2-x86_64-unknown-linux-gnu" "--suite" "ui" "--mode" "ui" "--target" "x86_64-unknown-linux-gnu" "--host" "x86_64-unknown-linux-gnu" "--llvm-filecheck" "/usr/lib/llvm-12/bin/FileCheck" "--nodejs" "/usr/bin/node" "--host-rustcflags" "-Crpath -O -Cdebuginfo=0  -Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "--target-rustcflags" "-Crpath -O -Cdebuginfo=0  -Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "--docck-python" "/usr/bin/python3" "--lldb-python" "/usr/bin/python3" "--gdb" "/usr/bin/gdb" "--quiet" "--llvm-version" "12.0.0" "--llvm-components" "aarch64 aarch64asmparser aarch64codegen aarch64desc aarch64disassembler aarch64info aarch64utils aggressiveinstcombine all all-targets amdgpu amdgpuasmparser amdgpucodegen amdgpudesc amdgpudisassembler amdgpuinfo amdgpuutils analysis arm armasmparser armcodegen armdesc armdisassembler arminfo armutils asmparser asmprinter avr avrasmparser avrcodegen avrdesc avrdisassembler avrinfo binaryformat bitreader bitstreamreader bitwriter bpf bpfasmparser bpfcodegen bpfdesc bpfdisassembler bpfinfo cfguard codegen core coroutines coverage debuginfocodeview debuginfodwarf debuginfogsym debuginfomsf debuginfopdb demangle dlltooldriver dwarflinker engine executionengine extensions filecheck frontendopenacc frontendopenmp fuzzmutate globalisel hellonew hexagon hexagonasmparser hexagoncodegen hexagondesc hexagondisassembler hexagoninfo instcombine instrumentation interfacestub interpreter ipo irreader jitlink lanai lanaiasmparser lanaicodegen lanaidesc lanaidisassembler lanaiinfo libdriver lineeditor linker lto mc mca mcdisassembler mcjit mcparser mips mipsasmparser mipscodegen mipsdesc mipsdisassembler mipsinfo mirparser msp430 msp430asmparser msp430codegen msp430desc msp430disassembler msp430info native nativecodegen nvptx nvptxcodegen nvptxdesc nvptxinfo objcarcopts object objectyaml option orcjit orcshared orctargetprocess passes perfjitevents powerpc powerpcasmparser powerpccodegen powerpcdesc powerpcdisassembler powerpcinfo profiledata remarks riscv riscvasmparser riscvcodegen riscvdesc riscvdisassembler riscvinfo runtimedyld scalaropts selectiondag sparc sparcasmparser sparccodegen sparcdesc sparcdisassembler sparcinfo support symbolize systemz systemzasmparser systemzcodegen systemzdesc systemzdisassembler systemzinfo tablegen target textapi transformutils vectorize webassembly webassemblyasmparser webassemblycodegen webassemblydesc webassemblydisassembler webassemblyinfo windowsmanifest x86 x86asmparser x86codegen x86desc x86disassembler x86info xcore xcorecodegen xcoredesc xcoredisassembler xcoreinfo xray" "--system-llvm" "--cc" "" "--cxx" "" "--cflags" "" "--adb-path" "adb" "--adb-test-dir" "/data/tmp/work" "--android-cross-path" "" "--channel" "nightly" "--color" "always"


Build completed unsuccessfully in 0:14:44

@bors
Copy link
Contributor

bors commented Jan 6, 2022

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

@Mark-Simulacrum
Copy link
Member

r+'d #92169, once that merges we can rebase this and do another perf run.

@Mark-Simulacrum Mark-Simulacrum 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 10, 2022
@JohnCSimon
Copy link
Member

Ping from triage:
@In-line:

Mark-Simulacrum commented 26 days ago
r+'d #92169, once that merges we can rebase this and do another perf run.

When it's ready for review send a message containing
@rustbot ready to switch back to S-waiting-on-review

@jyn514 jyn514 removed their request for review February 15, 2022 04:52
@Dylan-DPC
Copy link
Member

@In-line if you can rebase this, we can push this forward

@JohnCSimon
Copy link
Member

@In-line
Ping from triage: I'm closing this due to inactivity, Please reopen when you are ready to continue with this. Thanks for your contribution.

@rustbot label: +S-inactive

@JohnCSimon JohnCSimon closed this Mar 20, 2022
@rustbot rustbot added the S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. label Mar 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-diagnostics Area: Messages for errors, warnings, and lints perf-regression Performance regression. S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. 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.