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

Implement diagnostic for String conversion #90645

Merged
merged 1 commit into from Nov 15, 2021
Merged

Conversation

terrarier2111
Copy link
Contributor

This is my first real contribution to rustc, any feedback is highly appreciated.
This should fix #89856

Thanks to @estebank for guiding me.

@rust-highfive
Copy link
Collaborator

r? @cjgillot

(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 Nov 6, 2021
@joshtriplett
Copy link
Member

@terrarier2111 This seems like a great suggestion! Could you please add a test case that triggers the new diagnostic and tests its output?

(That will also make it easier to review.)

@terrarier2111
Copy link
Contributor Author

@terrarier2111 This seems like a great suggestion! Could you please add a test case that triggers the new diagnostic and tests its output?

(That will also make it easier to review.)

Is this okay, how i did it?

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@terrarier2111
Copy link
Contributor Author

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)

diff of stderr:

1 error[E0308]: mismatched types
-  --> $DIR/issue89856.rs:6:20
-   |
- 6 |     take_str_maybe(option);
-   |                    ^^^^^^
-   |                    |
-   |                    expected `str`, found struct `String`
-   |                    help: try converting the passed type into a &str: `.map(|x| &**x)`
-   |
Some tests failed in compiletest suite=ui mode=ui host=x86_64-unknown-linux-gnu target=x86_64-unknown-linux-gnu
-   = note: expected enum `Option<&str>`
-              found enum `Option<&String>`
+    |
+    |
+ LL |     take_str_maybe(option);
+    |                    |
+    |                    expected `str`, found struct `String`
+    |                    expected `str`, found struct `String`
+    |                    help: try converting the passed type into a &str: `.map(|x| &**x)`
+    = note: expected enum `Option<&str>`
+               found enum `Option<&String>`
12 
13 error: aborting due to previous error
---
To only update this specific test, also pass `--test-args typeck/issue89856.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/typeck/issue89856.rs" "-Zthreads=1" "--target=x86_64-unknown-linux-gnu" "--error-format" "json" "-Ccodegen-units=1" "-Zui-testing" "-Zdeduplicate-diagnostics=no" "-Zemit-future-incompat-report" "--emit" "metadata" "-C" "prefer-dynamic" "--out-dir" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/typeck/issue89856" "-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/typeck/issue89856/auxiliary"
------------------------------------------

------------------------------------------
stderr:
stderr:
------------------------------------------
error[E0308]: mismatched types
  --> /checkout/src/test/ui/typeck/issue89856.rs:6:20
   |
LL |     take_str_maybe(option);
   |                    ^^^^^^
   |                    |
   |                    expected `str`, found struct `String`
   |                    help: try converting the passed type into a &str: `.map(|x| &**x)`
   = note: expected enum `Option<&str>`
              found enum `Option<&String>`

error: aborting due to previous error
---
test result: FAILED. 12276 passed; 1 failed; 110 ignored; 0 measured; 0 filtered out; finished in 138.90s



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:13:49

What did i do wrong here? i don't understand this.

@estebank
Copy link
Contributor

estebank commented Nov 9, 2021

@terrarier2111, to create/update the .stderr file you should use ./x.py test src/test/ui --bless. The .stderr files have some conversions that are hard to get right by hand. If you look at the error, it is giving you a diff between what the file was and what it expected. It is pointing at the whole file because of the leading whitespace (because the output in your terminal was 6, while in the test suite it always gets replaced with LL, which is wider, to avoid nasty reformats when changing the tests). I would also rename issue89856.rs to issue-89856.rs.

@rust-log-analyzer

This comment has been minimized.

@terrarier2111
Copy link
Contributor Author

@estebank could you review this?

Copy link
Contributor

@estebank estebank left a comment

Choose a reason for hiding this comment

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

Thank you for doing this!

I left a couple of nitpicks. Could you also squash your commits into one before we merge?

src/test/ui/typeck/issue-89856.rs Outdated Show resolved Hide resolved
{
if let ty::Adt(_def, subst) = found.kind() {
if subst.len() != 0 {
if let rustc_middle::ty::subst::GenericArgKind::Type(ty) = subst[0].unpack()
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you maybe use rustc_middle::ty::subst::GenericArgKind? We generally import the names we use. I don't think we ever use full paths unless they are very short.

I think there are ways you could shorten these. I think that subst is a Vec. If so, then you can write something like the following and get rid of two levels of indentation:

if let ty::Adt(_def, [subst]) = found.kind() {
    if let GenericArgKind::Type(ty) = subst.unpack() {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This doesn't seem to work. But i can change the path.

@apiraino apiraino added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Nov 11, 2021
@rust-log-analyzer

This comment has been minimized.

Co-authored-by: Esteban Kuber <estebank@users.noreply.github.com>
@terrarier2111
Copy link
Contributor Author

@estebank I squashed all commits, is there anything left to do for me, or is this good to go?

@estebank
Copy link
Contributor

Lets land this as is (str/String conversions are very common), but ideally we'll come back to this and make it work for any two A and B that impl AsRef<A> for B.

@bors r+

@bors
Copy link
Contributor

bors commented Nov 15, 2021

📌 Commit 829a528 has been approved by estebank

@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 Nov 15, 2021
@bors
Copy link
Contributor

bors commented Nov 15, 2021

⌛ Testing commit 829a528 with merge d5a0c7c...

@bors
Copy link
Contributor

bors commented Nov 15, 2021

☀️ Test successful - checks-actions
Approved by: estebank
Pushing d5a0c7c to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Nov 15, 2021
@bors bors merged commit d5a0c7c into rust-lang:master Nov 15, 2021
@rustbot rustbot added this to the 1.58.0 milestone Nov 15, 2021
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (d5a0c7c): comparison url.

Summary: This change led to moderate relevant mixed results 🤷 in compiler performance.

  • Moderate improvement in instruction counts (up to -1.2% on incr-unchanged builds of html5ever)
  • Small regression in instruction counts (up to 0.4% on incr-unchanged builds of style-servo)

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-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 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

@rustbot rustbot added the perf-regression Performance regression. label Nov 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Recommend Option::as_deref when given Option<&String> in place of Option<&str>
10 participants