AsyncDrop trait without sync Drop generates an error #142606
Conversation
rustbot
added
S-waiting-on-review
azhogin
force-pushed
the
azhogin/async-drop-without-sync-drop-error
branch
from
73c7006 to
f1df989
Compare
| if let Some(async_dtor) = adt_async_destructor(tcx, def_id) { | ||
| // When type has AsyncDrop impl, but doesn't have Drop impl, generate error | ||
| let span = tcx.def_span(def_id.to_def_id()); | ||
| let impl_span = tcx.def_span(async_dtor.impl_did); |
There was a problem hiding this comment.
Make this the main span. Unsure if pointing to the type is useful, we could just leave it off entirely
| @@ -46,6 +46,10 @@ hir_analysis_associated_type_trait_uninferred_generic_params = cannot use the {$ | |||
| hir_analysis_associated_type_trait_uninferred_generic_params_multipart_suggestion = use a fully qualified path with explicit lifetimes | |||
| hir_analysis_async_drop_without_sync_drop = `AsyncDrop` impl without `Drop` impl | |||
| .help = type implementing `AsyncDrop` trait must also implement `Drop` trait to be used in sync context and unwinds | |||
There was a problem hiding this comment.
Which "sync contexts" are currently remaining besides unwinds?
Ideally async drop should just always be called, but while it's not it would be good to list the unimplemented cases somewhere.
There was a problem hiding this comment.
Sync context - any sync function, where AsyncDrop type is dropped inside.
fn foo() { _ = Dropee::new(); }, where Dropee has only AsyncDrop trait without Drop trait.
There was a problem hiding this comment.
Ah, I thought cases like this are already reported as errors (they should be).
There was a problem hiding this comment.
Yea that's an alternative to this PR (or that allows reverting this PR): error at use sites of sync drop of types that have no Drop impl but an AsyncDrop impl. This has some fun implications though, as we'd need to make sure that Copy and AsyncDrop also need to be mutually exclusive, just like Copy and Drop are. Or more generally: any logic checking for Drop needs to also check for AsyncDrop and handle that.
So it may be better long term to merge various things like adt_destructor and adt_async_destructor to only return a single Option with an enum inside for either an async or a sync variant, making it easier not to forget to check both together. There are probably many other functions that need similar treatment.
There was a problem hiding this comment.
Well, error at use sites of sync drop may help only after implementing async-drop support for unwinds. Otherwise, we still need both traits.
azhogin
force-pushed
the
azhogin/async-drop-without-sync-drop-error
branch
from
f1df989 to
eee2d7b
Compare
|
@bors r+ rollup |
bors
added
S-waiting-on-bors
…-sync-drop-error, r=oli-obk AsyncDrop trait without sync Drop generates an error When type implements `AsyncDrop` trait, it must also implement sync `Drop` trait to be used in sync context and unwinds. This PR adds error generation in such a case. Fixes: rust-lang#140696
Rollup of 12 pull requests Successful merges: - #135656 (Add `-Z hint-mostly-unused` to tell rustc that most of a crate will go unused) - #140774 (Affirm `-Cforce-frame-pointers=off` does not override) - #141610 (Stabilize `feature(generic_arg_infer)`) - #142123 (Implement initial support for timing sections (`--json=timings`)) - #142383 (CodeGen: rework Aggregate implemention for rvalue_creates_operand cases) - #142502 (rustdoc_json: improve handling of generic args) - #142591 (Add spawn APIs for BootstrapCommand to support deferred command execution) - #142606 (AsyncDrop trait without sync Drop generates an error) - #142619 (apply clippy::or_fun_call) - #142624 (Actually take `--build` into account in bootstrap) - #142627 (Add `StepMetadata` to describe steps) - #142660 (remove joboet from review rotation) r? `@ghost` `@rustbot` modify labels: rollup
When type implements
AsyncDroptrait, it must also implement syncDroptrait to be used in sync context and unwinds.This PR adds error generation in such a case.
Fixes: #140696