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

Remove trailing semicolon from macro call span #89860

Merged
merged 5 commits into from
Oct 16, 2021
Merged

Remove trailing semicolon from macro call span #89860

merged 5 commits into from
Oct 16, 2021

Conversation

camsteffen
Copy link
Contributor

@camsteffen camsteffen commented Oct 13, 2021 •
edited
Loading

Macro call site spans are now less surprising/more consistent since they no longer contain a semicolon after the macro call.

The downside is that we need to do a little guesswork to get the semicolon in diagnostics. But this should not be noticeable since it is rare for the semicolon to not immediately follow the macro call.

@rust-highfive
Copy link
Collaborator

Some changes occurred in src/tools/clippy.

cc @rust-lang/clippy

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 13, 2021
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@petrochenkov petrochenkov self-assigned this Oct 13, 2021
@petrochenkov
Copy link
Contributor

Any specific reasons to do this?
The change makes spans inconsistent between statement position spans stmt!(); and free/associated/foreign item position macros.

The change seems fine to me if all bang macro spans are changed, not only statements.
This can be done by removing span from InvocationKind::Bang and taking it from mac when necessary.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@camsteffen
Copy link
Contributor Author

camsteffen commented Oct 13, 2021 •
edited
Loading

Any specific reasons to do this?

Honestly I was motivated from finding multiple instances in Clippy where we have to work around the Span surprisingly containing the semicolon. But I also think the output just seems more sensible. It also fixes some wrong "this warning originates in the macro..." messages.

The change seems fine to me if all bang macro spans are changed, not only statements.
This can be done by removing span from InvocationKind::Bang and taking it from mac when necessary.

I removed the span arg from collect_bang, but I think InvocationKind::Bang still needs span because of this.

Also note I added a special case for macros that expand to an expression. The expanded statement in this case will have the pre-expansion span with the semicolon.

@petrochenkov
Copy link
Contributor

InvocationKind::Bang still needs span because of this.

Nah, no span - no need to update it there.
r? @petrochenkov

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@camsteffen
Copy link
Contributor Author

Uh oh. Not sure about this one...

@camsteffen
Copy link
Contributor Author

Restoring InvocationKind::Bang.span fixes the incremental test.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@JohnTitor
Copy link
Member

@bors r- rollup=iffy
Failed in rollup, needs reblessing tests.

@bors bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Oct 15, 2021
@camsteffen
Copy link
Contributor Author

@bors r=petrochenkov

@bors
Copy link
Contributor

bors commented Oct 15, 2021

📌 Commit cc4bc57 has been approved by petrochenkov

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 15, 2021
@bors
Copy link
Contributor

bors commented Oct 15, 2021

⌛ Testing commit cc4bc57 with merge 71e2fcd99e736c8b8bef74894df929a3d9a7d79f...

@rust-log-analyzer
Copy link
Collaborator

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

Click to see the possible cause of the failure (guessed by this bot) 
---- [ui] ui/macros/macro-use-one.rs stdout ----

error: test run failed!
status: exit status: 101
command: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/remote-test-client" "run" "1" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/macros/macro-use-one/a" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/macros/macro-use-one/auxiliary/libtwo_macros.so"
------------------------------------------
------------------------------------------
uploaded "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/macros/macro-use-one/a", waiting for result
Some tests failed in compiletest suite=ui mode=ui host=x86_64-unknown-linux-gnu target=arm-unknown-linux-gnueabihf
------------------------------------------
stderr:
------------------------------------------
------------------------------------------
thread 'main' panicked at 'client.read_exact(&mut header) failed with Connection reset by peer (os error 104)', src/tools/remote-test-client/src/main.rs:310:9

------------------------------------------


---
test result: FAILED. 12120 passed; 1 failed; 175 ignored; 0 measured; 0 filtered out; finished in 622.49s



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/arm-unknown-linux-gnueabihf/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-arm-unknown-linux-gnueabihf" "--suite" "ui" "--mode" "ui" "--target" "arm-unknown-linux-gnueabihf" "--host" "x86_64-unknown-linux-gnu" "--llvm-filecheck" "/checkout/obj/build/x86_64-unknown-linux-gnu/llvm/build/bin/FileCheck" "--linker" "arm-linux-gnueabihf-gcc" "--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/arm-unknown-linux-gnueabihf/native/rust-test-helpers" "--docck-python" "/usr/bin/python3" "--lldb-python" "/usr/bin/python3" "--llvm-version" "13.0.0-rust-1.57.0-nightly" "--llvm-components" "aarch64 aarch64asmparser aarch64codegen aarch64desc aarch64disassembler aarch64info aarch64utils aggressiveinstcombine all all-targets 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 dwp engine executionengine extensions filecheck frontendopenacc frontendopenmp fuzzmutate globalisel hexagon hexagonasmparser hexagoncodegen hexagondesc hexagondisassembler hexagoninfo instcombine instrumentation interfacestub interpreter ipo irreader jitlink libdriver lineeditor linker lto m68k m68kasmparser m68kcodegen m68kdesc m68kdisassembler m68kinfo 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 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 webassemblyutils windowsmanifest x86 x86asmparser x86codegen x86desc x86disassembler x86info xray" "--cc" "" "--cxx" "" "--cflags" "" "--remote-test-client" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/remote-test-client" "--adb-path" "adb" "--adb-test-dir" "/data/tmp/work" "--android-cross-path" "" "--channel" "nightly" "--color" "always"


Build completed unsuccessfully in 0:27:08

@bors
Copy link
Contributor

bors commented Oct 15, 2021

💔 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 Oct 15, 2021
@camsteffen
Copy link
Contributor Author

@bors retry

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

bors commented Oct 16, 2021

⌛ Testing commit cc4bc57 with merge 4e89811...

@bors
Copy link
Contributor

bors commented Oct 16, 2021

☀️ Test successful - checks-actions
Approved by: petrochenkov
Pushing 4e89811 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Oct 16, 2021
@bors bors merged commit 4e89811 into rust-lang:master Oct 16, 2021
@rustbot rustbot added this to the 1.57.0 milestone Oct 16, 2021
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (4e89811): comparison url.

Summary: This benchmark run did not return any relevant changes.

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

@rustbot label: -perf-regression

@camsteffen camsteffen deleted the macro-semi branch October 17, 2021 00:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@flip1995 flip1995 flip1995 left review comments

@petrochenkov petrochenkov petrochenkov left review comments

Assignees

@petrochenkov petrochenkov

Labels
merged-by-bors This PR was explicitly merged by bors. 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
Milestone
1.57.0
Development

Successfully merging this pull request may close these issues.