Implement `#[must_not_suspend]` #88865

guswynn · 2021-09-11T17:43:59Z

implements #83310

Some notes on the impl:

The code that searches for the attribute on the ADT is basically copied from the must_use lint. It's not shared, as the logic did diverge
The RFC does specify that the attribute can be placed on fn's (and fn-like objects), like must_use. I think this is a direct copy from the must_use reference definition. This implementation does NOT support this, as I felt that ADT's (+ impl Trait + dyn Trait) cover the usecase's people actually want on the RFC, and adding an imp for the fn call case would be significantly harder. The must_use impl can do a single check at fn call stmt time, but must_not_suspend would need to answer the question: "for some value X with type T, find any fn call that COULD have produced this value". That would require significant changes to generator_interior.rs, and I would need mentorship on that. @eholk and I are discussing it.
@estebank do you know a way I can make the user-provided reason note pop out? right now it seems quite hidden

Also, I am not sure if we should run perf on this

r? @nikomatsakis

compiler/rustc_lint_defs/src/builtin.rs

compiler/rustc_passes/src/check_attr.rs

compiler/rustc_typeck/src/check/generator_interior.rs

src/test/ui/lint/must_not_suspend/boxed.rs

src/test/ui/lint/must_not_suspend/return.rs

src/test/ui/lint/must_not_suspend/unit.stderr

guswynn · 2021-09-11T18:12:25Z

@jyn514 the panic is:

thread 'rustc' panicked at 'Box<dyn Any>', compiler/rustc_errors/src/lib.rs:1147:9
stack backtrace:
   0: std::panicking::begin_panic
   1: std::panic::panic_any
   2: rustc_errors::HandlerInner::bug
   3: rustc_errors::Handler::bug
   4: rustc_middle::util::bug::opt_span_bug_fmt::{{closure}}
   5: rustc_middle::ty::context::tls::with_opt::{{closure}}
   6: rustc_middle::ty::context::tls::with_opt
   7: rustc_middle::util::bug::opt_span_bug_fmt
   8: rustc_middle::util::bug::bug_fmt
   9: rustc_middle::ich::impls_ty::<impl rustc_data_structures::stable_hasher::HashStable<rustc_middle::ich::hcx::StableHashingContext> for rustc_middle::ty::sty::RegionKind>::hash_stable
  10: rustc_middle::ty::sty::_DERIVE_rustc_data_structures_stable_hasher_HashStable_rustc_middle_ich_StableHashingContext_ctx_FOR_TyKind::<impl rustc_data_structures::stable_hasher::HashStable<rustc_middle::ich::hcx::StableHashingContext> for rustc_middle::ty::sty::TyKind>::hash_stable
  11: <T as rustc_query_system::dep_graph::dep_node::DepNodeParams<Ctxt>>::to_fingerprint
  12: rustc_query_system::dep_graph::dep_node::DepNode<K>::construct
  13: rustc_query_system::query::plumbing::get_query_impl
  14: rustc_query_system::query::plumbing::get_query
  15: <rustc_query_impl::Queries as rustc_middle::ty::query::QueryEngine>::type_uninhabited_from
  16: rustc_middle::ty::inhabitedness::<impl rustc_middle::ty::TyS>::uninhabited_from
  17: rustc_middle::ty::inhabitedness::<impl rustc_middle::ty::context::TyCtxt>::is_ty_uninhabited_from
  18: rustc_typeck::check::generator_interior::check_must_not_suspend_ty
  19: rustc_typeck::check::generator_interior::InteriorVisitor::record
  20: <rustc_typeck::check::generator_interior::InteriorVisitor as rustc_hir::intravisit::Visitor>::visit_pat
  21: rustc_hir::intravisit::walk_block
  22: <rustc_typeck::check::generator_interior::InteriorVisitor as rustc_hir::intravisit::Visitor>::visit_expr
  23: rustc_typeck::check::generator_interior::resolve_interior
  24: rustc_typeck::check::fn_ctxt::_impl::<impl rustc_typeck::check::fn_ctxt::FnCtxt>::resolve_generator_interiors
  25: rustc_infer::infer::InferCtxtBuilder::enter
  ...

Do you know what that could be?

guswynn · 2021-09-11T19:07:35Z

Seems like the is_ty_uninhabitated_from check, which was copied from the #[must_use] check, is a query only really available AFTER typeck, as it required that ReVar and RePlaceholder are purged.

I am not sure what this does, and if its needed for this lint, the last person to touch it seems to be @oli-obk in 7894509 ?

guswynn · 2021-09-11T19:26:13Z

this pr should be reviewable, except for I am unsure about if the is_ty_uninhabitated check is needed, currently its off

compiler/rustc_typeck/src/check/generator_interior.rs

compiler/rustc_feature/src/active.rs

compiler/rustc_lint_defs/src/builtin.rs

compiler/rustc_typeck/src/check/generator_interior.rs

estebank · 2021-09-13T09:46:01Z

do you know a way I can make the user-provided reason note pop out? right now it seems quite hidden

There are a few things I do to make important info appear more obvious:

make them part of the primary label, instead of using a note, use a span_label
- it might make sense to make must_not_suspend have a way to be used as #[must_not_suspend(label = "fooberize this", note = "foos must bar")
use a span_note to make subdiagnostic more obvious, this also gives the header (note/help) a color, which we don't show when they are shown as a single line
move the note earlier or later, they are harder to read in the middle

Sadly this is about as much as we can accomplish with the APIs we have today.

compiler/rustc_typeck/src/check/generator_interior.rs

guswynn · 2021-09-13T14:59:50Z

@estebank
(edit)

I tried some options, I think note is the best we have right now

estebank · 2021-09-14T07:02:49Z

@guswynn let's make a note (pun intended) to fix it later.

nikomatsakis · 2021-09-15T16:37:52Z

I think I'd prefer to have @estebank or @oli-obk review this! I'm going to put

r? @oli-obk

since it feels pretty close to clippy lint territory. =)

eholk

Do you have any test cases that make sure dropping a must_not_suspend value before a suspend point works as expected? I suspect we may have some trouble with this until I finish the liveness-based analysis for generators.

compiler/rustc_lint_defs/src/builtin.rs

compiler/rustc_typeck/src/check/generator_interior.rs

guswynn · 2021-09-15T18:27:23Z

@eholk good point, it looks like drop doesn't work, but the scope does, so I'll narrow the suggestion for now

oli-obk · 2021-09-16T07:22:23Z

compiler/rustc_typeck/src/check/generator_interior.rs

+// Returns whether it emitted a diagnostic or not
+// Note that this fn and the proceding one are based on the code
+// for creating must_use diagnostics
+pub fn check_must_not_suspend_ty<'tcx>(


The complexity of this function seems ~okay to me, but I'm still wondering if the (discussed and dismissed in the RFC) marker trait (Suspend?) approach would work better. The attribute could impl !Suspend for structs, add it as a supertrait for traits and then the one caller to check_must_not_suspend_ty could make the InferCtxt add an obligation for Suspend to exist.

This would require some extra logic to make that obligation failure a lint instead of an error, so if we don't already have something like this, we should keep doing what we're doing here. Maybe just add the above paragraph as a comment for why we are implementing it this way?

Is the choice basically "we decided against this in the RFC"?

it was never discussed in the RFC except for an honorable mention here

oli-obk · 2021-09-16T07:28:03Z

One additional test that I could think of would be a generic function that holds a generic argument over an await point and a caller to that generic function that uses a must_not_suspend type for the generic argument. Obviously this can't lint within the generic function, but we could probably inspect the generator created by the generic async function to see whether it stores said argument or whether it just uses it before its first resume.

basically

#[must_not_suspend]
struct No;

async fn shushspend() {}

async fn wheeee<T>(t: T) {
    shushspend().await;
    drop(t);
}

async fn yes() {
    wheeee(No).await;
}

guswynn · 2021-09-18T19:23:21Z

@oli-obk interestingly your example actually triggers the lint fine (although, its not perfectly formatted, it doubles up just like the reference case)

guswynn · 2021-09-18T19:49:08Z

actually, no its triggering on the whee yield point, ill update the test to show whats going on

guswynn · 2021-09-18T20:01:37Z

Added deduplication logic, currently at the hirid level, let me know what you think about what the ref test looks like now (It triggers now only on the value being borrowed, not the reference variable

oli-obk · 2021-09-19T10:25:41Z

actually, no its triggering on the whee yield point, ill update the test to show whats going on

yea, I expected it to trigger in yes, while pointing into wheeee with secondary spans can be useful, that function may be in a different crate, so we should not put too much effort into things

oli-obk · 2021-09-19T10:34:07Z

src/test/ui/lint/must_not_suspend/dedup.stderr

+help: consider using a block (`{ ... }`) to shrink the value's scope, ending before the suspend point
+  --> $DIR/dedup.rs:16:12
+   |
+LL |     wheeee(No {}).await;
+   |            ^^^^^


this only makes sense where the value is held locally. If it was found in anther generator, then we should probably not emit this help

this is triggering on the No { } value being held across the whee yield point, I think? I believe that @eholk is improving this analysis

oli-obk · 2021-09-19T10:37:36Z

@bors r+

bors · 2021-09-19T10:37:37Z

📌 Commit 08e0266 has been approved by oli-obk

bors · 2021-09-22T06:43:36Z

⌛ Testing commit 08e0266 with merge ce45663...

bors · 2021-09-22T09:32:21Z

☀️ Test successful - checks-actions
Approved by: oli-obk
Pushing ce45663 to master...

rust-timer · 2021-09-22T11:32:07Z

Finished benchmarking commit (ce45663): comparison url.

Summary: This benchmark run did not return any relevant changes.

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

nayato · 2021-10-11T20:55:08Z

@eholk good point, it looks like drop doesn't work, but the scope does, so I'll narrow the suggestion for now

@guswynn, Does that mean that warning should not fire when drop is used?
I am seeing on current nightly that if Ref dropped warning for must_not_suspend is issued.

EDIT: I see there's #89562 for this already.

guswynn · 2021-10-11T20:58:50Z

@nayato The noise caused by drop false positives is being dealt with by making the lint allow-by-default until we fix the analysis: #89787

rust-highfive assigned nikomatsakis Sep 11, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 11, 2021

must_not_suspend impl

2271376

guswynn force-pushed the must_not_suspend branch from cf1d0ec to 2271376 Compare September 11, 2021 17:49

remove attempt at fn call

67ee91e

jyn514 reviewed Sep 11, 2021

View reviewed changes

This comment has been minimized.

Sign in to view

skip the uninhabitated check and comments

74ea163

guswynn force-pushed the must_not_suspend branch from 30c2037 to 74ea163 Compare September 11, 2021 19:10

guswynn marked this pull request as ready for review September 11, 2021 19:10

array comment + test for references

461a0f3

fix doctests

ee1d2ea

This comment has been minimized.

Sign in to view

camelid reviewed Sep 11, 2021

View reviewed changes

estebank reviewed Sep 13, 2021

View reviewed changes

compiler/rustc_lint_defs/src/builtin.rs Outdated Show resolved Hide resolved

estebank reviewed Sep 13, 2021

View reviewed changes

compiler/rustc_typeck/src/check/generator_interior.rs Show resolved Hide resolved

oli-obk reviewed Sep 13, 2021

View reviewed changes

compiler/rustc_typeck/src/check/generator_interior.rs Show resolved Hide resolved

oli-obk reviewed Sep 13, 2021

View reviewed changes

compiler/rustc_typeck/src/check/generator_interior.rs Show resolved Hide resolved

oli-obk reviewed Sep 13, 2021

View reviewed changes

compiler/rustc_typeck/src/check/generator_interior.rs Outdated Show resolved Hide resolved

guswynn mentioned this pull request Sep 13, 2021

ensure that the lint level is defined here is always the last note #88908

Open

error formatting and fix build

2af1ebf

apiraino added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Sep 13, 2021

rust-highfive unassigned nikomatsakis Sep 15, 2021

rust-highfive assigned oli-obk Sep 15, 2021

eholk reviewed Sep 15, 2021

View reviewed changes

compiler/rustc_lint_defs/src/builtin.rs Outdated Show resolved Hide resolved

compiler/rustc_typeck/src/check/generator_interior.rs Outdated Show resolved Hide resolved

compiler/rustc_typeck/src/check/generator_interior.rs Outdated Show resolved Hide resolved

factor into struct, and comments

110aecd

oli-obk reviewed Sep 16, 2021

View reviewed changes

generic test

f1021bf

deduplication

08e0266

oli-obk reviewed Sep 19, 2021

View reviewed changes

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 Sep 19, 2021

bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 22, 2021

bors merged commit ce45663 into rust-lang:master Sep 22, 2021

rustbot added this to the 1.57.0 milestone Sep 22, 2021

This was referenced Sep 22, 2021

Check for duplicate attributes. #88681

Merged

Support #[track_caller] on closures and generators #87064

Merged

Automatically implement AsRepr and allow deriving FromRepr for fieldless enums #81642

Closed

guswynn deleted the must_not_suspend branch September 26, 2021 18:51

tmandry mentioned this pull request Feb 15, 2022

Tracking Issue for must_not_suspend lint (RFC #3014) #83310

Open

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement `#[must_not_suspend]` #88865

Implement `#[must_not_suspend]` #88865

guswynn commented Sep 11, 2021 •

edited

Loading

This comment has been minimized.

guswynn commented Sep 11, 2021

guswynn commented Sep 11, 2021

guswynn commented Sep 11, 2021

This comment has been minimized.

estebank commented Sep 13, 2021

guswynn commented Sep 13, 2021 •

edited

Loading

estebank commented Sep 14, 2021

nikomatsakis commented Sep 15, 2021

eholk left a comment

guswynn commented Sep 15, 2021

oli-obk Sep 16, 2021

guswynn Sep 16, 2021

oli-obk Sep 16, 2021

oli-obk commented Sep 16, 2021

guswynn commented Sep 18, 2021

guswynn commented Sep 18, 2021

guswynn commented Sep 18, 2021

oli-obk commented Sep 19, 2021

oli-obk Sep 19, 2021

guswynn Sep 20, 2021

oli-obk commented Sep 19, 2021

bors commented Sep 19, 2021

bors commented Sep 22, 2021

bors commented Sep 22, 2021

rust-timer commented Sep 22, 2021

nayato commented Oct 11, 2021 •

edited

Loading

guswynn commented Oct 11, 2021

Implement #[must_not_suspend] #88865

Implement #[must_not_suspend] #88865

Conversation

guswynn commented Sep 11, 2021 • edited Loading

This comment has been minimized.

guswynn commented Sep 11, 2021

guswynn commented Sep 11, 2021

guswynn commented Sep 11, 2021

This comment has been minimized.

estebank commented Sep 13, 2021

guswynn commented Sep 13, 2021 • edited Loading

estebank commented Sep 14, 2021

nikomatsakis commented Sep 15, 2021

eholk left a comment

Choose a reason for hiding this comment

guswynn commented Sep 15, 2021

oli-obk Sep 16, 2021

Choose a reason for hiding this comment

guswynn Sep 16, 2021

Choose a reason for hiding this comment

oli-obk Sep 16, 2021

Choose a reason for hiding this comment

oli-obk commented Sep 16, 2021

guswynn commented Sep 18, 2021

guswynn commented Sep 18, 2021

guswynn commented Sep 18, 2021

oli-obk commented Sep 19, 2021

oli-obk Sep 19, 2021

Choose a reason for hiding this comment

guswynn Sep 20, 2021

Choose a reason for hiding this comment

oli-obk commented Sep 19, 2021

bors commented Sep 19, 2021

bors commented Sep 22, 2021

bors commented Sep 22, 2021

rust-timer commented Sep 22, 2021

nayato commented Oct 11, 2021 • edited Loading

guswynn commented Oct 11, 2021

Implement `#[must_not_suspend]` #88865

Implement `#[must_not_suspend]` #88865

guswynn commented Sep 11, 2021 •

edited

Loading

guswynn commented Sep 13, 2021 •

edited

Loading

nayato commented Oct 11, 2021 •

edited

Loading