2229: Mark insignificant dtor in stdlib#89144

arora-aman · 2021-09-21T09:11:32Z

I looked at all public stdlib Drop implementations and categorized them into Insigificant/Maybe/Significant Drop.

Reasons are noted here: https://docs.google.com/spreadsheets/d/19edb9r5lo2UqMrCOVjV0fwcSdS-R7qvKNL76q7tO8VA/edit#gid=1838773501

One thing missing from this PR is tagging HashMap as insigificant destructor as that needs some discussion.

r? @Mark-Simulacrum

cc @nikomatsakis

library/std/src/lazy.rs

src/test/ui/closures/2229_closure_analysis/migrations/insignificant_drop.rs

petrochenkov · 2021-09-22T09:35:30Z

It's pretty worrying when something that was rejected as a general purpose language feature is widely employed by a standard library and makes observable difference that cannot (?) be emulated on stable Rust.

Or is rustc_insignificant_dtor going to be stabilized in some form?

Mark-Simulacrum · 2021-09-22T11:34:26Z

This feature only affects migrations for RFC 2229, so is both largely a quality of life improvement and something we could drop with no impact to stability guarantees (just affects lint's, and ones usually not used).

For these reasons I don't find it too concerning that stable Rust can't use this attribute. In practice I believe our expectation is that most crates will find std annotations sufficient to avoid spurious migrations.

petrochenkov · 2021-09-22T13:26:27Z

I see, thanks.
I assumed that it actually affects the logic of the fine-grained captures.

Mark-Simulacrum · 2021-09-22T13:34:35Z

compiler/rustc_ty_utils/src/needs_drop.rs

+                    // Since the destructor is insignificant, we just want to make sure all of
+                    // the passed in type parameters are also insignificant.
+                    // Eg: Vec<T> dtor is insignificant when T=i32 but significant when T=Mutex.
+                    return Ok(substs.types().collect::<Vec<Ty<'_>>>().into_iter());


@nikomatsakis is probably better to say here, but I think this is actually the wrong thing to check? We probably need to look not at the subst types but at the fields with substs applied -- in particular, for something like the following I suspect this will do the wrong thing right now?

struct Foo<T: Trait> { var: T::Bar, // T::Bar is significant drop, T is not }

That said I suspect this won't affect any of the std types... so it might be OK

Exactly-- I agree that it's not necessarily what you would want in the "general case" but in practice I think it's adequate.

It's also not obvious that you want the fields -- for example, in the case of our HashMap that wraps another one, if we looked at the fields, we would flag that as a significant dtor

nikomatsakis · 2021-09-23T15:30:37Z

compiler/rustc_ty_utils/src/needs_drop.rs

+                    // Since the destructor is insignificant, we just want to make sure all of
+                    // the passed in type parameters are also insignificant.
+                    // Eg: Vec<T> dtor is insignificant when T=i32 but significant when T=Mutex.
+                    return Ok(substs.types().collect::<Vec<Ty<'_>>>().into_iter());


It's also not obvious that you want the fields -- for example, in the case of our HashMap that wraps another one, if we looked at the fields, we would flag that as a significant dtor

Mark-Simulacrum · 2021-09-23T16:55:51Z

r? @nikomatsakis -- I could approve this I think, but ultimately you seem like the better candidate :)

Also beta-nominating since I think it's important we have this for the RFC 2229 migrations out of the box, it gets less useful the more people migrate their code.

nikomatsakis · 2021-09-23T23:00:51Z

@bors r+ p=1

bors · 2021-09-23T23:00:52Z

📌 Commit 994793f has been approved by nikomatsakis

bors · 2021-09-24T04:51:24Z

⌛ Testing commit 994793f with merge d01801521a35e3f4bcd85a0db57ba567c13cdbae...

rust-log-analyzer · 2021-09-24T05:29:03Z

The job i686-msvc-1 failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

test [rustdoc] rustdoc\wrapping.rs ... ok

failures:

---- [rustdoc] rustdoc\macro-document-private-duplicate.rs stdout ----
Some tests failed in compiletest suite=rustdoc mode=rustdoc host=i686-pc-windows-msvc target=i686-pc-windows-msvc

error: htmldocck failed!
status: exit code: 1
command: "C:\\hostedtoolcache\\windows\\Python\\3.9.7\\x64\\python3.exe" "D:\\a\\rust\\rust\\src/etc/htmldocck.py" "D:\\a\\rust\\rust\\build\\i686-pc-windows-msvc\\test\\rustdoc\\macro-document-private-duplicate" "D:\\a\\rust\\rust\\src/test\\rustdoc\\macro-document-private-duplicate.rs"
------------------------------------------

------------------------------------------
stderr:
stderr:
------------------------------------------
12: @has check failed
 `PATTERN` did not match
 // @has macro_document_private_duplicate/macro.a_macro.html 'Doc 1.'
19: @!has check failed
 `PATTERN` did not match
 // @!has macro_document_private_duplicate/macro.a_macro.html 'Doc 2.'
Encountered 2 errors

------------------------------------------

---
test result: FAILED. 462 passed; 1 failed; 7 ignored; 0 measured; 0 filtered out; finished in 68.24s



command did not execute successfully: "D:\\a\\rust\\rust\\build\\i686-pc-windows-msvc\\stage0-tools-bin\\compiletest.exe" "--compile-lib-path" "D:\\a\\rust\\rust\\build\\i686-pc-windows-msvc\\stage2\\bin" "--run-lib-path" "D:\\a\\rust\\rust\\build\\i686-pc-windows-msvc\\stage2\\lib\\rustlib\\i686-pc-windows-msvc\\lib" "--rustc-path" "D:\\a\\rust\\rust\\build\\i686-pc-windows-msvc\\stage2\\bin\\rustc.exe" "--rustdoc-path" "D:\\a\\rust\\rust\\build\\i686-pc-windows-msvc\\stage2\\bin\\rustdoc.exe" "--src-base" "D:\\a\\rust\\rust\\src/test\\rustdoc" "--build-base" "D:\\a\\rust\\rust\\build\\i686-pc-windows-msvc\\test\\rustdoc" "--stage-id" "stage2-i686-pc-windows-msvc" "--suite" "rustdoc" "--mode" "rustdoc" "--target" "i686-pc-windows-msvc" "--host" "i686-pc-windows-msvc" "--llvm-filecheck" "D:\\a\\rust\\rust\\build\\i686-pc-windows-msvc\\llvm\\build\\bin\\FileCheck.exe" "--nodejs" "C:\\Program Files\\nodejs\\node" "--npm" "C:\\Program Files\\nodejs\\npm" "--host-rustcflags" "-Crpath -O -Cdebuginfo=0  -Lnative=D:\\a\\rust\\rust\\build\\i686-pc-windows-msvc\\native\\rust-test-helpers" "--target-rustcflags" "-Crpath -O -Cdebuginfo=0  -Lnative=D:\\a\\rust\\rust\\build\\i686-pc-windows-msvc\\native\\rust-test-helpers" "--docck-python" "C:\\hostedtoolcache\\windows\\Python\\3.9.7\\x64\\python3.exe" "--lldb-python" "C:\\hostedtoolcache\\windows\\Python\\3.9.7\\x64\\python3.exe" "--gdb" "C:\\msys64\\usr\\bin\\gdb" "--llvm-version" "13.0.0-rust-1.57.0-nightly" "--llvm-components" "aarch64 aarch64asmparser aarch64codegen aarch64desc aarch64disassembler aarch64info aarch64utils aggressiveinstcombine all all-targets analysis arm armasmparser armcodegen armdesc armdisassembler arminfo armutils asmparser asmprinter avr avrasmparser avrcodegen avrdesc avrdisassembler avrinfo binaryformat bitreader bitstreamreader bitwriter bpf bpfasmparser bpfcodegen bpfdesc bpfdisassembler bpfinfo cfguard codegen core coroutines coverage debuginfocodeview debuginfodwarf debuginfogsym debuginfomsf debuginfopdb demangle dlltooldriver dwarflinker dwp engine executionengine extensions filecheck frontendopenacc frontendopenmp fuzzmutate globalisel hexagon hexagonasmparser hexagoncodegen hexagondesc hexagondisassembler hexagoninfo instcombine instrumentation interfacestub interpreter ipo irreader jitlink libdriver lineeditor linker lto m68k m68kasmparser m68kcodegen m68kdesc m68kdisassembler m68kinfo mc mca mcdisassembler mcjit mcparser mips mipsasmparser mipscodegen mipsdesc mipsdisassembler mipsinfo mirparser msp430 msp430asmparser msp430codegen msp430desc msp430disassembler msp430info native nativecodegen nvptx nvptxcodegen nvptxdesc nvptxinfo objcarcopts object objectyaml option orcjit orcshared orctargetprocess passes powerpc powerpcasmparser powerpccodegen powerpcdesc powerpcdisassembler powerpcinfo profiledata remarks riscv riscvasmparser riscvcodegen riscvdesc riscvdisassembler riscvinfo runtimedyld scalaropts selectiondag sparc sparcasmparser sparccodegen sparcdesc sparcdisassembler sparcinfo support symbolize systemz systemzasmparser systemzcodegen systemzdesc systemzdisassembler systemzinfo tablegen target textapi transformutils vectorize webassembly webassemblyasmparser webassemblycodegen webassemblydesc webassemblydisassembler webassemblyinfo webassemblyutils windowsmanifest x86 x86asmparser x86codegen x86desc x86disassembler x86info xray" "--cc" "" "--cxx" "" "--cflags" "" "--adb-path" "adb" "--adb-test-dir" "/data/tmp/work" "--android-cross-path" "" "--channel" "nightly" "--color" "always"


Build completed unsuccessfully in 0:33:43
Build completed unsuccessfully in 0:33:43
make: *** [Makefile:72: ci-subset-1] Error 1

bors · 2021-09-24T05:29:44Z

💔 Test failed - checks-actions

ehuss · 2021-09-26T19:00:05Z

@bors retry

macro-document-private-duplicate.rs has been disabled.

bors · 2021-09-26T19:36:03Z

⌛ Testing commit 994793f with merge 05044c2...

bors · 2021-09-26T22:43:56Z

☀️ Test successful - checks-actions
Approved by: nikomatsakis
Pushing 05044c2 to master...

rust-timer · 2021-09-27T04:18:02Z

Finished benchmarking commit (05044c2): 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

apiraino · 2021-09-30T14:29:28Z

Beta backport accepted as per compiler team on Zulip

@rustbot label +beta-accepted

2229: Mark insignificant dtor in stdlib I looked at all public [stdlib Drop implementations](https://doc.rust-lang.org/stable/std/ops/trait.Drop.html#implementors) and categorized them into Insigificant/Maybe/Significant Drop. Reasons are noted here: https://docs.google.com/spreadsheets/d/19edb9r5lo2UqMrCOVjV0fwcSdS-R7qvKNL76q7tO8VA/edit#gid=1838773501 One thing missing from this PR is tagging HashMap as insigificant destructor as that needs some discussion. r? `@Mark-Simulacrum` cc `@nikomatsakis`

[beta] Beta rollup * Fix WinUWP std compilation errors due to I/O safety rust-lang#88587 * Disable RemoveZsts in generators to avoid query cycles rust-lang#88979 * Disable the evaluation cache when in intercrate mode rust-lang#88994 * Fix linting when trailing macro expands to a trailing semi rust-lang#88996 * Don't use projection cache or candidate cache in intercrate mode rust-lang#89125 * 2229: Mark insignificant dtor in stdlib rust-lang#89144 * Temporarily rename int_roundings functions to avoid conflicts rust-lang#89184 * [rfc 2229] Drop fully captured upvars in the same order as the regular drop code rust-lang#89208 * Use the correct edition for syntax highlighting doctests rust-lang#89277 * Don't normalize opaque types with escaping late-bound regions rust-lang#89285 * Update Let's Encrypt ROOT CA certificate in dist-(i686|x86_64)-linux docker images rust-lang#89486 Cargo update: * - [beta] 1.56 backports (rust-lang/cargo#9958) * - [beta] Revert "When a dependency does not have a version, git or path… (rust-lang/cargo#9912) * - [beta] Fix rustc --profile=dev unstable check. (rust-lang/cargo#9901)

…, r=Amanieu Mark some more types as having insignificant dtor These were caught by rust-lang#129864 (comment), which is implementing a lint for some changes in drop order for temporaries in tail expressions. Specifically, the destructors of `CString` and the bitpacked repr for `std::io::Error` are insignificant insofar as they don't have side-effects on things like locking or synchronization; they just free memory. See some discussion on rust-lang#89144 for what makes a drop impl "significant"

Rollup merge of rust-lang#130914 - compiler-errors:insignificant-dtor, r=Amanieu Mark some more types as having insignificant dtor These were caught by rust-lang#129864 (comment), which is implementing a lint for some changes in drop order for temporaries in tail expressions. Specifically, the destructors of `CString` and the bitpacked repr for `std::io::Error` are insignificant insofar as they don't have side-effects on things like locking or synchronization; they just free memory. See some discussion on rust-lang#89144 for what makes a drop impl "significant"

arora-aman added 4 commits September 21, 2021 04:06

2229: Early exit when we see an insigificant drop

95cfbe4

2229: Annotate stdlib with insignficant dtors

099a34c

Update tests

1c98988

Handle type params in insig dtors

d2cbe21

rust-highfive assigned Mark-Simulacrum Sep 21, 2021

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

nikomatsakis requested changes Sep 21, 2021

View reviewed changes

library/std/src/lazy.rs Show resolved Hide resolved

src/test/ui/closures/2229_closure_analysis/migrations/insignificant_drop.rs Show resolved Hide resolved

the8472 added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Sep 21, 2021

PR fixup

994793f

Mark-Simulacrum reviewed Sep 22, 2021

View reviewed changes

nikomatsakis approved these changes Sep 23, 2021

View reviewed changes

rust-highfive assigned nikomatsakis and unassigned Mark-Simulacrum Sep 23, 2021

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 23, 2021

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Sep 24, 2021

petrochenkov mentioned this pull request Sep 24, 2021

Test src\test\rustdoc\macro-document-private-duplicate.rs fails spuriously #89228

Open

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 26, 2021

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

bors merged commit 05044c2 into rust-lang:master Sep 26, 2021

rustbot added this to the 1.57.0 milestone Sep 26, 2021

bors mentioned this pull request Sep 26, 2021

Add support for allocators in Rc & Arc #89132

Merged

rustbot added the beta-accepted Accepted for backporting to the compiler in the beta channel. label Sep 30, 2021

ehuss mentioned this pull request Oct 4, 2021

[beta] Beta rollup #89527

Merged

ehuss removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Oct 4, 2021

Mark-Simulacrum modified the milestones: 1.57.0, 1.56.0 Oct 6, 2021

Mark-Simulacrum mentioned this pull request Oct 15, 2021

RFC 2229 - closure migration suggestion bug? #89080

Closed

hellow554 mentioned this pull request Oct 28, 2021

Error running cargo fix --edition --allow-dirty on nightly with const generics #90350

Closed

compiler-errors mentioned this pull request Sep 27, 2024

Mark some more types as having insignificant dtor #130914

Merged

Uh oh!

Conversation

arora-aman commented Sep 21, 2021

Uh oh!

Uh oh!

Uh oh!

petrochenkov commented Sep 22, 2021

Uh oh!

Mark-Simulacrum commented Sep 22, 2021

Uh oh!

petrochenkov commented Sep 22, 2021

Uh oh!

Mark-Simulacrum Sep 22, 2021

Choose a reason for hiding this comment

Uh oh!

Mark-Simulacrum Sep 22, 2021

Choose a reason for hiding this comment

Uh oh!

nikomatsakis Sep 23, 2021

Choose a reason for hiding this comment

Uh oh!

nikomatsakis Sep 23, 2021

Choose a reason for hiding this comment

Uh oh!

nikomatsakis Sep 23, 2021

Choose a reason for hiding this comment

Uh oh!

Mark-Simulacrum commented Sep 23, 2021

Uh oh!

nikomatsakis commented Sep 23, 2021

Uh oh!

bors commented Sep 23, 2021

Uh oh!

bors commented Sep 24, 2021

Uh oh!

rust-log-analyzer commented Sep 24, 2021

Uh oh!

bors commented Sep 24, 2021

Uh oh!

ehuss commented Sep 26, 2021

Uh oh!

bors commented Sep 26, 2021

Uh oh!

bors commented Sep 26, 2021

Uh oh!

rust-timer commented Sep 27, 2021

Uh oh!

apiraino commented Sep 30, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

12 participants