caller_location: point to macro invocation sites, like file!/line!, and use in core::panic!. by eddyb · Pull Request #65973 · rust-lang/rust

eddyb · 2019-10-30T16:59:58Z

The main change here is to core::panic!, trying to fix this remaining regression: #65927 (comment)

However, in order for caller_location to be usable from macros the same way file!()/line!() are, it needs to have the same behavior (of extracting the macro invocation site Span and using that).

Arguably we would've had to do this at some point anyway, if we want to use #[track_caller] to replace the file!()/line!() uses from macros, but I'm not sure the RFC mentions this at all.

r? @petrochenkov cc @anp @nnethercote

eddyb · 2019-10-30T17:00:36Z

@bors try @rust-timer queue

rust-timer · 2019-10-30T17:00:38Z

Awaiting bors try build completion

bors · 2019-10-30T17:00:53Z

⌛ Trying commit 49f9626 with merge d9ee820ba4b43bd12ef85454cdcd7d55f2763b6d...

Centril · 2019-10-30T17:48:31Z

src/librustc_mir/interpret/intrinsics.rs

            "caller_location" => {
-                let caller = self.tcx.sess.source_map().lookup_char_pos(span.lo());
+                let topmost = span.ctxt().outer_expn().expansion_cause().unwrap_or(span);
+                let caller = self.tcx.sess.source_map().lookup_char_pos(topmost.lo());


Can we de-dup this?

Would be nice if we could pass Span into the query, but @anp ran into trouble with that, I think the result was ICEs from the incremental system.

You could just do:

fn caller(&self, span: Span) -> Whatever { let topmost = span.ctxt().outer_expn().expansion_cause().unwrap_or(span); self.sess.source_map().lookup_char_pos(topmost.lo()) }

Where would you put this? Also, "caller" is misleading, in fact the name of caller_location is suboptimal (likely due to the RFC assuming an implementation strategy less versatile than the one being followed by @anp).

@eddyb I think you are taking the snippet too literally; rename it to anything you see fit. It would be placed on TyCtxt -- it could also be placed on SourceMap.

But, like, there is little generality in it. It's literally "caller_location helper" if it does those two things, and that doesn't feel right to put anywhere outside the query for it (which, again, would ideally take Span).

I suppose, but it does make sure the logics don't grow apart. I guess we'll have to ensure that through reviews instead.

FWIW "you could just do" is my least favorite way to phrase review feedback.

@eddyb is correct that I had ICEs when using a Span as a parameter to a query. I agree with them that two callsites does not a need for deduplication make (yet).

petrochenkov · 2019-10-30T18:00:16Z

cc #65664 (need to read before reviewing this)

bors · 2019-10-30T20:03:49Z

☀️ Try build successful - checks-azure
Build commit: d9ee820ba4b43bd12ef85454cdcd7d55f2763b6d (d9ee820ba4b43bd12ef85454cdcd7d55f2763b6d)

rust-timer · 2019-10-30T20:03:51Z

Queued d9ee820ba4b43bd12ef85454cdcd7d55f2763b6d with parent c553e8e, future comparison URL.

rust-timer · 2019-10-31T02:36:51Z

Finished benchmarking try commit d9ee820ba4b43bd12ef85454cdcd7d55f2763b6d, comparison URL.

eddyb · 2019-10-31T06:41:38Z

Looks like it fixed the regression, and comparing before the original PR to after this change agrees.

petrochenkov · 2019-11-03T07:44:54Z

@bors r+

bors · 2019-11-03T07:44:55Z

📌 Commit 49f9626 has been approved by petrochenkov

Centril · 2019-11-05T19:35:28Z

src/libcore/macros.rs

+    core_intrinsics,
+)]
 #[stable(feature = "core", since = "1.6.0")]
 macro_rules! panic {


btw, I think a previous PR forgot to change the macro in libstd (which is not the same as the one in libcore). cc @anp

@anp told me it was intentional, as they use different entry-points, and we can probably wait to have full #[track_caller] before changing libstd.

@petrochenkov

…ochenkov caller_location: point to macro invocation sites, like file!/line!, and use in core::panic!. The main change here is to `core::panic!`, trying to fix this remaining regression: rust-lang#65927 (comment) However, in order for `caller_location` to be usable from macros the same way `file!()`/`line!()` are, it needs to have the same behavior (of extracting the macro invocation site `Span` and using that). Arguably we would've had to do this at some point anyway, if we want to use `#[track_caller]` to replace the `file!()`/`line!()` uses from macros, but I'm not sure the RFC mentions this at all. r? @petrochenkov cc @anp @nnethercote

@ghost

Rollup of 9 pull requests Successful merges: - #65776 (Rename `LocalInternedString` and more) - #65973 (caller_location: point to macro invocation sites, like file!/line!, and use in core::panic!.) - #66015 (librustc_lexer: Refactor the module) - #66062 (Configure LLVM module PIC level) - #66086 (bump smallvec to 1.0) - #66092 (Use KERN_ARND syscall for random numbers on NetBSD, same as FreeBSD.) - #66103 (Add target thumbv7neon-unknown-linux-musleabihf) - #66133 (Update the bundled `wasi-libc` repository) - #66139 (use American spelling for `pluralize!`) Failed merges: r? @ghost

eddyb added 2 commits October 30, 2019 18:54

caller_location: point to macro invocation sites, like file!/line!.

c17f89d

caller_location: use in core::panic!.

49f9626

rust-highfive assigned petrochenkov Oct 30, 2019

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 30, 2019

eddyb mentioned this pull request Oct 30, 2019

Don't use eval_always for miri queries used from codegen. #65927

Merged

Centril reviewed Oct 30, 2019

View reviewed changes

Centril added the F-track_caller `#![feature(track_caller)]` label Nov 1, 2019

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 3, 2019

Centril reviewed Nov 5, 2019

View reviewed changes

Centril mentioned this pull request Nov 6, 2019

Rollup of 9 pull requests #66143

Merged

bors merged commit 49f9626 into rust-lang:master Nov 6, 2019

eddyb deleted the caller-location-panic branch November 6, 2019 12:33

SimonSapin mentioned this pull request Nov 12, 2019

sccache consistently errors on CI since Rust nightly-2019-11-07 servo/servo#24714

Closed

alexcrichton mentioned this pull request Jul 30, 2020

WebAssembly size regression between 1.40 and 1.41 #74947

Open

Uh oh!

Conversation

eddyb commented Oct 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eddyb commented Oct 30, 2019

Uh oh!

rust-timer commented Oct 30, 2019

Uh oh!

bors commented Oct 30, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

petrochenkov commented Oct 30, 2019

Uh oh!

bors commented Oct 30, 2019

Uh oh!

rust-timer commented Oct 30, 2019

Uh oh!

rust-timer commented Oct 31, 2019

Uh oh!

eddyb commented Oct 31, 2019

Uh oh!

petrochenkov commented Nov 3, 2019

Uh oh!

bors commented Nov 3, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

eddyb commented Oct 30, 2019 •

edited

Loading