Fixes incorrect handling of ADT's drop requirements #90218

JakobDegen · 2021-10-23T22:28:23Z

Fixes #90024 and a bunch of duplicates.

The main issue was just that the contract of NeedsDropTypes::adt_components was inconsistent; the list of types it might return were the generic parameters themselves or the fields of the ADT, depending on the nature of the drop impl. This meant that the caller could not determine whether a .subst() call was still needed on those types; it called .subst() in all cases, and this led to ICEs when the returned types were the generic params.

First contribution of more than a few lines, so feedback definitely appreciated.

See rust-lang#90024 (comment)

rust-highfive · 2021-10-23T22:28:26Z

r? @wesleywiser

(rust-highfive has picked a reviewer for you, use r? to override)

Uses 2 MCVEs from the issue tracker that test opposite sides of the problem.

wesleywiser · 2021-10-25T18:59:30Z

r? @nikomatsakis

arora-aman

Thank you for the fix.

LGTM, nits only.

compiler/rustc_ty_utils/src/needs_drop.rs

JakobDegen · 2021-10-26T00:47:41Z

Cleaned those up and fixed the function names on a couple others, doing this before had slipped my mind. Thanks!

pnkfelix · 2021-10-28T14:28:32Z

nominating because @apiraino was concerned this might get overlooked

apiraino · 2021-10-28T14:30:37Z

Adding note of the Zulip thread

@rustbot label -I-nominated +I-compiler-nominated

nikomatsakis · 2021-10-28T14:47:11Z

@bors r+

Thanks for the comment explaining the purpose of the PR! Always helpful.

bors · 2021-10-28T14:47:12Z

📌 Commit aff37f8 has been approved by nikomatsakis

ehuss · 2021-10-28T14:50:54Z

@bors p=1

To help stem the tide of reports.

bors · 2021-10-28T16:03:16Z

⌛ Testing commit aff37f8 with merge 85c0558...

bors · 2021-10-28T19:00:25Z

☀️ Test successful - checks-actions
Approved by: nikomatsakis
Pushing 85c0558 to master...

rust-timer · 2021-10-28T23:40:04Z

Finished benchmarking commit (85c0558): comparison url.

Summary: This change led to very large relevant regressions 😿 in compiler performance.

Very large regression in instruction counts (up to 5.3% on full builds of regression-31157)

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression

apiraino · 2021-10-29T08:07:00Z

The perf report suggests a significant regression on incremental compilation scenarios (if I correctly read the report): something worth investigating before backporting?

pnkfelix · 2021-11-02T14:51:19Z

compiler/rustc_ty_utils/src/needs_drop.rs

-                                self.param_env,
-                                required_ty.subst(tcx, substs),
-                            );
+                            let subst_ty =


Note: I probably would have renamed this variable. I think the reason it was named subst_ty is because it, prior to this PR, had the substs applied to it. But that is no longer the case here, if I understand correctly.

pnkfelix · 2021-11-02T15:18:17Z

Visiting for performance triage this week.

regression-31157 check regressed by 4.7% on incr and 5.3 on full.
issue-46449 check regressed by 1.89% on incr-full.
wg-grammar regressed by 1.3-1.4% in a bunch of scenarios.
but otherwise, this does not seem too bad. I think we should keep this PR approved for backport, while also look into fixing the regression on nightly.

pnkfelix · 2021-11-02T15:21:49Z

Filed issue #90504 for follow-up investigation.

@rustbot label: +perf-regression-triaged

apiraino · 2021-11-02T23:58:16Z

thanks @pnkfelix !

pnkfelix · 2021-11-04T14:22:25Z

discussed in T-compiler meeting. backport approved.

…lacrum Address performance regression introduced by rust-lang#90218 As part of the changes in rust-lang#90218 , the `adt_drop_tys` and friends code stopped recursing through the query system, meaning that intermediate computations did not get cached. This change adds the recursions back in without re-introducing any of the old issues. On local benchmarks this fixes the 5% regressions in rust-lang#90504 ; the wg-grammar regressions didn't seem to move too much. I may take some time later to look into those. Not sure who to request for review here, so will leave it up to whoever gets it.

[beta] backports - Fix assertion failures in OwnedHandle with windows_subsystem. rust-lang#88798 - Ensure that pushing empty path works as before on verbatim paths rust-lang#89665 - Feature gate + make must_not_suspend allow-by-default rust-lang#89826 - Only use clone3 when needed for pidfd rust-lang#89930 - Fix documentation header sizes rust-lang#90186 - Fixes incorrect handling of ADT's drop requirements rust-lang#90218 - Fix ICE when forgetting to Box a parameter to a Self::func call rust-lang#90221 - Prevent duplicate caller bounds candidates by exposing default substs in Unevaluated rust-lang#90266 - Update odht crate to 0.3.1 (big-endian bugfix) rust-lang#90403 - rustdoc: Go back to loading all external crates unconditionally rust-lang#90489 - Split doc_cfg and doc_auto_cfg features rust-lang#90502 - Apply adjustments for field expression even if inaccessible rust-lang#90508 - Warn for variables that are no longer captured rust-lang#90597 - Properly register text_direction_codepoint_in_comment lint. rust-lang#90626 - CI: Use ubuntu image to download openssl, curl sources, cacert.pem for x86 dist builds rust-lang#90457 - Android is not GNU rust-lang#90834 - Update llvm submodule rust-lang#90954 Additionally, this bumps the stage 0 compiler from beta to stable 1.56.1. r? `@Mark-Simulacrum`

Fixes incorrect handling of ADT's drop requirements

9158fc2

See rust-lang#90024 (comment)

rust-highfive assigned wesleywiser Oct 23, 2021

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

Add regresstion test for rust-lang#90024.

eae42fd

Uses 2 MCVEs from the issue tracker that test opposite sides of the problem.

JakobDegen marked this pull request as ready for review October 24, 2021 02:39

rust-highfive assigned nikomatsakis and unassigned wesleywiser Oct 25, 2021

ehuss mentioned this pull request Oct 25, 2021

ICE: cargo fix --edition could not fully normalize Iterator::Item #90140

Closed

arora-aman approved these changes Oct 26, 2021

View reviewed changes

compiler/rustc_ty_utils/src/needs_drop.rs Outdated Show resolved Hide resolved

compiler/rustc_ty_utils/src/needs_drop.rs Outdated Show resolved Hide resolved

compiler/rustc_ty_utils/src/needs_drop.rs Outdated Show resolved Hide resolved

Clean up debug statements in needs_drop

aff37f8

arora-aman mentioned this pull request Oct 28, 2021

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

Closed

apiraino added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Oct 28, 2021

pnkfelix added the I-nominated label Oct 28, 2021

rustbot added I-compiler-nominated Nominated for discussion during a compiler team meeting. and removed I-nominated labels Oct 28, 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 Oct 28, 2021

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

bors added the merged-by-bors This PR was explicitly merged by bors. label Oct 28, 2021

bors merged commit 85c0558 into rust-lang:master Oct 28, 2021

rustbot added this to the 1.58.0 milestone Oct 28, 2021

rustbot added the perf-regression Performance regression. label Oct 28, 2021

ehuss mentioned this pull request Oct 29, 2021

Cargo fix --edition leads to "error: overflow while checking ... requires drop" #90147

Closed

pnkfelix reviewed Nov 2, 2021

View reviewed changes

pnkfelix mentioned this pull request Nov 2, 2021

perf regression from adt_significant_drop_fix #90504

Closed

rustbot added the perf-regression-triaged The performance regression has been triaged. label Nov 2, 2021

ehuss mentioned this pull request Nov 3, 2021

cargo fix --edition crashes with internal compiler error #90523

Closed

pnkfelix added the beta-accepted Accepted for backporting to the compiler in the beta channel. label Nov 4, 2021

JakobDegen deleted the adt_significant_drop_fix branch November 11, 2021 18:25

JakobDegen mentioned this pull request Nov 12, 2021

Address performance regression introduced by #90218 #90845

Merged

cuviper mentioned this pull request Nov 16, 2021

[beta] backports #90938

Merged

cuviper removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Nov 16, 2021

cuviper modified the milestones: 1.58.0, 1.57.0 Nov 16, 2021

EwoutH mentioned this pull request Dec 2, 2021

2021 cargo fix --edition ICE: type parameter out of range when substituting #90024

Closed

apiraino removed the I-compiler-nominated Nominated for discussion during a compiler team meeting. label Feb 3, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes incorrect handling of ADT's drop requirements #90218

Fixes incorrect handling of ADT's drop requirements #90218

JakobDegen commented Oct 23, 2021 •

edited

Loading

rust-highfive commented Oct 23, 2021

wesleywiser commented Oct 25, 2021

arora-aman left a comment

JakobDegen commented Oct 26, 2021

pnkfelix commented Oct 28, 2021

apiraino commented Oct 28, 2021 •

edited

Loading

nikomatsakis commented Oct 28, 2021

bors commented Oct 28, 2021

ehuss commented Oct 28, 2021

bors commented Oct 28, 2021

bors commented Oct 28, 2021

rust-timer commented Oct 28, 2021

apiraino commented Oct 29, 2021

pnkfelix Nov 2, 2021

pnkfelix commented Nov 2, 2021

pnkfelix commented Nov 2, 2021

apiraino commented Nov 2, 2021

pnkfelix commented Nov 4, 2021

Fixes incorrect handling of ADT's drop requirements #90218

Fixes incorrect handling of ADT's drop requirements #90218

Conversation

JakobDegen commented Oct 23, 2021 • edited Loading

rust-highfive commented Oct 23, 2021

wesleywiser commented Oct 25, 2021

arora-aman left a comment

Choose a reason for hiding this comment

JakobDegen commented Oct 26, 2021

pnkfelix commented Oct 28, 2021

apiraino commented Oct 28, 2021 • edited Loading

nikomatsakis commented Oct 28, 2021

bors commented Oct 28, 2021

ehuss commented Oct 28, 2021

bors commented Oct 28, 2021

bors commented Oct 28, 2021

rust-timer commented Oct 28, 2021

apiraino commented Oct 29, 2021

pnkfelix Nov 2, 2021

Choose a reason for hiding this comment

pnkfelix commented Nov 2, 2021

pnkfelix commented Nov 2, 2021

apiraino commented Nov 2, 2021

pnkfelix commented Nov 4, 2021

JakobDegen commented Oct 23, 2021 •

edited

Loading

apiraino commented Oct 28, 2021 •

edited

Loading