Skip to content

const_caller_location to use real Span instead of DUMMY_SP #146991

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
Merged
merged 1 commit into from
Sep 26, 2025
Merged

const_caller_location to use real Span instead of DUMMY_SP #146991

merged 1 commit into from
Sep 26, 2025

Conversation

cachebag
Copy link
Contributor

@cachebag cachebag commented Sep 24, 2025 •
edited
Loading

Clarifying usage of DUMMY_SP

@rustbot
Copy link
Collaborator

rustbot commented Sep 24, 2025

Some changes occurred to the CTFE machinery

cc @RalfJung, @oli-obk, @lcnr

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Sep 24, 2025
@rustbot
Copy link
Collaborator

rustbot commented Sep 24, 2025

r? @petrochenkov

rustbot has assigned @petrochenkov.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot

This comment has been minimized.

Sign in to view
@cachebag cachebag force-pushed the span-caller-location branch from aae6c5d to c6a16fc Compare September 24, 2025 16:35
@RalfJung
Copy link
Member

RalfJung commented Sep 24, 2025 •
edited
Loading

What does the PR title have to do with the PR contents? The PR does not link anything to any issue, as far as I can tell.

@cachebag
Copy link
Contributor Author

cachebag commented Sep 24, 2025 •
edited
Loading

What does the PR title have to do with the PR contents? The PR does not link anything to any issue, as far as I can tell.

For some reason I thought it would link. My mistake.

@cachebag cachebag closed this Sep 24, 2025
@rustbot rustbot removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 24, 2025
@RalfJung
Copy link
Member

Maybe I misunderstood what you mean by "link"...

@cachebag
Copy link
Contributor Author

cachebag commented Sep 24, 2025 •
edited
Loading

@RalfJung

I thought that linking the issue # in the title would create a hyperlink to that issue, thus the title.

@RalfJung
Copy link
Member

I'm not talking about the use of #146990 in the title. I'm talking about the rest of the title where you say "link ... to tracking issue ...". The PR title should describe what you changed in the code, but I don't see any connection between that title and what you changed in the code.

@jackh726
Copy link
Member

jackh726 commented Sep 24, 2025 •
edited
Loading

Hey @cachebag, thanks for the PR :)

I think we can land this, seems simple enough. Do you want to reopen? Like @RalfJung said, we don't open issues just to link them in a PR, but that's okay.

Not sure how difficult it would be, but it'd be great if we could get this change covered by a test. Though, for this, I'm not sure it's necessary.

@jackh726 jackh726 assigned jackh726 and unassigned petrochenkov Sep 24, 2025
@cachebag cachebag changed the title link span_as_caller_location FIXME to tracking issue #146990 const_caller_location to use real Span instead of DUMMY_SP Sep 24, 2025
@cachebag
Copy link
Contributor Author

cachebag commented Sep 24, 2025 •
edited
Loading

@RalfJung I see what you are saying, and it makes sense. A link to the issue which describes what would be changed made sense in my head, but given that GH doesn't even link in titles I understand this isn't ideal. I just changed it, thanks.

@jackh726 Hi Jack, thanks for offering to keep this open for me.

I can definitely see what it would take to test this. Do you know where I could start?

After reading the guide fully this afternoon I can see why this PR raised eyebrows- I'll definitely make sure I follow proper contributing guidelines next time I come back here.

@cachebag cachebag reopened this Sep 24, 2025
@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 24, 2025
@RalfJung
Copy link
Member

That span is only relevant if an error occurs in this interpreter. I don't think it's possible for the interpreter to error in const_caller_location_provider. So I don't think a test is possible... and that also would mean it's likely not worth passing through the span if it'll anyway never be used. My first inclination would be to just replace the FIXME by a comment saying the span doesn't matter here since can be no errors.

@oli-obk knows this code better than me though, I may be missing something.

@jackh726
Copy link
Member

I wonder if an alternative would be to make InterpCx::new not take a Span (and thus also no Span for mk_eval_cx_to_read_const_val)? Would be curious if changing all uses to DUMMY_SP changes any tests.

Looking over, it seems like most uses of the Span are just in span_bugs, but an important one is in the LONG_RUNNING_CONST_EVAL lint. Maybe just need to make mk_eval_cx_to_read_const_val pass a DUMMY_SP directly. Worth trying a couple different things.

Worst case, as Ralf said, can just remove the FIXME (and possibly add a comment with reasoning)

@RalfJung
Copy link
Member

Interpreters that run actual user code should have a span indicating the "root" of the evaluation. This is used in various places for error reporting.

But dummy interpreters like this one don't necessarily need a span.

@oli-obk
Copy link
Contributor

oli-obk commented Sep 25, 2025 •
edited
Loading

Yea, we could remove the fixme and also just bug! in case an error occurs since it does look like it can't really fail

@cachebag cachebag force-pushed the span-caller-location branch 3 times, most recently from 70e8b82 to 4b87d94 Compare September 25, 2025 18:01
@cachebag
Copy link
Contributor Author

@RalfJung @oli-obk

Thanks for the clarifications. I reverted the span plumbing, and just replaced the FIXME with the details you all mentioned.

also just bug! in case an error occurs

There's no need for another bug!() site right? I'm going to assume I should just leave it as it is, because I thought the only place errors can surface in const_caller_location_provider is after intern_const_alloc_recursive

RalfJung
RalfJung reviewed Sep 25, 2025
View reviewed changes
@cachebag cachebag force-pushed the span-caller-location branch from 913d34a to c98b3b2 Compare September 25, 2025 20:09
@cachebag cachebag requested a review from RalfJung September 25, 2025 20:11
@RalfJung
Copy link
Member

Looks good, thanks. :)

@bors r+ rollup

@bors
Copy link
Collaborator

bors commented Sep 25, 2025

📌 Commit c98b3b2 has been approved by RalfJung

It is now in the queue for this repository.

@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 Sep 25, 2025
jhpratt added a commit to jhpratt/rust that referenced this pull request Sep 26, 2025
@jhpratt
Rollup merge of rust-lang#146991 - cachebag:span-caller-location, r=R…
d2b5c3d 
…alfJung

const_caller_location to use real Span instead of `DUMMY_SP`

Closes: rust-lang#146990
@jhpratt jhpratt mentioned this pull request Sep 26, 2025
Closed
bors added a commit that referenced this pull request Sep 26, 2025
@bors
Auto merge of #147053 - jhpratt:rollup-tek9gok, r=jhpratt
0c34d2c 
Rollup of 8 pull requests

Successful merges:

 - #146283 (Resolve: (Ref)Cell wrappers to deny mutation during spec resolution.)
 - #146453 (Add general arm-linux.md platform doc.)
 - #146991 (const_caller_location to use real Span instead of `DUMMY_SP`)
 - #146994 (Add `clippy::unconditional_recursion` to `./x clippy ci`)
 - #147027 (Add new `tyalias` intra-doc link disambiguator)
 - #147038 (Rename verbosity functions in bootstrap)
 - #147047 (rustdoc: put the toolbar on the all item index)
 - #147049 (std: fix warning in VEXos stdio module)

r? `@ghost`
`@rustbot` modify labels: rollup
@matthiaskrgr matthiaskrgr mentioned this pull request Sep 26, 2025
Merged
bors added a commit that referenced this pull request Sep 26, 2025
@bors
Auto merge of #147054 - matthiaskrgr:rollup-660g92w, r=matthiaskrgr
5b9007b 
Rollup of 7 pull requests

Successful merges:

 - #146283 (Resolve: (Ref)Cell wrappers to deny mutation during spec resolution.)
 - #146453 (Add general arm-linux.md platform doc.)
 - #146991 (const_caller_location to use real Span instead of `DUMMY_SP`)
 - #146994 (Add `clippy::unconditional_recursion` to `./x clippy ci`)
 - #147038 (Rename verbosity functions in bootstrap)
 - #147047 (rustdoc: put the toolbar on the all item index)
 - #147049 (std: fix warning in VEXos stdio module)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit b184550 into rust-lang:master Sep 26, 2025
10 checks passed
@rustbot rustbot added this to the 1.92.0 milestone Sep 26, 2025
rust-timer added a commit that referenced this pull request Sep 26, 2025
@rust-timer
Unrolled build for #146991
6aeccf6 
Rollup merge of #146991 - cachebag:span-caller-location, r=RalfJung

const_caller_location to use real Span instead of `DUMMY_SP`

Clarifying usage of DUMMY_SP
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RalfJung RalfJung Awaiting requested review from RalfJung

Assignees

@jackh726 jackh726

Labels
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.92.0
Development

Successfully merging this pull request may close these issues.
7 participants
@cachebag @rustbot @RalfJung @jackh726 @oli-obk @bors @petrochenkov