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

Make unconditional_recursion warning detect recursive drops #113902

Merged
merged 3 commits into from Aug 7, 2023

Conversation

Enselic
Copy link
Member

@Enselic Enselic commented Jul 20, 2023

Closes #55388

Also closes #50049 unless we want to keep it for the second example which this PR does not solve, but I think it is better to track that work in #57965.

r? @oli-obk since you are the mentor for #55388

Unresolved questions:

  • There are two false positives that must be fixed before merging (see diff). I suspect the best way to solve them is to perform analysis after drop elaboration instead of before, as now, but I have not explored that any further yet. Could that be an option? Answer: Yes, that solved the problem.

@rustbot label +T-compiler +C-enhancement +A-lint

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. C-enhancement Category: An issue proposing an enhancement or a PR with one. labels Jul 20, 2023
@rust-log-analyzer

This comment has been minimized.

@Enselic
Copy link
Member Author

Enselic commented Jul 21, 2023

I'll explore ways to fix the false positives before I send this to review. (Guidance on how to do it still appreciated.)

@rustbot author

Edit: I think I have something that works, I just need to clean up the code

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 21, 2023
@rustbot
Copy link
Collaborator

rustbot commented Jul 22, 2023

These commits modify the Cargo.lock file. Unintentional changes to Cargo.lock can be introduced when switching branches and rebasing PRs.

If this was unintentional then you should revert the changes before this PR is merged.
Otherwise, you can ignore this comment.

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

@Enselic
Copy link
Member Author

Enselic commented Jul 22, 2023

Moving the check for drop recursion until after drop elaboration indeed fixed the false positives. So I consider this ready for review now.

We probably want to do a perf run? Probably best to wait until after at least an initial review round. Maybe we want to refactor the code to avoid rustc_mir_transform depending on rustc_mir_build? I would be happy to do that. Just let me know.

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 22, 2023
@cjgillot
Copy link
Contributor

mir_drops_elaborated_and_const_checked does not run on all MIR bodies, so moving the lint there will cause it not to run. In particular in check builds on non-generic non-inline functions.

@cjgillot
Copy link
Contributor

That said, I don't see how else run this lint after drop-elaboration.

@oli-obk oli-obk added needs-fcp This change is insta-stable, so needs a completed FCP to proceed. I-lang-nominated The issue / PR has been nominated for discussion during a lang team meeting. labels Jul 25, 2023
@oli-obk
Copy link
Contributor

oli-obk commented Jul 25, 2023

Nominating for lang team discussion. This extends the unconditional_recursion lint to also detect when a Drop impl causes values of the same type getting dropped unconditionally. The one caveat being, that this is only reported in full builds, not in check builds. The existing lint is unaffected.

@Enselic
Copy link
Member Author

Enselic commented Jul 25, 2023

(Just so we all are on the same page: that phenomenon is old and not introduced by this PR. See #49292)

@joshtriplett
Copy link
Member

@rfcbot merge

@nikomatsakis
Copy link
Contributor

@rfcbot fcp merge

Sounds good, starting FCP!

@nikomatsakis
Copy link
Contributor

@rustbot labels -I-lang-nominated

@rustbot rustbot added T-lang Relevant to the language team, which will review and decide on the PR/issue. and removed T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jul 25, 2023
@pnkfelix
Copy link
Member

@rfcbot fcp merge

(see above, I'm just making it happen)

@rfcbot
Copy link

rfcbot commented Jul 25, 2023

Team member @pnkfelix has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns.
See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Jul 25, 2023
@pnkfelix
Copy link
Member

(once T-lang finishes FCP, we can relabel this back to T-compiler again)

@scottmcm
Copy link
Member

Personally, I'm happy for the lint to detect all cases of unconditional recursion, wherever or whenever, without extra lang confirmation. (I see lang as saying "yes, unconditional recursion is worth linting about, and compiler reviewers as sufficient for expanding that to catch more places of the same thing.)

The check/build difference is a shame, but false negatives are normal for lints, so I don't see it as a big issue. (Unlike for errors.)

@rfcbot reviewed

@oli-obk
Copy link
Contributor

oli-obk commented Jul 28, 2023

I checked niko's and josh's box as both of them tried to fcp merge this PR: #113902 (comment) and #113902 (comment)

@rfcbot rfcbot added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels Jul 28, 2023
@rfcbot
Copy link

rfcbot commented Jul 28, 2023

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. to-announce Announce this issue on triage meeting and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Aug 7, 2023
@rfcbot
Copy link

rfcbot commented Aug 7, 2023

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

@oli-obk
Copy link
Contributor

oli-obk commented Aug 7, 2023

@bors r+

@bors
Copy link
Contributor

bors commented Aug 7, 2023

📌 Commit b4b33df has been approved by oli-obk

It is now in the queue for this repository.

@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 Aug 7, 2023
@bors
Copy link
Contributor

bors commented Aug 7, 2023

⌛ Testing commit b4b33df with merge 84ec263...

@bors
Copy link
Contributor

bors commented Aug 7, 2023

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

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 7, 2023
@bors bors merged commit 84ec263 into rust-lang:master Aug 7, 2023
12 checks passed
@rustbot rustbot added this to the 1.73.0 milestone Aug 7, 2023
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (84ec263): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
3.2% [3.2%, 3.2%] 1
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 3.2% [3.2%, 3.2%] 1

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
2.1% [2.1%, 2.1%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) - - 0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 652.316s -> 650.87s (-0.22%)

@Enselic Enselic deleted the lint-recursive-drop branch August 8, 2023 03:43
@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Aug 10, 2023
wip-sync pushed a commit to NetBSD/pkgsrc-wip that referenced this pull request Oct 6, 2023
Language
--------

- [Uplift `clippy::fn_null_check` lint as `useless_ptr_null_checks`.]
  (rust-lang/rust#111717)
- [Make `noop_method_call` warn by default.]
  (rust-lang/rust#111916)
- [Support interpolated block for `try` and `async` in macros.]
  (rust-lang/rust#112953)
- [Make `unconditional_recursion` lint detect recursive drops.]
  (rust-lang/rust#113902)
- [Future compatibility warning for some impls being incorrectly
  considered not overlapping.]
  (rust-lang/rust#114023)
- [The `invalid_reference_casting` lint is now **deny-by-default**
  (instead of allow-by-default)]
  (rust-lang/rust#112431)

Compiler
--------

- [Write version information in a `.comment` section like GCC/Clang.]
  (rust-lang/rust#97550)
- [Add documentation on v0 symbol mangling.]
  (rust-lang/rust#97571)
- [Stabilize `extern "thiscall"` and `"thiscall-unwind"` ABIs.]
  (rust-lang/rust#114562)
- [Only check outlives goals on impl compared to trait.]
  (rust-lang/rust#109356)
- [Infer type in irrefutable slice patterns with fixed length as array.]
  (rust-lang/rust#113199)
- [Discard default auto trait impls if explicit ones exist.]
  (rust-lang/rust#113312)
- Add several new tier 3 targets:
    - [`aarch64-unknown-teeos`]
      (rust-lang/rust#113480)
    - [`csky-unknown-linux-gnuabiv2`]
      (rust-lang/rust#113658)
    - [`riscv64-linux-android`]
      (rust-lang/rust#112858)
    - [`riscv64gc-unknown-hermit`]
      (rust-lang/rust#114004)
    - [`x86_64-unikraft-linux-musl`]
      (rust-lang/rust#113411)
    - [`x86_64-unknown-linux-ohos`]
      (rust-lang/rust#113061)
- [Add `wasm32-wasi-preview1-threads` as a tier 2 target.]
  (rust-lang/rust#112922)

Refer to Rust's [platform support page][platform-support-doc]
for more information on Rust's tiered platform support.

Libraries
---------

- [Add `Read`, `Write` and `Seek` impls for `Arc<File>`.]
  (rust-lang/rust#94748)
- [Merge functionality of `io::Sink` into `io::Empty`.]
  (rust-lang/rust#98154)
- [Implement `RefUnwindSafe` for `Backtrace`]
  (rust-lang/rust#100455)
- [Make `ExitStatus` implement `Default`]
  (rust-lang/rust#106425)
- [`impl SliceIndex<str> for (Bound<usize>, Bound<usize>)`]
  (rust-lang/rust#111081)
- [Change default panic handler message format.]
  (rust-lang/rust#112849)
- [Cleaner `assert_eq!` & `assert_ne!` panic messages.]
  (rust-lang/rust#111071)
- [Correct the (deprecated) Android `stat` struct definitions.]
  (rust-lang/rust#113130)

Stabilized APIs
---------------

- [Unsigned `{integer}::div_ceil`]
  (https://doc.rust-lang.org/stable/std/primitive.u32.html#method.div_ceil)
- [Unsigned `{integer}::next_multiple_of`]
  (https://doc.rust-lang.org/stable/std/primitive.u32.html#method.next_multiple_of)
- [Unsigned `{integer}::checked_next_multiple_of`]
  (https://doc.rust-lang.org/stable/std/primitive.u32.html#method.checked_next_multiple_of)
- [`std::ffi::FromBytesUntilNulError`]
  (https://doc.rust-lang.org/stable/std/ffi/struct.FromBytesUntilNulError.html)
- [`std::os::unix::fs::chown`]
  (https://doc.rust-lang.org/stable/std/os/unix/fs/fn.chown.html)
- [`std::os::unix::fs::fchown`]
  (https://doc.rust-lang.org/stable/std/os/unix/fs/fn.fchown.html)
- [`std::os::unix::fs::lfchown`]
  (https://doc.rust-lang.org/stable/std/os/unix/fs/fn.lchown.html)
- [`LocalKey::<Cell<T>>::get`]
  (https://doc.rust-lang.org/stable/std/thread/struct.LocalKey.html#method.get)
- [`LocalKey::<Cell<T>>::set`]
  (https://doc.rust-lang.org/stable/std/thread/struct.LocalKey.html#method.set)
- [`LocalKey::<Cell<T>>::take`]
  (https://doc.rust-lang.org/stable/std/thread/struct.LocalKey.html#method.take)
- [`LocalKey::<Cell<T>>::replace`]
  (https://doc.rust-lang.org/stable/std/thread/struct.LocalKey.html#method.replace)
- [`LocalKey::<RefCell<T>>::with_borrow`]
  (https://doc.rust-lang.org/stable/std/thread/struct.LocalKey.html#method.with_borrow)
- [`LocalKey::<RefCell<T>>::with_borrow_mut`]
  (https://doc.rust-lang.org/stable/std/thread/struct.LocalKey.html#method.with_borrow_mut)
- [`LocalKey::<RefCell<T>>::set`]
  (https://doc.rust-lang.org/stable/std/thread/struct.LocalKey.html#method.set-1)
- [`LocalKey::<RefCell<T>>::take`]
  (https://doc.rust-lang.org/stable/std/thread/struct.LocalKey.html#method.take-1)
- [`LocalKey::<RefCell<T>>::replace`]
  (https://doc.rust-lang.org/stable/std/thread/struct.LocalKey.html#method.replace-1)

These APIs are now stable in const contexts:

- [`rc::Weak::new`]
  (https://doc.rust-lang.org/stable/alloc/rc/struct.Weak.html#method.new)
- [`sync::Weak::new`]
  (https://doc.rust-lang.org/stable/alloc/sync/struct.Weak.html#method.new)
- [`NonNull::as_ref`]
  (https://doc.rust-lang.org/stable/core/ptr/struct.NonNull.html#method.as_ref)

Cargo
-----

- [Encode URL params correctly for `SourceId` in `Cargo.lock`.]
  (rust-lang/cargo#12280)
- [Bail out an error when using `cargo::` in custom build script.]
  (rust-lang/cargo#12332)

Compatibility Notes
-------------------

- [Update the minimum external LLVM to 15.]
  (rust-lang/rust#114148)
- [Check for non-defining uses of return position `impl Trait`.]
  (rust-lang/rust#112842)

Internal Changes
----------------

These changes do not affect any public interfaces of Rust, but they represent
significant improvements to the performance or internals of rustc and related
tools.

- [Remove LLVM pointee types, supporting only opaque pointers.]
  (rust-lang/rust#105545)
- [Port PGO/LTO/BOLT optimized build pipeline to Rust.]
  (rust-lang/rust#112235)
- [Replace in-tree `rustc_apfloat` with the new version of the crate.]
  (rust-lang/rust#113843)
- [Update to LLVM 17.]
  (rust-lang/rust#114048)
- [Add `internal_features` lint for internal unstable features.]
  (rust-lang/rust#108955)
- [Mention style for new syntax in tracking issue template.]
  (rust-lang/rust#113586)
netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this pull request Nov 16, 2023
Pkgsrc changes:
 * Adjust patches and cargo checksums to new versions.
 * For an external LLVM, set dependency of llvm >= 15, in accordance
   with the upstream changes.
 * Add a patch with a backport from LLVM 17.0.3 fixing codegen for
   PPC, ref. rust-lang/rust#116845

Upstream changes:

Version 1.73.0 (2023-10-05)
==========================

Language
--------

- [Uplift `clippy::fn_null_check` lint as `useless_ptr_null_checks`.]
  (rust-lang/rust#111717)
- [Make `noop_method_call` warn by default.]
  (rust-lang/rust#111916)
- [Support interpolated block for `try` and `async` in macros.]
  (rust-lang/rust#112953)
- [Make `unconditional_recursion` lint detect recursive drops.]
  (rust-lang/rust#113902)
- [Future compatibility warning for some impls being incorrectly
  considered not overlapping.]
  (rust-lang/rust#114023)
- [The `invalid_reference_casting` lint is now **deny-by-default**
  (instead of allow-by-default)]
  (rust-lang/rust#112431

Compiler
--------

- [Write version information in a `.comment` section like GCC/Clang.]
  (rust-lang/rust#97550)
- [Add documentation on v0 symbol mangling.]
  (rust-lang/rust#97571)
- [Stabilize `extern "thiscall"` and `"thiscall-unwind"` ABIs.]
  (rust-lang/rust#114562)
- [Only check outlives goals on impl compared to trait.]
  (rust-lang/rust#109356)
- [Infer type in irrefutable slice patterns with fixed length as array.]
  (rust-lang/rust#113199)
- [Discard default auto trait impls if explicit ones exist.]
  (rust-lang/rust#113312)
- Add several new tier 3 targets:
    - [`aarch64-unknown-teeos`]
      (rust-lang/rust#113480)
    - [`csky-unknown-linux-gnuabiv2`]
      (rust-lang/rust#113658)
    - [`riscv64-linux-android`]
      (rust-lang/rust#112858)
    - [`riscv64gc-unknown-hermit`]
      (rust-lang/rust#114004)
    - [`x86_64-unikraft-linux-musl`]
      (rust-lang/rust#113411)
    - [`x86_64-unknown-linux-ohos`]
      (rust-lang/rust#113061)
- [Add `wasm32-wasi-preview1-threads` as a tier 2 target.]
  (rust-lang/rust#112922)

Refer to Rust's [platform support page][platform-support-doc]
for more information on Rust's tiered platform support.

Libraries
---------

- [Add `Read`, `Write` and `Seek` impls for `Arc<File>`.]
  (rust-lang/rust#94748)
- [Merge functionality of `io::Sink` into `io::Empty`.]
  (rust-lang/rust#98154)
- [Implement `RefUnwindSafe` for `Backtrace`]
  (rust-lang/rust#100455)
- [Make `ExitStatus` implement `Default`]
  (rust-lang/rust#106425)
- [`impl SliceIndex<str> for (Bound<usize>, Bound<usize>)`]
  (rust-lang/rust#111081)
- [Change default panic handler message format.]
  (rust-lang/rust#112849)
- [Cleaner `assert_eq!` & `assert_ne!` panic messages.]
  (rust-lang/rust#111071)
- [Correct the (deprecated) Android `stat` struct definitions.]
  (rust-lang/rust#113130)

Stabilized APIs
---------------

- [Unsigned `{integer}::div_ceil`]
  (https://doc.rust-lang.org/stable/std/primitiv e.u32.html#method.div_ceil)
- [Unsigned `{integer}::next_multiple_of`]
  (https://doc.rust-lang.org/stable/std/primitive.u32.html#method.next_multiple_of)
- [Unsigned `{integer}::checked_next_multiple_of`]
  (https://doc.rust-lang.org/stable/std/primitive.u32.html#method.checked_next_multiple_of)
- [`std::ffi::FromBytesUntilNulError`]
  (https://doc.rust-lang.org/stable/std/ffi/struct.FromBytesUntilNulError.html)
- [`std::os::unix::fs::chown`]
  (https://doc.rust-lang.org/stable/std/os/unix/fs/fn.chown.html)
- [`std::os::unix::fs::fchown`]
  (https://doc.rust-lang.org/stable/std/os/unix/fs/fn.fchown.html)
- [`std::os::unix::fs::lfchown`]
  (https://doc.rust-lang.org/stable/std/os/unix/fs/fn.lchown.html)
- [`LocalKey::<Cell<T>>::get`]
  (https://doc.rust-lang.org/stable/std/thread/struct.LocalKey.html#method.get)
- [`LocalKey::<Cell<T>>::set`]
  (https://doc.rust-lang.org/stable/std/thread/struct.LocalKey.html#method.set)
- [`LocalKey::<Cell<T>>::take`]
  (https://doc.rust-lang.org/stable/std/thread/struct.LocalKey.html#method.take)
- [`LocalKey::<Cell<T>>::replace`]
  (https://doc.rust-lang.org/stable/std/thread/struct.LocalKey.html#method.replace)
- [`LocalKey::<RefCell<T>>::with_borrow`]
  (https://doc.rust-lang.org/stable/std/thread/struct.LocalKey.html#method.with_borrow)
- [`LocalKey::<RefCell<T>>::with_borrow_mut`]
  (https://doc.rust-lang.org/stable/std/thread/struct.LocalKey.html#method.with_borrow_mut)
- [`LocalKey::<RefCell<T>>::set`]
  (https://doc.rust-lang.org/stable/std/thread/struct.LocalKey.html#method.set-1)
- [`LocalKey::<RefCell<T>>::take`]
  (https://doc.rust-lang.org/stable/std/thread/struct.LocalKey.html#method.take-1)
- [`LocalKey::<RefCell<T>>::replace`]
  (https://doc.rust-lang.org/stable/std/thread/struct.LocalKey.html#method.replace-1)

These APIs are now stable in const contexts:

- [`rc::Weak::new`]
  (https://doc.rust-lang.org/stable/alloc/rc/struct.Weak.html#method.new)
- [`sync::Weak::new`]
  (https://doc.rust-lang.org/stable/alloc/sync/struct.Weak.html#method.new)
- [`NonNull::as_ref`]
  (https://doc.rust-lang.org/stable/core/ptr/struct.NonNull.html#method.as_ref)

Cargo
-----

- [Encode URL params correctly for `SourceId` in `Cargo.lock`.]
  (rust-lang/cargo#12280)
- [Bail out an error when using `cargo::` in custom build script.]
  (rust-lang/cargo#12332)

Misc
----

Compatibility Notes
-------------------

- [Update the minimum external LLVM to 15.]
  (rust-lang/rust#114148)
- [Check for non-defining uses of return position `impl Trait`.]
  (rust-lang/rust#112842)

Internal Changes
----------------

These changes do not affect any public interfaces of Rust, but they
represent significant improvements to the performance or internals
of rustc and related tools.

- [Remove LLVM pointee types, supporting only opaque pointers.]
  (rust-lang/rust#105545)
- [Port PGO/LTO/BOLT optimized build pipeline to Rust.]
  (rust-lang/rust#112235)
- [Replace in-tree `rustc_apfloat` with the new version of the crate.]
  (rust-lang/rust#113843)
- [Update to LLVM 17.]
  (rust-lang/rust#114048)
- [Add `internal_features` lint for internal unstable features.]
  (rust-lang/rust#108955)
- [Mention style for new syntax in tracking issue template.]
  (rust-lang/rust#113586)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. C-enhancement Category: An issue proposing an enhancement or a PR with one. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. merged-by-bors This PR was explicitly merged by bors. needs-fcp This change is insta-stable, so needs a completed FCP to proceed. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet