Add test for evaluate_obligation: Ok(EvaluatedToOkModuloRegions) ICE by wesleywiser · Pull Request #91065 · rust-lang/rust

wesleywiser · 2021-11-20T02:03:50Z

Adds the minimial repro test case from #85360. The fix for #85360 was
supposed to be #85868 however the repro was resolved in the 2021-07-05
nightly while #85868 didn't land until 2021-09-03. The reason for that
is d34a3a4 also resolves that
issue.

To test if #85868 actually fixes #85360, I reverted
d34a3a4 and found that #85868 does
indeed resolve #85360.

With that question resolved, add a test case to our incremental test
suite for the original Ok(EvaluatedToOkModuloRegions) ICE.

Thanks to @lqd for helping track this down!

rust-highfive · 2021-11-20T02:03:53Z

r? @Mark-Simulacrum

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

jackh726 · 2021-11-20T15:28:31Z

@Aaron1011 was #85868 supposed to fix something that #85090 didn't? Did you have a test case (different from the one that got minimised here) that was still failing after #85090?

Aaron1011 · 2021-11-20T18:07:15Z

@jackh726 #85090 is a pure optimization that shouldn't affect anything incremental-related.

jackh726 · 2021-11-20T19:33:13Z

According to @wesleywiser, #85090 did fix #85360 (I also see that in #85868 (comment)). I guess my thoughts are: if #85090 did completely fix the underlying incremental issue, then why did we need to land #85868? If it didn't, then the test added here isn't sufficient to ensure we don't regress. That's not to say that we shouldn't add this test - but it's unclear if there's an actual reproduction that motivated #85868, or a theoretical one.

wesleywiser · 2021-11-20T19:40:58Z

(I also see that in #85868 (comment))

Right. @lqd and I wanted to understand why the minimized repro stopped reproducing prior to the intended fix. I ran a bisect and got these results:

searched toolchains 71b8742 through b3d11f9

Regression in d34a3a4

searched nightlies: from nightly-2021-07-04 to nightly-2021-07-06
regressed nightly: nightly-2021-07-05
searched commits: from 71b8742 to b3d11f9
regressed commit: d34a3a4

@lqd had already determined previously when trying to construct a regression test that the nightly where this stopped reproducing was 2021-07-05 which is why the search space was so small.

Edit: Note, because the change we're looking for was "ICEing -> not ICEing", "regression" in the above report actually indicates the commit which fixed the reproducer.

Mark-Simulacrum · 2021-11-21T15:35:44Z

This seems fine to me, but I want to r? @jackh726 (or perhaps @Aaron1011) since I feel like you're better able to determine whether this is the right test.

Aaron1011 · 2021-11-22T18:50:19Z

Could you add #[rustc_evaluate_where_clauses] attributes, as done in #85186 ? That should allow us to catch any future caching changes that affect the evaluation results for this test (even if there isn't an incremental ICE).

Can you also add -Z assert-incr-state=not-loaded and -Z assert-incr-state=loaded to the first and second invocations respectively? See
https://github.com/rust-lang/rust/pull/90386/files#diff-1f16ba7af24cec6d9c6f8fc179a0313ec3307a4124c9cf7756f4541723e0feb2 for an example of doing this in an incremental test.

wesleywiser · 2021-11-23T18:53:33Z

Sure! I just pushed a commit that does that as a separate UI test. If there's a better way to combine the tests, feel free to let me know.

Aaron1011 · 2021-11-23T19:04:26Z

r=me unless @jackh726 wants any changes.

jackh726 · 2021-11-23T22:07:33Z

According to @wesleywiser, #85090 did fix #85360 (I also see that in #85868 (comment)). I guess my thoughts are: if #85090 did completely fix the underlying incremental issue, then why did we need to land #85868? If it didn't, then the test added here isn't sufficient to ensure we don't regress. That's not to say that we shouldn't add this test - but it's unclear if there's an actual reproduction that motivated #85868, or a theoretical one.

@Aaron1011 any thoughts here? Specifically, do you think there is a bug that isn't covered by this regression test?

Regardless, I think we should land this test. @bors r+ rollup

bors · 2021-11-23T22:07:35Z

📌 Commit c078c5ac597dfa58c59690a4cad8397ca2df2545 has been approved by jackh726

jackh726 · 2021-11-23T22:08:17Z

@bors r=Aaron1011,jackh726

just to register the dual review

bors · 2021-11-23T22:08:19Z

💡 This pull request was already approved, no need to approve it again.

There's another pull request that is currently being tested, blocking this pull request: [experiment] perf-test smallvec optimizations #90673

bors · 2021-11-23T22:08:20Z

📌 Commit c078c5ac597dfa58c59690a4cad8397ca2df2545 has been approved by Aaron1011,jackh726

Aaron1011 · 2021-11-23T22:14:57Z

PR 85090 causes us to always return EvaluatedToOk for certain predicates. Since the incremental bug requires that we end up with EvaluatedToOkModuloRegions in one compilation run, that PR reduces the number of cases where we could potentially hit the bug. However, it does actually fix the bug, since we will still need to return EvaluatedToOkModuloRegions for other predicates (which requires that the cache behaves correctly).

To make the test more robust, we could land a follow-up PR asserting that some where clause actually evaluates to EvaluatedToOkModuloRegions. Additionally, we can write a unit test that explicitly checks what happens when we evaluate an EvaluatedToOkModuloRegions predicate with a populated cache.

jackh726 · 2021-11-23T22:31:09Z

Okay, so sounds like this does not "close" #85360, at least in terms of a proper regression test for the evaluate_obligation ICE. At least, this doesn't completely alleviate my concerns about making further changes to the projection cache for performance only.

(Should we reopen #85360, or open a new issue to properly track the lack of a regression test to ensure that the cache behaves correctly for EvaluatedToOkModuloRegions?)

wesleywiser · 2021-11-24T14:12:32Z

@bors r-

I think I should go ahead and do that as part of this PR.

Aaron1011 · 2021-11-28T20:08:46Z

src/test/ui/traits/issue-85360-eval-obligation-ice.rs

You can make the top-level evaluation produce EvaluatedToOkModuloRegions by adding where for<'a> &'a bool: 'a to this impl. However, this will not work until #91329 is merged

Now that that PR has been merged, it should be possible to adjust this impl to produce EvaluatedToOkModuloRegions

Implemented. Thanks @Aaron1011!

@lqd

Adds the minimial repro test case from rust-lang#85360. The fix for rust-lang#85360 was supposed to be rust-lang#85868 however the repro was resolved in the 2021-07-05 nightly while rust-lang#85360 didn't land until 2021-09-03. The reason for that is d34a3a4 **also** resolves that issue. To test if rust-lang#85868 actually fixes rust-lang#85360, I reverted d34a3a4 and found that rust-lang#85868 does indeed resolve rust-lang#85360. With that question resolved, add a test case to our incremental test suite for the original Ok(EvaluatedToOkModuloRegions) ICE. Thanks to @lqd for helping track this down!

As suggested via reviewer feedback.

jackh726 · 2021-12-04T16:33:42Z

Does the current test fail after reverting #85868 but not #85090?

wesleywiser · 2021-12-06T15:10:28Z

Yes, that's correct.

jackh726 · 2021-12-06T15:14:33Z

Amazing. Thank you both @wesleywiser @Aaron1011

@bors r+

bors · 2021-12-06T15:14:35Z

📌 Commit 6fe13f6 has been approved by jackh726

Add test for evaluate_obligation: Ok(EvaluatedToOkModuloRegions) ICE Adds the minimial repro test case from rust-lang#85360. The fix for rust-lang#85360 was supposed to be rust-lang#85868 however the repro was resolved in the 2021-07-05 nightly while rust-lang#85868 didn't land until 2021-09-03. The reason for that is d34a3a4 **also** resolves that issue. To test if rust-lang#85868 actually fixes rust-lang#85360, I reverted d34a3a4 and found that rust-lang#85868 does indeed resolve rust-lang#85360. With that question resolved, add a test case to our incremental test suite for the original Ok(EvaluatedToOkModuloRegions) ICE. Thanks to `@lqd` for helping track this down!

Add test for evaluate_obligation: Ok(EvaluatedToOkModuloRegions) ICE Adds the minimial repro test case from rust-lang#85360. The fix for rust-lang#85360 was supposed to be rust-lang#85868 however the repro was resolved in the 2021-07-05 nightly while rust-lang#85868 didn't land until 2021-09-03. The reason for that is d34a3a4 **also** resolves that issue. To test if rust-lang#85868 actually fixes rust-lang#85360, I reverted d34a3a4 and found that rust-lang#85868 does indeed resolve rust-lang#85360. With that question resolved, add a test case to our incremental test suite for the original Ok(EvaluatedToOkModuloRegions) ICE. Thanks to ``@lqd`` for helping track this down!

Add test for evaluate_obligation: Ok(EvaluatedToOkModuloRegions) ICE Adds the minimial repro test case from rust-lang#85360. The fix for rust-lang#85360 was supposed to be rust-lang#85868 however the repro was resolved in the 2021-07-05 nightly while rust-lang#85868 didn't land until 2021-09-03. The reason for that is d34a3a4 **also** resolves that issue. To test if rust-lang#85868 actually fixes rust-lang#85360, I reverted d34a3a4 and found that rust-lang#85868 does indeed resolve rust-lang#85360. With that question resolved, add a test case to our incremental test suite for the original Ok(EvaluatedToOkModuloRegions) ICE. Thanks to ```@lqd``` for helping track this down!

…askrgr Rollup of 10 pull requests Successful merges: - rust-lang#87614 (Recommend fix `count()` -> `len()` on slices) - rust-lang#91065 (Add test for evaluate_obligation: Ok(EvaluatedToOkModuloRegions) ICE) - rust-lang#91312 (Fix AnonConst ICE) - rust-lang#91341 (Add `array::IntoIter::{empty, from_raw_parts}`) - rust-lang#91493 (Remove a dead code path.) - rust-lang#91503 (Tweak "call this function" suggestion to have smaller span) - rust-lang#91547 (Suggest try_reserve in try_reserve_exact) - rust-lang#91562 (Pretty print async block without redundant space) - rust-lang#91620 (Update books) - rust-lang#91622 (:arrow_up: rust-analyzer) Failed merges: - rust-lang#91571 (Remove unneeded access to pretty printer's `s` field in favor of deref) r? `@ghost` `@rustbot` modify labels: rollup

Mark-Simulacrum · 2022-01-05T22:07:20Z

Declining beta-backport: the test added by this PR does not pass when backported alongside #90423, and we believe that this is due to other PRs not being backported (which we don't want to backport at this time).

rust-highfive assigned Mark-Simulacrum Nov 20, 2021

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

wesleywiser mentioned this pull request Nov 20, 2021

Unstable fingerprints tracking issue #84970

Closed

rust-highfive assigned jackh726 and unassigned Mark-Simulacrum Nov 21, 2021

apiraino added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Nov 22, 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 Nov 23, 2021

bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Nov 24, 2021

Aaron1011 reviewed Nov 28, 2021

View reviewed changes

wesleywiser added 2 commits December 3, 2021 19:30

Add test with #[rustc_evaluate_where_clauses]

98e9b32

As suggested via reviewer feedback.

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 6, 2021

matthiaskrgr mentioned this pull request Dec 6, 2021

Rollup of 5 pull requests #91596

Closed

jackh726 mentioned this pull request Dec 6, 2021

Re-introduce concept of projection cache 'completion' #89831

Merged

matthiaskrgr mentioned this pull request Dec 6, 2021

Rollup of 9 pull requests #91602

Closed

matthiaskrgr mentioned this pull request Dec 7, 2021

Rollup of 9 pull requests #91621

Closed

matthiaskrgr mentioned this pull request Dec 7, 2021

Rollup of 10 pull requests #91627

Merged

bors merged commit 42d0f83 into rust-lang:master Dec 7, 2021

rustbot added this to the 1.59.0 milestone Dec 7, 2021

pnkfelix added beta-accepted Accepted for backporting to the compiler in the beta channel. beta-nominated Nominated for backporting to the compiler in the beta channel. labels Dec 16, 2021

pnkfelix mentioned this pull request Dec 16, 2021

Deduplicate projection sub-obligations #90423

Merged

Mark-Simulacrum removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Jan 5, 2022

Mark-Simulacrum modified the milestones: 1.59.0, 1.58.0 Jan 5, 2022

Mark-Simulacrum mentioned this pull request Jan 5, 2022

[beta] backports #92592

Merged

Mark-Simulacrum removed the beta-accepted Accepted for backporting to the compiler in the beta channel. label Jan 5, 2022

Mark-Simulacrum modified the milestones: 1.58.0, 1.59.0 Jan 5, 2022

Uh oh!

Conversation

wesleywiser commented Nov 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rust-highfive commented Nov 20, 2021

Uh oh!

jackh726 commented Nov 20, 2021

Uh oh!

Aaron1011 commented Nov 20, 2021

Uh oh!

jackh726 commented Nov 20, 2021

Uh oh!

wesleywiser commented Nov 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Mark-Simulacrum commented Nov 21, 2021

Uh oh!

Aaron1011 commented Nov 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wesleywiser commented Nov 23, 2021

Uh oh!

Aaron1011 commented Nov 23, 2021

Uh oh!

jackh726 commented Nov 23, 2021

Uh oh!

bors commented Nov 23, 2021

Uh oh!

jackh726 commented Nov 23, 2021

Uh oh!

bors commented Nov 23, 2021

Uh oh!

bors commented Nov 23, 2021

Uh oh!

Aaron1011 commented Nov 23, 2021

Uh oh!

jackh726 commented Nov 23, 2021

Uh oh!

wesleywiser commented Nov 24, 2021

Uh oh!

Aaron1011 Nov 28, 2021

Choose a reason for hiding this comment

Uh oh!

Aaron1011 Dec 3, 2021

Choose a reason for hiding this comment

Uh oh!

wesleywiser Dec 4, 2021

Choose a reason for hiding this comment

Uh oh!

jackh726 commented Dec 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wesleywiser commented Dec 6, 2021

Uh oh!

jackh726 commented Dec 6, 2021

Uh oh!

bors commented Dec 6, 2021

Uh oh!

Mark-Simulacrum commented Jan 5, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

wesleywiser commented Nov 20, 2021 •

edited

Loading

wesleywiser commented Nov 20, 2021 •

edited

Loading

Aaron1011 commented Nov 22, 2021 •

edited

Loading

jackh726 commented Dec 4, 2021 •

edited

Loading