Skip to content

Fix Debug for Location #142373

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 2 commits into from
Jun 16, 2025
Merged

Fix Debug for Location #142373

merged 2 commits into from
Jun 16, 2025
+20 −1

Conversation

m-ou-se
Copy link
Member

@m-ou-se m-ou-se commented Jun 11, 2025

Fixes #142279

@rustbot
Copy link
Collaborator

rustbot commented Jun 11, 2025

r? @tgross35

rustbot has assigned @tgross35.
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 rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Jun 11, 2025
@tgross35
Copy link
Contributor

@bors r+ rollup

@bors
Copy link
Collaborator

bors commented Jun 11, 2025

📌 Commit a57d053 has been approved by tgross35

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 Jun 11, 2025
joshtriplett
joshtriplett reviewed Jun 11, 2025
View reviewed changes
library/coretests/tests/panic/location.rs Outdated
#[test]
fn location_debug() {
let f = format!("{:?}", Location::caller());
assert!(f.contains("location.rs"));
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
assert!(f.contains("location.rs"));
assert!(f.contains("location.rs"));
assert!(!f.contains("\0"));
assert!(!f.contains("\\0"));

Copy link
Member Author

Choose a reason for hiding this comment

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

I immediately clicked 'commit suggestion', but actually: i think this might cause funny failures:

If someone works on windows in a directory that starts with a 0, the file name might be "C:\\Users\\0abc\\Rust\\...", which will contain \\0.

Copy link
Member Author

Choose a reason for hiding this comment

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

I changed it to assert!(f.contains(&format!("{:?}", file!())));. That way, it checks the entire file name including the surrounding quotes. Then we can be sure there's no additional \0 displayed.

@tgross35
Copy link
Contributor

@bors r-

@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 Jun 11, 2025
tgross35
tgross35 reviewed Jun 11, 2025
View reviewed changes
library/coretests/tests/panic/location.rs
fn location_debug() {
let f = format!("{:?}", Location::caller());
assert!(f.contains("location.rs"));
assert!(f.contains("35"));
Copy link
Contributor

Choose a reason for hiding this comment

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

I suppose (line!() - 2).to_string() would be more robust to test changes than literal 35

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure. But the other tests in this file also just hardcode the number. I like putting as little logic as possible in tests.

Copy link
Contributor

Choose a reason for hiding this comment

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

Didn't realize the entire file hardcodes them, 🤷‍♂️ in that case

@m-ou-se m-ou-se force-pushed the debug-for-location branch from 363e07b to 26155d1 Compare June 11, 2025 18:46
tgross35
tgross35 approved these changes Jun 11, 2025
View reviewed changes
Copy link
Contributor

@tgross35 tgross35 left a comment

Choose a reason for hiding this comment

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

Could use a squash, r=me again after that

@tgross35 tgross35 force-pushed the debug-for-location branch from 26155d1 to 5ac1cd9 Compare June 16, 2025 04:18
@tgross35
Copy link
Contributor

@m-ou-se I just squashed the last two for you so we can get this fix in. Thanks!

@bors r+ rollup

@bors
Copy link
Collaborator

bors commented Jun 16, 2025

📌 Commit 5ac1cd9 has been approved by tgross35

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 16, 2025
tgross35 added a commit to tgross35/rust that referenced this pull request Jun 16, 2025
@tgross35
Rollup merge of rust-lang#142373 - m-ou-se:debug-for-location, r=tgro…
a5e9bfd 
…ss35

Fix Debug for Location

Fixes rust-lang#142279
@tgross35 tgross35 mentioned this pull request Jun 16, 2025
Closed
bors added a commit that referenced this pull request Jun 16, 2025
@bors
Auto merge of #142564 - tgross35:rollup-v12umar, r=tgross35
10a3c71 
Rollup of 8 pull requests

Successful merges:

 - #140809 (Reduce special casing for the panic runtime)
 - #142082 (Refactor `rustc_attr_data_structures` documentation)
 - #142125 (Stabilize "file_lock" feature)
 - #142373 (Fix Debug for Location)
 - #142414 (ignore `run-make` tests that need `std` on targets without `std`)
 - #142416 (Assorted bootstrap cleanups (step 2))
 - #142431 (Add initial version of snapshot tests to bootstrap)
 - #142528 (clarify `rustc_do_not_const_check` comment)

r? `@ghost`
`@rustbot` modify labels: rollup
@Kobzol Kobzol mentioned this pull request Jun 16, 2025
Merged
bors added a commit that referenced this pull request Jun 16, 2025
@bors
Auto merge of #142574 - Kobzol:rollup-ldj386u, r=Kobzol
3bc767e 
Rollup of 12 pull requests

Successful merges:

 - #141639 (Expose discriminant values in stable_mir)
 - #142082 (Refactor `rustc_attr_data_structures` documentation)
 - #142125 (Stabilize "file_lock" feature)
 - #142236 (Add documentation for `PathBuf`'s `FromIterator` and `Extend` impls)
 - #142373 (Fix Debug for Location)
 - #142416 (Assorted bootstrap cleanups (step 2))
 - #142431 (Add initial version of snapshot tests to bootstrap)
 - #142450 (Add documentation on top of `rustc_middle/src/query/mod.rs`)
 - #142528 (clarify `rustc_do_not_const_check` comment)
 - #142530 (use `if let` guards where possible)
 - #142561 (Remove an `njn:` comment accidentaly left behind.)
 - #142566 (Fix `-nopt` CI jobs)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit d9c83bb into rust-lang:master Jun 16, 2025
10 checks passed
@rustbot rustbot added this to the 1.89.0 milestone Jun 16, 2025
rust-timer added a commit that referenced this pull request Jun 16, 2025
@rust-timer
Unrolled build for #142373
72e126f 
Rollup merge of #142373 - m-ou-se:debug-for-location, r=tgross35

Fix Debug for Location

Fixes #142279
compiler-errors pushed a commit to compiler-errors/rust that referenced this pull request Jun 16, 2025
@bors @compiler-errors
Auto merge of rust-lang#142574 - Kobzol:rollup-ldj386u, r=Kobzol
ccd6824 
Rollup of 12 pull requests

Successful merges:

 - rust-lang#141639 (Expose discriminant values in stable_mir)
 - rust-lang#142082 (Refactor `rustc_attr_data_structures` documentation)
 - rust-lang#142125 (Stabilize "file_lock" feature)
 - rust-lang#142236 (Add documentation for `PathBuf`'s `FromIterator` and `Extend` impls)
 - rust-lang#142373 (Fix Debug for Location)
 - rust-lang#142416 (Assorted bootstrap cleanups (step 2))
 - rust-lang#142431 (Add initial version of snapshot tests to bootstrap)
 - rust-lang#142450 (Add documentation on top of `rustc_middle/src/query/mod.rs`)
 - rust-lang#142528 (clarify `rustc_do_not_const_check` comment)
 - rust-lang#142530 (use `if let` guards where possible)
 - rust-lang#142561 (Remove an `njn:` comment accidentaly left behind.)
 - rust-lang#142566 (Fix `-nopt` CI jobs)

r? `@ghost`
`@rustbot` modify labels: rollup
@m-ou-se m-ou-se deleted the debug-for-location branch June 16, 2025 20:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joshtriplett joshtriplett joshtriplett left review comments

@tgross35 tgross35 tgross35 approved these changes

Assignees

@tgross35 tgross35

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.89.0
Development

Successfully merging this pull request may close these issues.

panic::Location Debug implementation now prints a byte array
5 participants
@m-ou-se @rustbot @tgross35 @bors @joshtriplett