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

panic msg for slicing used to contain path to user's code, now contains path to core #100696

Closed
carols10cents opened this issue Aug 17, 2022 · 8 comments · Fixed by #100759
Closed
Labels
C-bug Category: This is a bug. P-high High priority regression-from-stable-to-stable Performance or correctness regression from one stable version to another. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@carols10cents
Copy link
Member

carols10cents commented Aug 17, 2022

I just noticed this regression from 1.60 to 1.61 because TRPL reasons.

Code

I tried this code:

fn main() {
    let s = "ü";
    println!("{}", &s[0..1]);
}

I expected to see a panic with this message, pointing to the line in my code that caused the panic, which is what I see if I run cargo +1.60 run:

thread 'main' panicked at 'byte index 1 is not a char boundary; it is inside 'ü' (bytes 0..2) of `ü`', 
src/main.rs:3:21

Instead, if I run cargo +1.61 run, I see a panic with a message pointing to a path in core:

thread 'main' panicked at 'byte index 1 is not a char boundary; it is inside 'ü' (bytes 0..2) of `ü`', 
library/core/src/str/mod.rs:127:5

For clearer viewing:

- [...] it is inside 'ü' (bytes 0..2) of `ü`', src/main.rs:3:21
+ [...] it is inside 'ü' (bytes 0..2) of `ü`', library/core/src/str/mod.rs:127:5

The core path is still what I see with cargo +1.63 run.

Version it worked on

It most recently worked on: Rust 1.60.0

Version with regression

It stopped working with 1.61.0

rustc --version --verbose:

rustc 1.61.0 (fe5b13d68 2022-05-18)
binary: rustc
commit-hash: fe5b13d681f25ee6474be29d748c65adcd91f69e
commit-date: 2022-05-18
host: aarch64-apple-darwin
release: 1.61.0
LLVM version: 14.0.0

Other info

What's fun is because of the infrastructure I have with the book, I can see that Rust 1.47 was the first version to point to the user's code; before then it pointed to core as well.

This does NOT seem to occur with all panics coming from core; for example, trying to unwrap a None points to main:

fn main() {
    None.unwrap()
}
thread 'main' panicked at 'called `Option::unwrap()` on a `None` value', src/main.rs:2:10

Indexing out of bounds of an array points to main:

use rand;

fn main() {
    let index: usize = rand::random(); // to get around the compile-time catch of out-of-bound constants
    let foo = [1, 2, 3][index];
}
thread 'main' panicked at 'index out of bounds: the len is 3 but the index is 13912094962097789605', 
src/main.rs:5:15

Interestingly, slicing out of bounds of an array points to core (that is, this is the same issue):

use rand;

fn main() {
    let index: usize = rand::random(); // to get around the compile-time catch of out-of-bound constants
    let foo = &[1, 2, 3][index..index + 1];
}
thread 'main' panicked at 'range end index 17812862265448483785 out of range for slice of length 3', 
library/core/src/slice/index.rs:73:5

so it doesn't appear to be a problem with panic messages in general, but something specific about slicing panics in particular.

@carols10cents carols10cents added C-bug Category: This is a bug. regression-untriaged Untriaged performance or correctness regression. labels Aug 17, 2022
@rustbot rustbot added the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Aug 17, 2022
@carols10cents carols10cents changed the title panic msg for slicing on non-char boundary used to contain path to user's code, now contains path to core panic msg for slicing used to contain path to user's code, now contains path to core Aug 17, 2022
@compiler-errors
Copy link
Member

I didn't bisect this, but I'm about 90% sure that this is due to #94657 cc @fee1-dead

@fee1-dead
Copy link
Member

fee1-dead commented Aug 18, 2022

This is a flaw in the const_eval_select intrinsic's implementation. We currently rewire it to use the implementation which calls the functions from the FnOnce trait. That call cannot be track_caller, and therefore panics will trace to the call instead. The previous design was to use lang item machinery to rewire it to call function items, which would fix this.

@Mark-Simulacrum
Copy link
Member

This seems like a pretty unfortunate regression in ability to debug these - do we have line of sight to fixing that? If I follow the PR, it's for unstable indexing or so? (i.e., perhaps we should revert it if we can't improve the diagnostic quality here)?

@apiraino
Copy link
Contributor

WG-prioritization assigning priority (Zulip discussion).

@rustbot label -I-prioritize +P-high

@rustbot rustbot added P-high High priority and removed I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Aug 18, 2022
@apiraino apiraino added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. regression-from-stable-to-stable Performance or correctness regression from one stable version to another. and removed regression-untriaged Untriaged performance or correctness regression. labels Aug 18, 2022
@fee1-dead
Copy link
Member

I'll try to fix this without reverting. If you don't hear from me within the next four hours, feel free to do a revert.

@fee1-dead fee1-dead assigned fee1-dead and unassigned fee1-dead Aug 18, 2022
@fee1-dead
Copy link
Member

Tried and it is a bit hard. Feel free to revert now and I will followup later.

@estebank
Copy link
Contributor

Lets make sure we add a regression test, regardless of the solution (revert or fix).

@fee1-dead
Copy link
Member

#100759 is in the queue and contains a regression test.

Dylan-DPC added a commit to Dylan-DPC/rust that referenced this issue Aug 25, 2022
…t_real_intrinsic, r=oli-obk,RalfJung

Make `const_eval_select` a real intrinsic

This fixes issues where `track_caller` functions do not have nice panic
messages anymore when there is a call to the function, and uses the
MIR system to replace the call instead of dispatching via lang items.

Fixes rust-lang#100696.
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this issue Aug 25, 2022
…t_real_intrinsic, r=oli-obk,RalfJung

Make `const_eval_select` a real intrinsic

This fixes issues where `track_caller` functions do not have nice panic
messages anymore when there is a call to the function, and uses the
MIR system to replace the call instead of dispatching via lang items.

Fixes rust-lang#100696.
@bors bors closed this as completed in 9358d09 Sep 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: This is a bug. P-high High priority regression-from-stable-to-stable Performance or correctness regression from one stable version to another. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants