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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4c73b64
Use proper HirId for async track_caller attribute check
nbdd0121 Dec 2, 2022
ec4080b
Fix async track caller for assoc fn and trait impl fn
nbdd0121 Dec 2, 2022
34c3773
Add fixme note
nbdd0121 Dec 5, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 18 additions & 14 deletions compiler/rustc_ast_lowering/src/expr.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,6 +147,7 @@ impl<'hir> LoweringContext<'_, 'hir> {
),
ExprKind::Async(capture_clause, closure_node_id, block) => self.make_async_expr(
*capture_clause,
None,
*closure_node_id,
None,
e.span,
Expand Down Expand Up @@ -581,6 +582,7 @@ impl<'hir> LoweringContext<'_, 'hir> {
pub(super) fn make_async_expr(
&mut self,
capture_clause: CaptureBy,
outer_hir_id: Option<hir::HirId>,
closure_node_id: NodeId,
ret_ty: Option<hir::FnRetTy<'hir>>,
span: Span,
Expand Down Expand Up @@ -647,18 +649,20 @@ impl<'hir> LoweringContext<'_, 'hir> {

hir::ExprKind::Closure(c)
};
let parent_has_track_caller = self
.attrs
.values()
.find(|attrs| attrs.into_iter().find(|attr| attr.has_name(sym::track_caller)).is_some())
.is_some();
let unstable_span =
self.mark_span_with_reason(DesugaringKind::Async, span, self.allow_gen_future.clone());

let hir_id = if parent_has_track_caller {
let generator_hir_id = self.lower_node_id(closure_node_id);
let track_caller = outer_hir_id
.and_then(|id| self.attrs.get(&id.local_id))
.map_or(false, |attrs| attrs.into_iter().any(|attr| attr.has_name(sym::track_caller)));

let hir_id = self.lower_node_id(closure_node_id);
if track_caller {
let unstable_span = self.mark_span_with_reason(
DesugaringKind::Async,
span,
self.allow_gen_future.clone(),
);
self.lower_attrs(
generator_hir_id,
hir_id,
&[Attribute {
kind: AttrKind::Normal(ptr::P(NormalAttr {
item: AttrItem {
Expand All @@ -673,10 +677,7 @@ impl<'hir> LoweringContext<'_, 'hir> {
span: unstable_span,
}],
);
generator_hir_id
} else {
self.lower_node_id(closure_node_id)
};
}

let generator = hir::Expr { hir_id, kind: generator_kind, span: self.lower_span(span) };

Expand Down Expand Up @@ -1012,6 +1013,9 @@ 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?

capture_clause,
// FIXME(nbdd0121): This should also use a proper HIR id so `#[track_caller]`
// can be applied on async closures as well.
None,
inner_closure_id,
async_ret_ty,
body.span,
Expand Down
28 changes: 20 additions & 8 deletions compiler/rustc_ast_lowering/src/item.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -268,8 +268,13 @@ impl<'hir> LoweringContext<'_, 'hir> {
// only cares about the input argument patterns in the function
// declaration (decl), not the return types.
let asyncness = header.asyncness;
let body_id =
this.lower_maybe_async_body(span, &decl, asyncness, body.as_deref());
let body_id = this.lower_maybe_async_body(
span,
hir_id,
&decl,
asyncness,
body.as_deref(),
);

let mut itctx = ImplTraitContext::Universal;
let (generics, decl) = this.lower_generics(generics, id, &mut itctx, |this| {
Expand Down Expand Up @@ -766,6 +771,7 @@ impl<'hir> LoweringContext<'_, 'hir> {

fn lower_trait_item(&mut self, i: &AssocItem) -> &'hir hir::TraitItem<'hir> {
let hir_id = self.lower_node_id(i.id);
self.lower_attrs(hir_id, &i.attrs);
let trait_item_def_id = hir_id.expect_owner();

let (generics, kind, has_default) = match &i.kind {
Expand All @@ -789,7 +795,7 @@ impl<'hir> LoweringContext<'_, 'hir> {
AssocItemKind::Fn(box Fn { sig, generics, body: Some(body), .. }) => {
let asyncness = sig.header.asyncness;
let body_id =
self.lower_maybe_async_body(i.span, &sig.decl, asyncness, Some(&body));
self.lower_maybe_async_body(i.span, hir_id, &sig.decl, asyncness, Some(&body));
let (generics, sig) = self.lower_method_sig(
generics,
sig,
Expand Down Expand Up @@ -824,7 +830,6 @@ impl<'hir> LoweringContext<'_, 'hir> {
AssocItemKind::MacCall(..) => panic!("macro item shouldn't exist at this point"),
};

self.lower_attrs(hir_id, &i.attrs);
let item = hir::TraitItem {
owner_id: trait_item_def_id,
ident: self.lower_ident(i.ident),
Expand Down Expand Up @@ -863,6 +868,8 @@ impl<'hir> LoweringContext<'_, 'hir> {
// Since `default impl` is not yet implemented, this is always true in impls.
let has_value = true;
let (defaultness, _) = self.lower_defaultness(i.kind.defaultness(), has_value);
let hir_id = self.lower_node_id(i.id);
self.lower_attrs(hir_id, &i.attrs);

let (generics, kind) = match &i.kind {
AssocItemKind::Const(_, ty, expr) => {
Expand All @@ -875,8 +882,13 @@ impl<'hir> LoweringContext<'_, 'hir> {
AssocItemKind::Fn(box Fn { sig, generics, body, .. }) => {
self.current_item = Some(i.span);
let asyncness = sig.header.asyncness;
let body_id =
self.lower_maybe_async_body(i.span, &sig.decl, asyncness, body.as_deref());
let body_id = self.lower_maybe_async_body(
i.span,
hir_id,
&sig.decl,
asyncness,
body.as_deref(),
);
let (generics, sig) = self.lower_method_sig(
generics,
sig,
Expand Down Expand Up @@ -909,8 +921,6 @@ impl<'hir> LoweringContext<'_, 'hir> {
AssocItemKind::MacCall(..) => panic!("`TyMac` should have been expanded by now"),
};

let hir_id = self.lower_node_id(i.id);
self.lower_attrs(hir_id, &i.attrs);
let item = hir::ImplItem {
owner_id: hir_id.expect_owner(),
ident: self.lower_ident(i.ident),
Expand Down Expand Up @@ -1043,6 +1053,7 @@ impl<'hir> LoweringContext<'_, 'hir> {
fn lower_maybe_async_body(
&mut self,
span: Span,
fn_id: hir::HirId,
decl: &FnDecl,
asyncness: Async,
body: Option<&Block>,
Expand Down Expand Up @@ -1193,6 +1204,7 @@ impl<'hir> LoweringContext<'_, 'hir> {

let async_expr = this.make_async_expr(
CaptureBy::Value,
Some(fn_id),
closure_id,
None,
body.span,
Expand Down
11 changes: 11 additions & 0 deletions src/test/ui/async-await/track-caller/issue-105134.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
// check-pass
// edition:2021

#[track_caller]
fn f() {
let _ = async {};
}

fn main() {
f();
}
14 changes: 14 additions & 0 deletions src/test/ui/async-await/track-caller/panic-track-caller.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,19 @@ async fn foo_track_caller() {
bar_track_caller().await
}

struct Foo;

impl Foo {
#[track_caller]
async fn bar_assoc() {
panic!();
}
}

async fn foo_assoc() {
Foo::bar_assoc().await
}

fn panicked_at(f: impl FnOnce() + panic::UnwindSafe) -> u32 {
let loc = Arc::new(Mutex::new(None));

Expand All @@ -73,4 +86,5 @@ fn panicked_at(f: impl FnOnce() + panic::UnwindSafe) -> u32 {
fn main() {
assert_eq!(panicked_at(|| block_on(foo())), 41);
assert_eq!(panicked_at(|| block_on(foo_track_caller())), 54);
assert_eq!(panicked_at(|| block_on(foo_assoc())), 67);
}