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

2229: Mark insignificant dtor in stdlib #89144

Merged
merged 5 commits into from
Sep 26, 2021
Merged

Conversation

arora-aman
Copy link
Member

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 21, 2021
@the8472 the8472 added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Sep 21, 2021
@petrochenkov
Copy link
Contributor

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

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

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

// 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());
Copy link
Member

Choose a reason for hiding this comment

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

@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
}

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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 Mark-Simulacrum added beta-nominated Nominated for backporting to the compiler in the beta channel. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. and removed T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Sep 23, 2021
@Mark-Simulacrum
Copy link
Member

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

@bors r+ p=1

@bors
Copy link
Contributor

bors commented Sep 23, 2021

📌 Commit 994793f has been approved by nikomatsakis

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

bors commented Sep 24, 2021

⌛ Testing commit 994793f with merge d01801521a35e3f4bcd85a0db57ba567c13cdbae...

@rust-log-analyzer
Copy link
Collaborator

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

bors commented Sep 24, 2021

💔 Test failed - checks-actions

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

ehuss commented Sep 26, 2021

@bors retry

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

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

bors commented Sep 26, 2021

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

@bors
Copy link
Contributor

bors commented Sep 26, 2021

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

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

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

Beta backport accepted as per compiler team on Zulip

@rustbot label +beta-accepted

@rustbot rustbot added the beta-accepted Accepted for backporting to the compiler in the beta channel. label Sep 30, 2021
ehuss pushed a commit to ehuss/rust that referenced this pull request Oct 4, 2021
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`
@ehuss ehuss mentioned this pull request Oct 4, 2021
@ehuss ehuss removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Oct 4, 2021
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 4, 2021
[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)
@Mark-Simulacrum Mark-Simulacrum modified the milestones: 1.57.0, 1.56.0 Oct 6, 2021
tgross35 added a commit to tgross35/rust that referenced this pull request Sep 30, 2024
…, 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"
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Oct 1, 2024
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"
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
beta-accepted Accepted for backporting to the compiler in the beta channel. 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.