Fix E0040 suggestion for explicit Drop::drop UFCS calls#156103
Fix E0040 suggestion for explicit Drop::drop UFCS calls#156103Unique-Usman wants to merge 1 commit intorust-lang:mainfrom
Drop::drop UFCS calls#156103Conversation
rustbot
added
S-waiting-on-review
|
r? @chenyukang rustbot has assigned @chenyukang. Use Why was this reviewer chosen?The reviewer was selected based on:
|
|
r? @mejrs |
| LL - Drop::drop(&mut Foo) | ||
| LL + drop(&mut Foo) |
There was a problem hiding this comment.
It seems wrong to suggest this. The intent of the user probably is something like "I want to drop the foo" or "I want to run the Drop impl of foo" . Note that these are not the same things; the former calls both foo's Drop impl and its "drop glue".
Meanwhile, calling drop on a reference does nothing. I think it would be better to suggest drop(foo) always.
There was a problem hiding this comment.
This would not be triggered if it is not the Drop from the language i.e the language item, you can check here https://github.com/rust-lang/rust/pull/156103/changes#diff-f6f45f5b3bf300d1d65ce5d2d7a4d0b3ce9addc8166d4b357cd02a6ebaeed4b7L42. And rust does not allow using the Drop from language item. So we are sure that this would not fired if it is not the language item.
Meanwhile, calling drop on a reference does nothing. I think it would be better to suggest drop(foo) always.
Yeah you are right but, if you look at the test file it has this https://github.com/rust-lang/rust/blob/main/tests/ui/drop/explicit-drop-call-error.rs#L5
#![allow(dropping_references)] attribute is used which tells the compiler to not emit the warning showing that the user obviously understand that they are dropping the reference. So, this implementation check whether the attribute is present or not https://github.com/rust-lang/rust/pull/156103/changes#diff-19458ddc56ec4fc74023ef180e074afcf61157f4d4a26d35a31182a7cbcd275aR1022-R1029. If it is present, the reference will be preserved else the reference will be remove just like it did for the test I added which has absent of the attribute https://github.com/rust-lang/rust/pull/156103/changes#diff-40eb4a0c29630863c07759f36d49ee539912fe5d4bfef548bbfc7edfd651312bR2-R11
There was a problem hiding this comment.
But is there a case in which drop(&mut thing) is the right thing to do? I don't think there is, but I could be wrong. It'd be happy to see an example. In my experience, with this particular lint, it pretty much always indicates a bug or at least some misunderstanding.
Also; when that allow was added it was also added to many more files. It looks like they were added so the stderr of those files wouldn't change too much, rather than a conscious decision like "it's OK to drop refs in these cases".
the user obviously understand that they are dropping the reference.
Is that also true if #![allow(warnings)] is being used?
There was a problem hiding this comment.
But is there a case in which
drop(&mut thing)is the right thing to do? I don't think there is, but I could be wrong. It'd be happy to see an example. In my experience, with this particular lint, it pretty much always indicates a bug or at least some misunderstanding.
I also do not know about any case.
Also; when that
allowwas added it was also added to many more files. It looks like they were added so the stderr of those files wouldn't change too much, rather than a conscious decision like "it's OK to drop refs in these cases".the user obviously understand that they are dropping the reference.
Is that also true if
#![allow(warnings)]is being used?
It is only for #![allow(dropping_references)] and to me, the question I asked myself when working on this was why is there there such attribute in the compiler in the first place specifically for allowing dropping references and was added in that file so, that was what made me add the checking. But, I believe we might need to establish that #![allow(dropping_references)] as you said it was to ensure the stderr is would not change too much. If that is the case, then I will make the changes to the pr.
There was a problem hiding this comment.
Is that also true if #![allow(warnings)] is being used?
It is only for #![allow(dropping_references)]
But #![allow(warnings)] implies #![allow(dropping_references)] (as well as every other lint, basically).
why is there there such attribute in the compiler in the first place specifically for allowing dropping references
It exists because dropping_references is a lint (not an error), and so users can use allow/deny/etc to change the lint level of specific lints.
then I will make the changes to the pr.
Yes please :)
rustbot
added
S-waiting-on-author
|
Reminder, once the PR becomes ready for a review, use |
rustbot
added
S-waiting-on-review
| use default_could_be_derived::DefaultCouldBeDerived; | ||
| use deref_into_dyn_supertrait::*; | ||
| use disallowed_pass_by_ref::*; | ||
| pub use drop_forget_useless::DROPPING_REFERENCES; |
There was a problem hiding this comment.
| pub use drop_forget_useless::DROPPING_REFERENCES; |
This public export isn't necessary anymore.
`Drop::drop(&mut f)` now correctly suggests `drop(f)` instead of the bare `drop` with no argument. Signed-off-by: Usman Akinyemi <usmanakinyemi202@gmail.com>
|
@bors r+ rollup |
rust-bors
Bot
added
S-waiting-on-bors
Fix E0040 suggestion for explicit `Drop::drop` UFCS calls `Drop::drop(&mut f)` now correctly suggests `drop(f)` instead of the bare `drop` with no argument. Fix: rust-lang#156017
…uwer Rollup of 12 pull requests Successful merges: - #155848 ([doc]: Revert `core::io::ErrorKind` doc changes) - #155855 (Remove unnecessary `get_unchecked`) - #155543 (docs(unstable-book): Document const generics features) - #155962 (`rustc`: `target_features`: allow for `cfg`-only stable `target_features`) - #156043 (c-variadic: gate `va_arg` on `c_variadic_experimental_arch`) - #156082 (Move tests associated types) - #156092 (Clean up `TyCtxt::needs_crate_hash` usage and rename it to `needs_hir_hash`.) - #156103 (Fix E0040 suggestion for explicit `Drop::drop` UFCS calls) - #156104 (Relax `T: Sized` bound on `try_as_dyn` / `try_as_dyn_mut`) - #156128 (.mailmap: prefer matching just based on commit emails) - #156135 (Remove most uses of def_id_to_node_id) - #156152 (Update books)
Fix E0040 suggestion for explicit `Drop::drop` UFCS calls `Drop::drop(&mut f)` now correctly suggests `drop(f)` instead of the bare `drop` with no argument. Fix: rust-lang#156017
Rollup of 16 pull requests Successful merges: - #155848 ([doc]: Revert `core::io::ErrorKind` doc changes) - #155855 (Remove unnecessary `get_unchecked`) - #155543 (docs(unstable-book): Document const generics features) - #155962 (`rustc`: `target_features`: allow for `cfg`-only stable `target_features`) - #156043 (c-variadic: gate `va_arg` on `c_variadic_experimental_arch`) - #156082 (Move tests associated types) - #156087 (Improve `&pin` reference-pattern suggestions) - #156092 (Clean up `TyCtxt::needs_crate_hash` usage and rename it to `needs_hir_hash`.) - #156103 (Fix E0040 suggestion for explicit `Drop::drop` UFCS calls) - #156104 (Relax `T: Sized` bound on `try_as_dyn` / `try_as_dyn_mut`) - #156122 (Add a `doc_cfg` regression test to ensure foreign types impls are working as expected) - #156128 (.mailmap: prefer matching just based on commit emails) - #156135 (Remove most uses of def_id_to_node_id) - #156152 (Update books) - #156154 (tests: mark import UI tests as check-pass) - #156162 (Update `browser-ui-test` version to `0.23.5`)
4 participants
Drop::drop(&mut f)now correctly suggestsdrop(f)instead of the baredropwith no argument.Fix: #156017