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.

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

Implement #[must_not_suspend] #88865

Merged
merged 9 commits into from
Sep 22, 2021
Merged

Implement #[must_not_suspend] #88865

merged 9 commits into from
Sep 22, 2021

Conversation

guswynn
Copy link
Contributor

@guswynn guswynn commented Sep 11, 2021

implements #83310

Some notes on the impl:

  1. 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
  2. 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.
  3. @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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 11, 2021
compiler/rustc_lint_defs/src/builtin.rs Outdated Show resolved Hide resolved
compiler/rustc_passes/src/check_attr.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
src/test/ui/lint/must_not_suspend/boxed.rs Outdated Show resolved Hide resolved
src/test/ui/lint/must_not_suspend/return.rs Outdated Show resolved Hide resolved
src/test/ui/lint/must_not_suspend/return.rs Show resolved Hide resolved
src/test/ui/lint/must_not_suspend/unit.stderr Outdated Show resolved Hide resolved
@rust-log-analyzer

This comment has been minimized.

@guswynn
Copy link
Contributor Author

guswynn commented Sep 11, 2021

@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
Copy link
Contributor Author

guswynn commented Sep 11, 2021

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 guswynn marked this pull request as ready for review September 11, 2021 19:10
@guswynn
Copy link
Contributor Author

guswynn commented Sep 11, 2021

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

@rust-log-analyzer

This comment has been minimized.

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
compiler/rustc_typeck/src/check/generator_interior.rs Outdated Show resolved Hide resolved
compiler/rustc_feature/src/active.rs Outdated Show resolved Hide resolved
@estebank
Copy link
Contributor

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.

@guswynn
Copy link
Contributor Author

guswynn commented Sep 13, 2021

@estebank
(edit)

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

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

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

@nikomatsakis
Copy link
Contributor

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. =)

Copy link
Contributor

@eholk eholk left a comment

Choose a reason for hiding this comment

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

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 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
@guswynn
Copy link
Contributor Author

guswynn commented Sep 15, 2021

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

// 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>(
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

@oli-obk
Copy link
Contributor

oli-obk commented Sep 16, 2021

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
Copy link
Contributor Author

guswynn commented Sep 18, 2021

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

@guswynn
Copy link
Contributor Author

guswynn commented Sep 18, 2021

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

@guswynn
Copy link
Contributor Author

guswynn commented Sep 18, 2021

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
Copy link
Contributor

oli-obk commented Sep 19, 2021

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

Comment on lines +12 to +16
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;
| ^^^^^
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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
Copy link
Contributor

oli-obk commented Sep 19, 2021

@bors r+

@bors
Copy link
Contributor

bors commented Sep 19, 2021

📌 Commit 08e0266 has been approved by oli-obk

@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 Sep 19, 2021
@bors
Copy link
Contributor

bors commented Sep 22, 2021

⌛ Testing commit 08e0266 with merge ce45663...

@bors
Copy link
Contributor

bors commented Sep 22, 2021

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

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 22, 2021
@bors bors merged commit ce45663 into rust-lang:master Sep 22, 2021
@rustbot rustbot added this to the 1.57.0 milestone Sep 22, 2021
@rust-timer
Copy link
Collaborator

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

@guswynn guswynn deleted the must_not_suspend branch September 26, 2021 18:51
@nayato
Copy link

nayato commented Oct 11, 2021

@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
Copy link
Contributor Author

guswynn commented Oct 11, 2021

@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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.