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

Use proper HirId for async track_caller attribute check #105180

Merged
merged 3 commits into from
Dec 6, 2022
Merged

Use proper HirId for async track_caller attribute check #105180

merged 3 commits into from
Dec 6, 2022

Conversation

nbdd0121
Copy link
Contributor

@nbdd0121 nbdd0121 commented Dec 2, 2022

Fix #105134

@rustbot
Copy link
Collaborator

rustbot commented Dec 2, 2022

r? @jackh726

(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 Dec 2, 2022
compiler-errors
compiler-errors reviewed Dec 2, 2022
View reviewed changes
compiler/rustc_ast_lowering/src/expr.rs
@@ -1012,6 +1013,7 @@ impl<'hir> LoweringContext<'_, 'hir> {

let async_body = this.make_async_expr(
Copy link
Member

Choose a reason for hiding this comment

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

Async closures should probably also support #[track_caller]

Copy link
Contributor Author

@nbdd0121 nbdd0121 Dec 2, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

It currently doesn't, so that's why I haven't changed it in this PR.

It isn't difficult change to make, but it does require lifting the attribute lowering up from after ExprKind lowering to before it. Do you happen to know why we currently lower ID and attributes after ExprKind?

Copy link
Member

Choose a reason for hiding this comment

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

I don't think there's a particular reason why we do it -- it shouldn't matter to change the lowering order. I guess if you're not keen to make the change, can you at least add a FIXME? Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added. I then changed Option<HirId> to HirId to simplify code. We don't need to special case async blocks as #[track_caller] is not legal on async block. But I added a test anyway to avoid accidentally allow this on normal async block.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I know the reason now, because paren expr is special 😅

PTAL

Copy link
Member

Choose a reason for hiding this comment

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

Given the incremental issues, maybe we can revert the last change and go with Option<HirId>? Or are those test failures unrelated to the last change?

@compiler-errors
Copy link
Member

r? @compiler-errors @bors delegate+

Thanks, r=me with or without my comment.

cc @bryangarza, you will need to rebase your PR.

@bors
Copy link
Contributor

bors commented Dec 2, 2022

✌️ @nbdd0121 can now approve this pull request

@compiler-errors
Copy link
Member

I'll review the changes in a bit

@nbdd0121
Copy link
Contributor Author

nbdd0121 commented Dec 2, 2022

Not sure why making this change cause incr-comp test to fail. Maybe different HirId ordering causes the typeck result happens to match? Might need some incr-comp expert to take a look @rust-lang/wg-incr-comp

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@cjgillot
Copy link
Contributor

cjgillot commented Dec 3, 2022

You can just bless the incremental tests. The change is in the right direction: stuff that were dirty are now clean.
In the places there is a reported error, remove the reported query from the rustc_clean attribute's except list.

@nbdd0121
Copy link
Contributor Author

nbdd0121 commented Dec 3, 2022

Thanks, @cjgillot! I thought that they were there to ensure that the results are dirty. So these are more like status-quo checks instead to catch accidental changes?

@cjgillot
Copy link
Contributor

cjgillot commented Dec 3, 2022

Pretty much, yes.

@compiler-errors
Copy link
Member

Given @cjgillot's comment and the blessed tests, this seems good to go now. Thanks for fixing this @nbdd0121.

@bors r+

@bors
Copy link
Contributor

bors commented Dec 4, 2022

📌 Commit 76ee06b330ab37588cc380a61f05ef5a56ea4d3c 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 Dec 4, 2022
@bors
Copy link
Contributor

bors commented Dec 5, 2022

⌛ Testing commit 76ee06b330ab37588cc380a61f05ef5a56ea4d3c with merge a3c1e064379c6c903f1d52530d1847e1bb130e50...

@bors
Copy link
Contributor

bors commented Dec 5, 2022

💔 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 Dec 5, 2022
@nbdd0121
Copy link
Contributor Author

nbdd0121 commented Dec 5, 2022

Given that #105134 is a critical regression and thus we want it to be fixed soon, I think it's best to split the async closure support (with HIR attribute lowering changes) into a separate PR. I have added a FIXME note and reverted the PR to the pre Option<HirId> -> HirId change state.

@bors r=compiler-errors

@bors
Copy link
Contributor

bors commented Dec 5, 2022

📌 Commit 34c3773 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 Dec 5, 2022
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@matthiaskrgr matthiaskrgr mentioned this pull request Dec 5, 2022
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 5, 2022
@bors
Auto merge of rust-lang#105328 - matthiaskrgr:rollup-qnfksmq, r=matth…
8e440b0 
…iaskrgr

Rollup of 8 pull requests

Successful merges:

 - rust-lang#104912 (PartialEq: PERs are homogeneous)
 - rust-lang#104952 (Streamline the user experience for `x.py setup`)
 - rust-lang#104953 (Ensure required submodules at the same time as updating existing submodules)
 - rust-lang#105180 (Use proper HirId for async track_caller attribute check)
 - rust-lang#105222 (std update libc version and freebsd image build dependencies)
 - rust-lang#105223 (suggest parenthesis around ExprWithBlock BinOp ExprWithBlock)
 - rust-lang#105230 (Skip recording resolution for duplicated generic params.)
 - rust-lang#105301 (update Miri)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 78cf0b9 into rust-lang:master Dec 6, 2022
@rustbot rustbot added this to the 1.67.0 milestone Dec 6, 2022
@nbdd0121 nbdd0121 deleted the async_track_caller branch December 6, 2022 01:05
@bryangarza bryangarza mentioned this pull request Dec 6, 2022
Merged
@Swatinem Swatinem mentioned this pull request Dec 6, 2022
Merged
@nbdd0121 nbdd0121 mentioned this pull request Dec 8, 2022
Merged
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Dec 13, 2022
@Dylan-DPC
Rollup merge of rust-lang#105464 - nbdd0121:hir, r=compiler-errors
d5a8be4 
Support #[track_caller] on async closures

Follow up on rust-lang#105180

r? `@compiler-errors`
cc `@cjgillot`
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Dec 13, 2022
@Dylan-DPC
Rollup merge of rust-lang#105464 - nbdd0121:hir, r=compiler-errors
07a7593 
Support #[track_caller] on async closures

Follow up on rust-lang#105180

r? ``@compiler-errors``
cc ``@cjgillot``
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Dec 13, 2022
@matthiaskrgr
Rollup merge of rust-lang#105464 - nbdd0121:hir, r=compiler-errors
5e38e70 
Support #[track_caller] on async closures

Follow up on rust-lang#105180

r? ```@compiler-errors```
cc ```@cjgillot```
Aaron1011 pushed a commit to Aaron1011/rust that referenced this pull request Jan 6, 2023
@matthiaskrgr
Rollup merge of rust-lang#105180 - nbdd0121:async_track_caller, r=com…
d26ac3f 
…piler-errors

Use proper HirId for async track_caller attribute check

Fix rust-lang#105134
Aaron1011 pushed a commit to Aaron1011/rust that referenced this pull request Jan 6, 2023
@bors
Auto merge of rust-lang#105328 - matthiaskrgr:rollup-qnfksmq, r=matth…
d35ad1a 
…iaskrgr

Rollup of 8 pull requests

Successful merges:

 - rust-lang#104912 (PartialEq: PERs are homogeneous)
 - rust-lang#104952 (Streamline the user experience for `x.py setup`)
 - rust-lang#104953 (Ensure required submodules at the same time as updating existing submodules)
 - rust-lang#105180 (Use proper HirId for async track_caller attribute check)
 - rust-lang#105222 (std update libc version and freebsd image build dependencies)
 - rust-lang#105223 (suggest parenthesis around ExprWithBlock BinOp ExprWithBlock)
 - rust-lang#105230 (Skip recording resolution for duplicated generic params.)
 - rust-lang#105301 (update Miri)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bryangarza bryangarza mentioned this pull request Apr 6, 2023
Open
4 tasks
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 left review comments

Assignees

@compiler-errors compiler-errors

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

Successfully merging this pull request may close these issues.

async{} blocks can no longer be used in functions marked as #[track_caller]
7 participants
@nbdd0121 @rustbot @compiler-errors @bors @rust-log-analyzer @cjgillot @jackh726