Skip to content

Add track_caller to DefId::expect_local() #96747

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 1 commit into from
May 6, 2022

Conversation

JohnTitor
Copy link
Member

Suggested in #96738 (comment).
DefId::expect_local() often causes ICEs (panics) and should be a good candidate to add track_caller.

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label May 5, 2022
@rust-highfive
Copy link
Contributor

r? @compiler-errors

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 5, 2022
@compiler-errors
Copy link
Member

@bors r+ rollup

@bors
Copy link
Collaborator

bors commented May 5, 2022

📌 Commit 2ed38cd has been approved by compiler-errors

@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 May 5, 2022
compiler-errors added a commit to compiler-errors/rust that referenced this pull request May 5, 2022
…, r=compiler-errors

Add `track_caller` to `DefId::expect_local()`

Suggested in rust-lang#96738 (comment).
`DefId::expect_local()` often causes ICEs (panics) and should be a good candidate to add `track_caller`.
compiler-errors added a commit to compiler-errors/rust that referenced this pull request May 6, 2022
…, r=compiler-errors

Add `track_caller` to `DefId::expect_local()`

Suggested in rust-lang#96738 (comment).
`DefId::expect_local()` often causes ICEs (panics) and should be a good candidate to add `track_caller`.
bors added a commit to rust-lang-ci/rust that referenced this pull request May 6, 2022
…piler-errors

Rollup of 7 pull requests

Successful merges:

 - rust-lang#96174 (mark ptr-int-transmute test as no_run)
 - rust-lang#96639 (Fix typo in `offset_from` documentation)
 - rust-lang#96704 (Add rotation animation on settings button when loading)
 - rust-lang#96730 (Add a regression test for rust-lang#64173 and rust-lang#66152)
 - rust-lang#96741 (Improve settings loading strategy)
 - rust-lang#96744 (Implement [OsStr]::join)
 - rust-lang#96747 (Add `track_caller` to `DefId::expect_local()`)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit b8c829b into rust-lang:master May 6, 2022
@rustbot rustbot added this to the 1.62.0 milestone May 6, 2022
@JohnTitor JohnTitor deleted the expect-local-track-caller branch May 6, 2022 07:29
@@ -279,6 +279,7 @@ impl DefId {
}

#[inline]
#[track_caller]
pub fn expect_local(self) -> LocalDefId {
self.as_local().unwrap_or_else(|| panic!("DefId::expect_local: `{:?}` isn't local", self))
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't work because there's a closure in the middle which doesn't track caller.
So either #[track_caller] need to be applied to the closure too, or match need to be used instead of unwrap_or_else.

Copy link
Member Author

Choose a reason for hiding this comment

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

Didn't know, I'll address it. Thanks!

Copy link
Member Author

Choose a reason for hiding this comment

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

Does the same apply to expect_non_local?

#[track_caller]
pub fn expect_non_local<OtherId>(self) -> Res<OtherId> {
self.map_id(|_| panic!("unexpected `Res::Local`"))
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, fn expect_non_local should have the same issue.

GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request May 6, 2022
…-take-2, r=petrochenkov

Remove closures on `expect_local` to apply `#[track_caller]`

Pointed out in rust-lang#96747 (comment)
Didn't change `expect_non_local` as I'm not sure if it's also the case.
r? `@petrochenkov`
compiler-errors added a commit to compiler-errors/rust that referenced this pull request May 7, 2022
…-take-2, r=petrochenkov

Remove closures on `expect_local` to apply `#[track_caller]`

Pointed out in rust-lang#96747 (comment)
Didn't change `expect_non_local` as I'm not sure if it's also the case.
r? ``@petrochenkov``
# for free to join this conversation on GitHub. Already have an account? # to comment
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
Development

Successfully merging this pull request may close these issues.

6 participants