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.

Sign up for GitHub

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

Jump to bottom

regression: Copy no longer implemented #88969

Closed
Mark-Simulacrum opened this issue Sep 15, 2021 · 4 comments · Fixed by #89125
Closed

regression: Copy no longer implemented #88969

Mark-Simulacrum opened this issue Sep 15, 2021 · 4 comments · Fixed by #89125
Labels
regression-from-stable-to-beta Performance or correctness regression from stable to beta. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Milestone
1.56.0

Comments

@Mark-Simulacrum
Copy link
Member

Found by 1.56 crater - https://crater-reports.s3.amazonaws.com/beta-1.56-2/beta-2021-09-12/reg/gmorph-0.1.0/log.txt

[INFO] [stdout] error[E0204]: the trait `Copy` may not be implemented for this type
[INFO] [stdout]   --> src/enc.rs:16:17
[INFO] [stdout]    |
[INFO] [stdout] 16 | #[derive(Debug, Copy, Clone, PartialEq, Serialize, Deserialize)]
[INFO] [stdout]    |                 ^^^^
[INFO] [stdout] 17 | pub struct Enc {
[INFO] [stdout] 18 |     inner: Matrix3<Q231>,
[INFO] [stdout]    |     -------------------- this field does not implement `Copy`
[INFO] [stdout]    |
[INFO] [stdout]    = note: this error originates in the derive macro `Copy` (in Nightly builds, run with -Z macro-backtrace for more info)
[INFO] [stdout]

Bisections seems to point at 371f3cd, cc @Aaron1011
@Mark-Simulacrum Mark-Simulacrum added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. regression-from-stable-to-beta Performance or correctness regression from stable to beta. labels Sep 15, 2021
@Mark-Simulacrum Mark-Simulacrum added this to the 1.56.0 milestone Sep 15, 2021
@rustbot rustbot added the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Sep 15, 2021
@Mark-Simulacrum
Copy link
Member Author

Potentially related:

@Mark-Simulacrum Mark-Simulacrum mentioned this issue Sep 15, 2021
Closed
@Aaron1011
Copy link
Member

Aaron1011 commented Sep 15, 2021 •
edited
Loading

This is caused by a bad interaction between SelectionContext's 'intercrate' mode, the trait evaluation cache, and the evaluate_root_obligation call added in #85868

In PR #85868, we create a new SelectionContext to 'speculatively' evaluate projection sub-obligations. If they evaluate to EvaluatedToOk, we discard them from the cached results, allowing future projections of the same predicate to avoid re-processing them. However, if the original SelectionContext was in 'intercrate' mode, then we'll end up using the same InferCtxt (and therefore the same local evaluation cache) in both intercrate and non-intercrate mode. The result of evaluation differs between these modes, so this leads to bugs like this issue.

There are several different ways of fixing this - fortunately, all of them are quite straightforward, and are suitable for backporting. I've opened #88994 and #88993 to check the performance of two approaches.

@jackh726 jackh726 mentioned this issue Sep 16, 2021
Merged
@Mark-Simulacrum Mark-Simulacrum mentioned this issue Sep 17, 2021
Merged
@Aaron1011 Aaron1011 mentioned this issue Sep 20, 2021
Merged
bors added a commit to rust-lang-ci/rust that referenced this issue Sep 21, 2021
@bors
Auto merge of rust-lang#89125 - Aaron1011:remove-intercrate-cache, r=…
7743c9f 
…jackh726

Don't use projection cache or candidate cache in intercrate mode

Fixes rust-lang#88969

It appears that *just* disabling the evaluation cache (in rust-lang#88994)
leads to other issues involving intercrate mode caching. I suspect
that since we now always end up performing the full evaluation
in intercrate mode, we end up 'polluting' the candidate and projection
caches with results that depend on being in intercrate mode in some way.
Previously, we might have hit a cached evaluation (stored during
non-intercrate mode), and skipped doing this extra work in
intercrate mode.

The whole situation with intercrate mode caching is turning into
a mess. Ideally, we would remove intercrate mode entirely - however,
this might require waiting on Chalk.
@bors bors closed this as completed in 6dbb9d4 Sep 21, 2021
@Mark-Simulacrum
Copy link
Member Author

Reopening -- @Aaron1011 can you make sure the appropriate PRs are beta-nominated?

@apiraino apiraino removed the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Sep 22, 2021
@lqd lqd mentioned this issue Sep 30, 2021
Merged
Manishearth added a commit to Manishearth/rust that referenced this issue Oct 1, 2021
@Manishearth
Rollup merge of rust-lang#89412 - lqd:zvariant-repro, r=Aaron1011
4571fea 
Add regression test for issues rust-lang#88969 and rust-lang#89119

This adds a regression test to complete rust-lang#89125, and thus for issues rust-lang#88969 and rust-lang#89119, which needed a test.

Used with multiple crates, [this](https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=f665e7e882059157e0f86cfb09c47187) minimized from `zvariant-2.8.0` reproduces the error on `nightly-2021-09-19`.

The test in this PR fails on master if the commit 6dbb9d4 from rust-lang#89125 is reverted, and passes otherwise since it's now fixed.

r? `@Aaron1011`
@Manishearth Manishearth mentioned this issue Oct 1, 2021
Merged
bors added a commit to rust-lang-ci/rust that referenced this issue Oct 1, 2021
@bors
Auto merge of rust-lang#89417 - Manishearth:rollup-j2gdu95, r=Manishe…
69eb996 
…arth

Rollup of 7 pull requests

Successful merges:

 - rust-lang#88838 (Do not suggest importing inaccessible items)
 - rust-lang#89251 (Detect when negative literal indices are used and suggest appropriate code)
 - rust-lang#89321 (Rebase resume argument projections during state transform)
 - rust-lang#89327 (Pick one possible lifetime in case there are multiple choices)
 - rust-lang#89344 (Cleanup lower_generics_mut and make span be the bound itself)
 - rust-lang#89397 (Update `llvm` submodule to fix function name mangling on x86 Windows)
 - rust-lang#89412 (Add regression test for issues rust-lang#88969 and rust-lang#89119 )

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
ehuss pushed a commit to ehuss/rust that referenced this issue Oct 4, 2021
@bors @ehuss
Auto merge of rust-lang#89125 - Aaron1011:remove-intercrate-cache, r=…
f8e5fda 
…jackh726

Don't use projection cache or candidate cache in intercrate mode

Fixes rust-lang#88969

It appears that *just* disabling the evaluation cache (in rust-lang#88994)
leads to other issues involving intercrate mode caching. I suspect
that since we now always end up performing the full evaluation
in intercrate mode, we end up 'polluting' the candidate and projection
caches with results that depend on being in intercrate mode in some way.
Previously, we might have hit a cached evaluation (stored during
non-intercrate mode), and skipped doing this extra work in
intercrate mode.

The whole situation with intercrate mode caching is turning into
a mess. Ideally, we would remove intercrate mode entirely - however,
this might require waiting on Chalk.
@Mark-Simulacrum
Copy link
Member Author

We've backported the relevant PRs I believe, so going to go ahead and close this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
regression-from-stable-to-beta Performance or correctness regression from stable to beta. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.56.0
Development

Successfully merging a pull request may close this issue.

Don't use projection cache or candidate cache in intercrate mode Aaron1011/rust
4 participants
@Aaron1011 @Mark-Simulacrum @apiraino @rustbot