Skip to content

Fix incorrect impl JoinSemiLattice for State - #164081

Open
tmiasko wants to merge 1 commit into
rust-lang:mainfrom
tmiasko:fix-join
Open

Fix incorrect impl JoinSemiLattice for State#164081
tmiasko wants to merge 1 commit into
rust-lang:mainfrom
tmiasko:fix-join

Conversation

@tmiasko

@tmiasko tmiasko commented Oct 10, 2026 •
edited by rustbot
Loading

Copy link
Copy Markdown
Contributor

View all comments

The implementation had an incorrect short-circruting behavior, i.e., it failed to join borrow when qualif changed. This lead implementation to accept code that should have been rejected.

This fixes an ICE.

Fix the issue and fallout in the standard library and tests.

@rustbot

rustbot commented Oct 10, 2026

Copy link
Copy Markdown
Collaborator

Some changes occurred to constck

cc @fee1-dead

Some changes occurred to the CTFE machinery

cc @RalfJung, @oli-obk, @lcnr

@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. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Oct 10, 2026
@rustbot

rustbot commented Oct 10, 2026

Copy link
Copy Markdown
Collaborator

r? @petrochenkov

rustbot has assigned @petrochenkov.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

  • Owners of files modified in this PR: compiler
  • compiler expanded to 77 candidates
  • Random selection from 18 candidates

@tmiasko tmiasko mentioned this pull request Oct 10, 2026
Closed
RalfJung
RalfJung reviewed Oct 10, 2026
View reviewed changes
Comment thread compiler/rustc_const_eval/src/check_consts/resolver.rs
@RalfJung

RalfJung commented Oct 10, 2026

Copy link
Copy Markdown
Member

Thanks!

This is a breaking change, right? We should crater it to see whether we can just land it as-is or whether we should try to make the analysis smarter to reduce the fallout.
@bors try

@rust-bors

This comment has been minimized.

Sign in to view
rust-bors Bot pushed a commit that referenced this pull request Oct 10, 2026
@bors
Auto merge of #164081 - tmiasko:fix-join, r=<try>
c79f6bc 
Fix incorrect impl JoinSemiLattice for State
RalfJung
RalfJung reviewed Oct 10, 2026
View reviewed changes
Comment thread tests/ui/consts/qualif-indirect-mutation-fail.rs
RalfJung
RalfJung reviewed Oct 10, 2026
View reviewed changes
Comment thread library/core/src/mem/maybe_dangling.rs Outdated
RalfJung
RalfJung reviewed Oct 10, 2026
View reviewed changes
Comment thread library/coretests/tests/cell.rs Outdated
RalfJung
RalfJung reviewed Oct 10, 2026
View reviewed changes
Comment thread library/alloctests/tests/vec.rs Outdated
Comment on lines +2794 to +2801
let mut v = ManuallyDrop::new(Vec::with_capacity(6));
let mut x = 1;
while x < 42 {
v.push(x);
x *= 2;
}
assert!(v.len() == 6);
v.const_make_global()
ManuallyDrop::into_inner(v).const_make_global()

@RalfJung RalfJung Oct 10, 2026 •
edited by rustbot
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Uh, that's not very nice. At the very least this needs FIXME(const-hack). But I am not sure we should land this at all. Does it help if we enable const_precise_live_drops?

View changes since the review

@tmiasko tmiasko Oct 10, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes - assuming it is fine if we enable if for the whole test crate.

@RalfJung RalfJung Oct 10, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

IMO that's fine, yeah.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@tmiasko
Fix incorrect impl JoinSemiLattice for State
589fcb5 
The implementation had an incorrect short-circruting behavior, i.e., it
failed to join `borrow` when `qualif` changed. This lead implementation
to accept code that should have been rejected.

Fix the issue and fallout in the standard library and tests.
@rustbot

rustbot commented Oct 10, 2026

Copy link
Copy Markdown
Collaborator

⚠️ #[rustc_allow_const_fn_unstable] needs careful audit to avoid accidentally exposing unstable
implementation details on stable.

cc @rust-lang/wg-const-eval

@rust-bors

rust-bors Bot commented Oct 10, 2026

Copy link
Copy Markdown
Contributor

☀️ Try build successful (CI)
Build commit: c79f6bc (c79f6bc4fc2389852aeff667626cb3e58adfe71c)
Base parent: 34f4a80 (34f4a807e2d84184fff4e42d4539092569b6fcd6)

@RalfJung

RalfJung commented Oct 10, 2026

Copy link
Copy Markdown
Member

@craterbot check

@RalfJung

RalfJung commented Oct 10, 2026

Copy link
Copy Markdown
Member

@craterbot check
is the bot alive?

@craterbot

craterbot commented Oct 10, 2026

Copy link
Copy Markdown
Collaborator

🚨 Error: missing start toolchain

🆘 If you have any trouble with Crater please ask in t-infra on Zulip
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@RalfJung

RalfJung commented Oct 10, 2026

Copy link
Copy Markdown
Member

@craterbot run mode=check-only start=34f4a807e2d84184fff4e42d4539092569b6fcd6 end=c79f6bc4fc2389852aeff667626cb3e58adfe71c

@craterbot

craterbot commented Oct 10, 2026

Copy link
Copy Markdown
Collaborator

👌 Experiment pr-164081 created and queued.
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot craterbot added S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 10, 2026
@theemathas theemathas mentioned this pull request Oct 11, 2026
Draft
@theemathas

theemathas commented Oct 11, 2026

Copy link
Copy Markdown
Contributor

Crater rollup in #164148

@craterbot abort

@craterbot

craterbot commented Oct 11, 2026

Copy link
Copy Markdown
Collaborator

🚨 Error: database is locked

🆘 If you have any trouble with Crater please ask in t-infra on Zulip
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@theemathas

theemathas commented Oct 11, 2026

Copy link
Copy Markdown
Contributor

@craterbot abort

@craterbot

craterbot commented Oct 11, 2026

Copy link
Copy Markdown
Collaborator

🗑️ Experiment pr-164081 deleted!

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot craterbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-crater Status: Waiting on a crater run to be completed. labels Oct 11, 2026

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@RalfJung RalfJung RalfJung left review comments
@nnethercote nnethercote nnethercote left review comments

At least 1 approving review is required to merge this pull request.

Assignees

@petrochenkov petrochenkov

Labels

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. T-libs Relevant to the library team, which will review and decide on the PR/issue.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@tmiasko @rustbot @RalfJung @rust-log-analyzer @craterbot @theemathas @nnethercote @petrochenkov