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.

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

Apply track_caller to closure on expect_non_local() #96894

Merged
merged 1 commit into from
Jun 2, 2022
Merged

Apply track_caller to closure on expect_non_local() #96894

merged 1 commit into from
Jun 2, 2022

Conversation

JohnTitor
Copy link
Member

r? @petrochenkov

Alternatively we could remove the closure by replicating the same logic of map_id(). I'm happy to switch to it if you'd like.

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label May 10, 2022
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 10, 2022
@JohnTitor
Copy link
Member Author

(Will fix the CI failure after discussing a preferred way)

@petrochenkov
Copy link
Contributor

Using #[track_caller] (and also #[cold]) on the closure would be the better alternative both here and in #96778.

However, something is broken in feature gating of #[track_caller] on closures, and they cannot be "ungated" even if the corresponding #![feature] is in place.
It would be great to find out why it happens and fix that issue.

@petrochenkov petrochenkov 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-review Status: Awaiting review from the assignee but also interested parties. labels May 10, 2022
@JohnTitor
Copy link
Member Author

Hmm, I'm afraid that I'm not familiar with it. At least, looking at #87064 and related code, feature gating is fine. We use expect_non_local() on rustc_resolve and librustdoc, I think this should be related (actually local build passes when I added the attr on rustc_resolve along with rustc_hir), right? My guess is an attribute (track_caller, in this case) is also added at callsite then a crate containing that callsite will need a feature attr.
However I don't think I can resolve this issue only by my hand, any help would be appreciated.

@petrochenkov
Copy link
Contributor

cc @Aaron1011

@Aaron1011
Copy link
Member

Aaron1011 commented May 11, 2022 •
edited
Loading

This looks like a broader issue - the codegen_fn_attrs query does feature-gating, and used to be called for foreign DefIds. This was recently changed in #96473 - and I confirmed that this issue doesn't occur with ./x.py build --stage 2 src/tools/rustdoc.

For now, I think you could just add #![feature(closure_track_caller)] to rustdoc behind a cfg_attr(bootstrap), and let someone remove it after the next beta bump.

@JohnTitor
Copy link
Member Author

👍, how about @petrochenkov?

@JohnTitor
Copy link
Member Author

By the way, is there an issue tracking that weird behavior?

@JohnTitor JohnTitor mentioned this pull request May 16, 2022
Open
@JohnTitor JohnTitor force-pushed the expect-non-local-track-caller branch 2 times, most recently from cba1cf0 to 99d7cc0 Compare May 16, 2022 10:09
@JohnTitor
Copy link
Member Author

Added the feature attribute to librustdoc as suggested, @petrochenkov could you take a look?

@JohnTitor JohnTitor added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 16, 2022
@petrochenkov
Copy link
Contributor

Maybe we should just wait until #96473 gets into the bootstrap compiler, it's going to happen pretty soon.
Adding the feature to all crates is not scalable, e.g. for #96778 that would mean adding it to majority of compiler crates.

@petrochenkov petrochenkov added S-blocked Status: Blocked on something else such as an RFC or other implementation work. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 16, 2022
@petrochenkov
Copy link
Contributor

Blocked on #97214.

@petrochenkov
Copy link
Contributor

#97214 has been merged.
@rustbot author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-blocked Status: Blocked on something else such as an RFC or other implementation work. labels May 29, 2022
@JohnTitor JohnTitor force-pushed the expect-non-local-track-caller branch from 99d7cc0 to c7db4b0 Compare May 31, 2022 14:57
@petrochenkov
Copy link
Contributor

Thanks!
@bors r+ rollup

@bors
Copy link
Contributor

bors commented May 31, 2022

📌 Commit c7db4b0 has been approved by petrochenkov

@bors bors removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label May 31, 2022
@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label May 31, 2022
@JohnTitor
Copy link
Member Author

The merge queue doesn't have this PR, let me close/reopen to fix it...

@JohnTitor JohnTitor closed this Jun 2, 2022
@JohnTitor JohnTitor reopened this Jun 2, 2022
@Dylan-DPC Dylan-DPC mentioned this pull request Jun 2, 2022
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 2, 2022
@bors
Auto merge of rust-lang#97644 - Dylan-DPC:rollup-xaeio91, r=Dylan-DPC
9598b4b 
Rollup of 6 pull requests

Successful merges:

 - rust-lang#96894 (Apply track_caller to closure on `expect_non_local()`)
 - rust-lang#97023 (Diagnose anonymous lifetimes errors more uniformly between async and regular fns)
 - rust-lang#97397 (Stabilize `box_into_pin`)
 - rust-lang#97587 (Migrate more diagnostics to use the `#[derive(SessionDiagnostic)]`)
 - rust-lang#97603 (Arc make_mut doc comment spelling correction.)
 - rust-lang#97635 (Fix file metadata documentation for Windows)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit ddc5d2c into rust-lang:master Jun 2, 2022
@rustbot rustbot added this to the 1.63.0 milestone Jun 2, 2022
@JohnTitor JohnTitor deleted the expect-non-local-track-caller branch June 2, 2022 22:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@petrochenkov petrochenkov

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.63.0
Development

Successfully merging this pull request may close these issues.
6 participants
@JohnTitor @petrochenkov @Aaron1011 @bors @rust-highfive @rustbot