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

Add separate feature gate for async fn track caller #112117

Merged
merged 1 commit into from
Aug 5, 2023
Merged

Add separate feature gate for async fn track caller #112117

merged 1 commit into from
Aug 5, 2023

Conversation

bryangarza
Copy link
Contributor

@bryangarza bryangarza commented May 30, 2023 •
edited
Loading

This patch adds a feature gate async_fn_track_caller that is separate from closure_track_caller. This is to allow enabling async_fn_track_caller separately.

Fixes #110009

@rustbot
Copy link
Collaborator

rustbot commented May 30, 2023

r? @wesleywiser

(rustbot has picked a reviewer for you, use r? to override)

@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 May 30, 2023
@bryangarza bryangarza force-pushed the track-caller-feature-gate branch 2 times, most recently from 6778fee to 937b429 Compare May 30, 2023 21:52
@bryangarza
Copy link
Contributor Author

@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-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 31, 2023
@bryangarza
Copy link
Contributor Author

One thing I'm not sure is if I should be updating this line that sets allow_gen_future:

rust/compiler/rustc_ast_lowering/src/item.rs

Line 85 in e6d4725

allow_gen_future: Some([sym::gen_future, sym::closure_track_caller][..].into()),

It's used here:
https://github.com/rust-lang/rust/blob/18de7625ce234a0ac6d789bb358885f9ac8a64f2/compiler/rustc_ast_lowering/src/expr.rs#L663-L664

The tests pass as-is, but I'm not sure if I should have to reference the new feature gate inside of allow_gen_future.

compiler-errors
compiler-errors reviewed Jun 5, 2023
View reviewed changes
tests/ui/async-await/track-caller/panic-track-caller.rs
@@ -81,7 +88,7 @@ async fn foo_closure() {

// Since compilation is expected to fail for this fn when using
// `nofeat`, we test that separately in `async-block.rs`
#[cfg(feat)]
#[cfg(cls)]
Copy link
Member

Choose a reason for hiding this comment

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

Why is this using cls and not afn? This is an async block -- shouldn't it only work when async_fn_track_caller is enabled?

Copy link
Member

Choose a reason for hiding this comment

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

Well, I guess it's separate.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah it's separate, this case is still gated by closure_track_caller

compiler-errors
compiler-errors reviewed Jun 6, 2023
View reviewed changes
Copy link
Member

@compiler-errors compiler-errors left a comment

Choose a reason for hiding this comment

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

Can you add more tests to make sure, e.g., nested closures inside of async fns don't accidentally allow #[track_caller] with just the async_fn_track_caller feature gate?

@bryangarza
Copy link
Contributor Author

Can you add more tests to make sure, e.g., nested closures inside of async fns don't accidentally allow #[track_caller] with just the async_fn_track_caller feature gate?

@compiler-errors I now updated async-block.rs and async-closure-gate.rs now with a couple more cases, is that what you had in mind?

@compiler-errors
Copy link
Member

Also while we're at it, that line you mentioned:

allow_gen_future: Some([sym::gen_future, sym::closure_track_caller][..].into()),

We could actually be even more conservative and change it so that we only add sym::closure_track_caller if tcx.features().async_fn_track_caller is active... 🤔

@bryangarza
Copy link
Contributor Author

@rustbot ready

@rustbot rustbot 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 Jul 13, 2023
@compiler-errors
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Jul 29, 2023

📌 Commit b9e395fd4e612742d7d41542fbbe2a80eb9b95d7 has been approved by compiler-errors

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 Jul 29, 2023
@bors
Copy link
Contributor

bors commented Jul 29, 2023

⌛ Testing commit b9e395fd4e612742d7d41542fbbe2a80eb9b95d7 with merge ea47e718b84c05a876f43cd313e78683bdfd7cca...

@bors
Copy link
Contributor

bors commented Jul 29, 2023

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jul 29, 2023
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@compiler-errors
Copy link
Member

@bryangarza you need to bless ui tests

@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-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 31, 2023
@bryangarza
Add separate feature gate for async fn track caller
673ab17 
This patch adds a feature gate `async_fn_track_caller` that is separate from `closure_track_caller`. This is to allow enabling `async_fn_track_caller` separately.

Fixes rust-lang#110009
@bryangarza
Copy link
Contributor Author

@rustbot ready

@rustbot rustbot 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 Aug 3, 2023
@compiler-errors
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Aug 4, 2023

📌 Commit 673ab17 has been approved by compiler-errors

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 Aug 4, 2023
@bors
Copy link
Contributor

bors commented Aug 4, 2023

⌛ Testing commit 673ab17 with merge e173a8e...

@bors
Copy link
Contributor

bors commented Aug 5, 2023

☀️ Test successful - checks-actions
Approved by: compiler-errors
Pushing e173a8e to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 5, 2023
@bors bors merged commit e173a8e into rust-lang:master Aug 5, 2023
12 checks passed
@rustbot rustbot added this to the 1.73.0 milestone Aug 5, 2023
@bors bors mentioned this pull request Aug 5, 2023
Merged
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (e173a8e): comparison URL.

Overall result: ✅ improvements - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)		 - - 0
Regressions ❌
(secondary)		 - - 0
Improvements ✅
(primary)		 -0.7% [-1.2%, -0.4%] 36
Improvements ✅
(secondary)		 -0.6% [-0.9%, -0.2%] 10
All ❌✅ (primary) -0.7% [-1.2%, -0.4%] 36

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)		 1.4% [1.0%, 1.8%] 3
Regressions ❌
(secondary)		 2.2% [1.7%, 2.5%] 3
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 -2.0% [-3.1%, -1.4%] 4
All ❌✅ (primary) 1.4% [1.0%, 1.8%] 3

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)		 - - 0
Regressions ❌
(secondary)		 - - 0
Improvements ✅
(primary)		 -2.1% [-2.1%, -2.1%] 1
Improvements ✅
(secondary)		 - - 0
All ❌✅ (primary) -2.1% [-2.1%, -2.1%] 1

Binary size

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)		 - - 0
Regressions ❌
(secondary)		 - - 0
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 -0.1% [-0.1%, -0.1%] 9
All ❌✅ (primary) - - 0

Bootstrap: 650.779s -> 650.501s (-0.04%)

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

@compiler-errors compiler-errors compiler-errors approved these changes

Assignees

@compiler-errors compiler-errors

Labels
merged-by-bors This PR was explicitly merged by bors. 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.73.0
Development

Successfully merging this pull request may close these issues.

Create separate feature gate for #[track_caller] on async fn
7 participants
@bryangarza @rustbot @compiler-errors @bors @rust-log-analyzer @rust-timer @wesleywiser