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

Remove assert that checks type equality #115215

Merged
merged 1 commit into from
Sep 12, 2023
Merged

Conversation

ouz-a
Copy link
Contributor

@ouz-a ouz-a commented Aug 25, 2023

#112307 although this prevented unsound issues it also seems to introduce regressions #114858 is example of this regression. I locally tested this #114858 (comment) issue and failing assert is this.

This is also related to #115025

@rustbot
Copy link
Collaborator

rustbot commented Aug 25, 2023

r? @compiler-errors

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Aug 25, 2023
@compiler-errors
Copy link
Member

Ideally this would have a test. Is it impossible to minimize one from the example you linked?

@ouz-a
Copy link
Contributor Author

ouz-a commented Aug 25, 2023

Ideally this would have a test. Is it impossible to minimize one from the example you linked?

Spent some time trying to minimize it but, it's too interconnected. I didn't put a test for that reason, if we still need it I could add it.

}
}

for (_, value) in values.into_iter().enumerate() {
Copy link
Member

Choose a reason for hiding this comment

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

Can you modify this to a FIXME like below? And add a test from the issue that was recently minimized?

@compiler-errors
Copy link
Member

@rustbot author

@rustbot rustbot 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-review Status: Awaiting review from the assignee but also interested parties. labels Aug 30, 2023
@ouz-a ouz-a requested a review from compiler-errors August 30, 2023 07:52
@wesleywiser
Copy link
Member

Since #114858 is a stable-to-stable regression, should we consider backporting this to stable & beta once it is merged?

@apiraino
Copy link
Contributor

apiraino commented Sep 7, 2023

I'll go ahead and nominate this patch for backports, I guess beta=1.73.0 and stable=1.72.1 (or 1.73.0 if we don't make it for the point release of Sept. 14th)

@rustbot label +beta-nominated +stable-nominated

@rustbot rustbot added beta-nominated Nominated for backporting to the compiler in the beta channel. stable-nominated Nominated for backporting to the compiler in the stable channel. labels Sep 7, 2023
@lqd
Copy link
Member

lqd commented Sep 7, 2023

The test was added so this looks ready to review.

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 7, 2023
@lcnr
Copy link
Contributor

lcnr commented Sep 7, 2023

as a safer fix here, can you try to anonymize bound vars + erase regions with a FIXME to figure out why we have to erase regions here?

I think that should also fix the issue here while still preventing all actually harmful bugs

@apiraino
Copy link
Contributor

apiraino commented Sep 7, 2023

Beta and stable backports provisionally accepted once merged, as per compiler team on Zulip.

@rustbot label +beta-accepted +stable-accepted

@rustbot rustbot added beta-accepted Accepted for backporting to the compiler in the beta channel. stable-accepted Accepted for backporting to the compiler in the stable channel. labels Sep 7, 2023
@ouz-a
Copy link
Contributor Author

ouz-a commented Sep 7, 2023

After discussing details of the issue with the @lcnr they suggested me to keep this as warn!, since underlying issue seems to be, these two types should be same after erasing their regions but even after erasing them, their upvars are different which shouldn't be.

let local = mir::Local::from_usize(local);
let expected_ty = self.monomorphize(self.mir.local_decls[local].ty);
if expected_ty != op.layout.ty {
warn!("Unexpected initial operand type.See the issues/114858");
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
warn!("Unexpected initial operand type.See the issues/114858");
warn!("Unexpected initial operand type. See the issues/114858");

@@ -1,5 +1,6 @@
// run-pass
// check-pass
Copy link
Member

Choose a reason for hiding this comment

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

maybe let's do build-pass? Does this not get triggered by this code -- I would expect a stderr file here.

@ouz-a ouz-a force-pushed the mir_issue branch 2 times, most recently from 79ed30f to 25cd191 Compare September 7, 2023 16:06
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@lcnr
Copy link
Contributor

lcnr commented Sep 12, 2023

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Sep 12, 2023

📌 Commit 3ec0165 has been approved by lcnr

It is now in the queue for this repository.

@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 12, 2023
@bors
Copy link
Contributor

bors commented Sep 12, 2023

⌛ Testing commit 3ec0165 with merge e5fedce...

@Mark-Simulacrum
Copy link
Member

@bors p=10

I'd like this landed quickly so we get testing on nightly prior to stable point release (currently aiming at 9/14).

@Mark-Simulacrum Mark-Simulacrum removed the stable-nominated Nominated for backporting to the compiler in the stable channel. label Sep 12, 2023
@bors
Copy link
Contributor

bors commented Sep 12, 2023

☀️ Test successful - checks-actions
Approved by: lcnr
Pushing e5fedce to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 12, 2023
@bors bors merged commit e5fedce into rust-lang:master Sep 12, 2023
11 checks passed
@rustbot rustbot added this to the 1.74.0 milestone Sep 12, 2023
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (e5fedce): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
2.3% [1.2%, 3.3%] 2
Regressions ❌
(secondary)
3.0% [1.6%, 3.9%] 3
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 2.3% [1.2%, 3.3%] 2

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 630.886s -> 632.565s (0.27%)
Artifact size: 317.92 MiB -> 317.90 MiB (-0.00%)

bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 12, 2023
…Simulacrum

[stable] 1.72.1 release

This backports:

*  Remove assert that checks type equality rust-lang#115215
*  implied bounds: do not ICE on unconstrained region vars rust-lang#115559
*  rustdoc: correctly deal with self ty params when eliding default object lifetimes rust-lang#115276
*  Stop emitting non-power-of-two vectors in (non-portable-SIMD) codegen rust-lang#115236
*  Normalize before checking if local is freeze in deduced_param_attrs rust-lang#114948

Some cherry-picks required merge conflict resolution, we'll see if I got it right based on CI (rustdoc fix and LLVM codegen test).

r? `@Mark-Simulacrum`
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 12, 2023
…Simulacrum

[stable] 1.72.1 release

This backports:

*  Remove assert that checks type equality rust-lang#115215
*  implied bounds: do not ICE on unconstrained region vars rust-lang#115559
*  rustdoc: correctly deal with self ty params when eliding default object lifetimes rust-lang#115276
*  Stop emitting non-power-of-two vectors in (non-portable-SIMD) codegen rust-lang#115236
*  Normalize before checking if local is freeze in deduced_param_attrs rust-lang#114948

Some cherry-picks required merge conflict resolution, we'll see if I got it right based on CI (rustdoc fix and LLVM codegen test).

r? `@Mark-Simulacrum`
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 13, 2023
…Simulacrum

[stable] 1.72.1 release

This backports:

*  Remove assert that checks type equality rust-lang#115215
*  implied bounds: do not ICE on unconstrained region vars rust-lang#115559
*  rustdoc: correctly deal with self ty params when eliding default object lifetimes rust-lang#115276
*  Stop emitting non-power-of-two vectors in (non-portable-SIMD) codegen rust-lang#115236
*  Normalize before checking if local is freeze in deduced_param_attrs rust-lang#114948

Some cherry-picks required merge conflict resolution, we'll see if I got it right based on CI (rustdoc fix and LLVM codegen test).

r? `@Mark-Simulacrum`
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 13, 2023
…Simulacrum

[stable] 1.72.1 release

This backports:

*  Remove assert that checks type equality rust-lang#115215
*  implied bounds: do not ICE on unconstrained region vars rust-lang#115559
*  rustdoc: correctly deal with self ty params when eliding default object lifetimes rust-lang#115276
*  Stop emitting non-power-of-two vectors in (non-portable-SIMD) codegen rust-lang#115236
*  Normalize before checking if local is freeze in deduced_param_attrs rust-lang#114948

Some cherry-picks required merge conflict resolution, we'll see if I got it right based on CI (rustdoc fix and LLVM codegen test).

r? `@Mark-Simulacrum`
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 13, 2023
…Simulacrum

[stable] 1.72.1 release

This backports:

*  Remove assert that checks type equality rust-lang#115215
*  implied bounds: do not ICE on unconstrained region vars rust-lang#115559
*  rustdoc: correctly deal with self ty params when eliding default object lifetimes rust-lang#115276
*  Stop emitting non-power-of-two vectors in (non-portable-SIMD) codegen rust-lang#115236
*  Normalize before checking if local is freeze in deduced_param_attrs rust-lang#114948

Some cherry-picks required merge conflict resolution, we'll see if I got it right based on CI (rustdoc fix and LLVM codegen test).

r? `@Mark-Simulacrum`
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 13, 2023
…Simulacrum

[stable] 1.72.1 release

This backports:

*  Remove assert that checks type equality rust-lang#115215
*  implied bounds: do not ICE on unconstrained region vars rust-lang#115559
*  rustdoc: correctly deal with self ty params when eliding default object lifetimes rust-lang#115276
*  Stop emitting non-power-of-two vectors in (non-portable-SIMD) codegen rust-lang#115236
*  Normalize before checking if local is freeze in deduced_param_attrs rust-lang#114948

Some cherry-picks required merge conflict resolution, we'll see if I got it right based on CI (rustdoc fix and LLVM codegen test).

r? `@Mark-Simulacrum`
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 13, 2023
…Simulacrum

[stable] 1.72.1 release

This backports:

*  Remove assert that checks type equality rust-lang#115215
*  implied bounds: do not ICE on unconstrained region vars rust-lang#115559
*  rustdoc: correctly deal with self ty params when eliding default object lifetimes rust-lang#115276
*  Stop emitting non-power-of-two vectors in (non-portable-SIMD) codegen rust-lang#115236
*  Normalize before checking if local is freeze in deduced_param_attrs rust-lang#114948

Some cherry-picks required merge conflict resolution, we'll see if I got it right based on CI (rustdoc fix and LLVM codegen test).

r? `@Mark-Simulacrum`
@Mark-Simulacrum Mark-Simulacrum removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Sep 16, 2023
@Mark-Simulacrum Mark-Simulacrum modified the milestones: 1.74.0, 1.73.0 Sep 16, 2023
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 17, 2023
…k-Simulacrum

[beta] backport

This PR backports:

- rust-lang#115785: Only suggest turbofish in patterns if we may recover
- rust-lang#115527: Don't require `Drop` for `[PhantomData<T>; N]` where `N` and `T` are generic, if `T` requires `Drop`
- rust-lang#115389: fix(resolve): update def if binding is warning ambiguity
- rust-lang#115215: Remove assert that checks type equality

r? `@Mark-Simulacrum`
@cuviper cuviper modified the milestones: 1.73.0, 1.72.1 Sep 19, 2023
mmastrac added a commit to denoland/deno that referenced this pull request Sep 20, 2023
> 1.72.1 resolves a few regressions introduced in 1.72.0:

> - [Partially revert codegen change, improving
codegen](rust-lang/rust#115236)
> - [rustdoc: Fix self ty params in objects with
lifetimes](rust-lang/rust#115276)
> - [Fix regression in compile
times](rust-lang/rust#114948)
> - Resolve some ICEs in the compiler:
>   - [#115215](rust-lang/rust#115215)
>   - [#115559](rust-lang/rust#115559)
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. stable-accepted Accepted for backporting to the compiler in the stable channel. 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.