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

regression: constants cannot refer to interior mutable data #123281

Open
Mark-Simulacrum opened this issue Mar 31, 2024 · 21 comments
Open

regression: constants cannot refer to interior mutable data #123281

Mark-Simulacrum opened this issue Mar 31, 2024 · 21 comments
Labels
A-const-eval Area: Constant evaluation (MIR interpretation) regression-from-stable-to-stable Performance or correctness regression from one stable version to another. relnotes Marks issues that should be documented in the release notes of the next release. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Milestone

Comments

@Mark-Simulacrum
Copy link
Member

[INFO] [stdout] error[E0492]: constants cannot refer to interior mutable data
[INFO] [stdout]    --> src/lib.rs:583:43
[INFO] [stdout]     |
[INFO] [stdout] 583 |     const VTABLE: &'static Self::VTable = &(*Lhs::VTABLE, *Rhs::VTABLE);
[INFO] [stdout]     |                                           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ this borrow of an interior mutable value may end up in the final value
[INFO] [stdout] 
[INFO] [stdout] 
[INFO] [stdout] error[E0492]: constants cannot refer to interior mutable data
[INFO] [stdout]    --> src/lib.rs:587:43
[INFO] [stdout]     |
[INFO] [stdout] 587 |     const VTABLE: &'static Self::VTable = &(*Lhs::VTABLE, *Rhs::VTABLE);
[INFO] [stdout]     |                                           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ this borrow of an interior mutable value may end up in the final value
[INFO] [stdout] 
@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 Mar 31, 2024
@Mark-Simulacrum Mark-Simulacrum added this to the 1.78.0 milestone Mar 31, 2024
@rustbot rustbot added I-prioritize Issue: Indicates that prioritization has been requested for this issue. needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Mar 31, 2024
@Noratrieb
Copy link
Member

probably relevant to @oli-obk @RalfJung

@RalfJung
Copy link
Member

Yeah I think this is a duplicate of #121250, which was filed by the author of the affected crate. Cc @sarah-ek

@RalfJung
Copy link
Member

RalfJung commented Mar 31, 2024

This got FCP'd by t-lang as acceptable breakage here, also based on these crater results.

@Mark-Simulacrum
Copy link
Member Author

Sounds good. I'll leave this open in case we decide to include some future-compat release note for it but untag for now.

@Mark-Simulacrum Mark-Simulacrum removed I-prioritize Issue: Indicates that prioritization has been requested for this issue. needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Mar 31, 2024
@apiraino
Copy link
Contributor

apiraino commented Apr 2, 2024

I'll preemptively label this breaking change (see prev. comment) for release notes and nominate for T-lang to discuss if these changes should be mentioned in the release notes.

@rustbot label +relnotes +I-lang-nominated

@rustbot rustbot added I-lang-nominated Nominated for discussion during a lang team meeting. relnotes Marks issues that should be documented in the release notes of the next release. labels Apr 2, 2024
@jieyouxu jieyouxu added the A-const-eval Area: Constant evaluation (MIR interpretation) label Apr 3, 2024
@tmandry
Copy link
Member

tmandry commented May 1, 2024

We discussed this in the lang team meeting and agreed that we should mention it in the release notes, since it is a compatibility change that could affect someone.

@rustbot label -I-lang-nominated

@rustbot rustbot removed the I-lang-nominated Nominated for discussion during a lang team meeting. label May 1, 2024
@p-avital
Copy link

p-avital commented May 2, 2024

We discussed this in the lang team meeting and agreed that we should mention it in the release notes, since it is a compatibility change that could affect someone.

@rustbot label -I-lang-nominated

I confirm, stabby is impacted and not having core::marker::Freeze available means there's no way for me to fix it either :(

@RalfJung
Copy link
Member

RalfJung commented May 2, 2024

Which versions of stabby are affected? Any idea why did this not show up in crater?

@p-avital
Copy link

p-avital commented May 2, 2024

Which versions of stabby are affected? Any idea why did this not show up in crater?

All of them, but stabby would detect that it's being compiled in nightly and use core::marker::Freeze to be usable in nightly.

I'm currently trying to find other workarounds, since core::marker::Freeze is still unstable... Any hints?

@RalfJung
Copy link
Member

RalfJung commented May 2, 2024

Ah... it's bad practice to automatically use nightly features (Cc #120804), and this is one example why.

@RalfJung
Copy link
Member

RalfJung commented May 2, 2024

I don't know any work-arounds beyond what was discussed in #121250 (adding indirections to avoid reborrowing unknown data). This used to work by accident (we never wanted code like yours to compile), and we fixed that accident. We used crater to check that the impact is minimal, and it was -- only a single crate failed, and that one did have a work-around. Freeze is the intended way to be able to write such code in the future.

I'm sorry this slipped through our crater checks. I am not sure there's much we could have done here though.

@p-avital
Copy link

p-avital commented May 2, 2024

Yeah, I was a bit overly optimistic about this fix being stabilized in a similar timeframe to core::marker::Freeze... Any ETA on that one?

@RalfJung
Copy link
Member

RalfJung commented May 2, 2024

We don't really have ETAs for nightly features. I currently don't have the capacity to try and push for this feature. No blocking concerns are listed but also this wasn't really widely discussed, so it's possible that there should be an RFC first.

@p-avital
Copy link

p-avital commented May 2, 2024

Doesn't crater also run on beta? I was hoping this would let crater catch it when the time would come, since beta disables nightly features.

@RalfJung
Copy link
Member

RalfJung commented May 2, 2024

I don't know how the beta crater runs are done, but stabby doesn't show up in https://crater-reports.s3.amazonaws.com/beta-1.78-1/index.html.

@RalfJung
Copy link
Member

RalfJung commented May 2, 2024

I did find this though: https://crater-reports.s3.amazonaws.com/beta-1.78-1/1.77.0/reg/stabby-abi-2.0.1/log.txt. stabby-abi failed to build even with 1.77 so when it also fails with the beta that's not a regression. The beta build seemes to have failed for yet another reason: https://crater-reports.s3.amazonaws.com/beta-1.78-1/beta-2024-03-24/reg/stabby-abi-2.0.1/log.txt.

(Confusingly the 2nd build log starts with "testing stabby-abi-2.0.1 against 1.77.0 for beta-1.78-1" even though it is supposed to be building against beta-2024-03-24... I guess we should ignore everything above "testing stabby-abi-2.0.1 against beta-2024-03-24 for beta-1.78-1"? @Mark-Simulacrum is it expected that the beta-XXX build log starts with a full copy of the stable build log?)

@p-avital
Copy link

p-avital commented May 2, 2024

I was just gonna report that stabby indeed doesn't build on beta.

So crater ran on an obsolete version of stabby (which predated the u128 ABI change), that explains why it didn't show up as regression :(

@RalfJung
Copy link
Member

RalfJung commented May 2, 2024

Yeah no idea why crater picked up stabby-abi 2.0.1. I can't even find stabby itself.

@p-avital
Copy link

p-avital commented May 2, 2024

Could be because stabby depends on stabby-abi, so if the later doesn't compile, no use checking the former?

The issue around version does make me sad, because stabby would then have been spotted :(

@RalfJung
Copy link
Member

RalfJung commented May 2, 2024

Could be because stabby depends on stabby-abi, so if the later doesn't compile, no use checking the former?

    {
      "name": "stabby-2.0.1",
      "url": "https://crates.io/crates/stabby/2.0.1",
      "krate": {
        "Registry": {
          "name": "stabby",
          "version": "2.0.1"
        }
      },
      "status": "",
      "res": "skipped",
      "runs": [
        null,
        null
      ]
    },

Looks like it.

@p-avital
Copy link

p-avital commented May 2, 2024

Well, I'm gonna work around it with a static map of vtables. It's not ideal because it'll make any construction of a new trait object search/insert the corresponding vtable in a set, but it'll do until core::marker::Freeze gets stabilized.

p-avital added a commit to ZettaScaleLabs/stabby that referenced this issue May 2, 2024
p-avital added a commit to ZettaScaleLabs/stabby that referenced this issue May 3, 2024
yellowhatter added a commit to ZettaScaleLabs/zenoh that referenced this issue May 20, 2024
OlivierHecart added a commit to eclipse-zenoh/zenoh that referenced this issue May 22, 2024
* Fixed stabby ( rust-lang/rust#123281 )

* temp: use fixed stabby from branch

---------

Co-authored-by: OlivierHecart <[email protected]>
OlivierHecart pushed a commit to eclipse-zenoh/zenoh that referenced this issue May 23, 2024
* Fixed stabby ( rust-lang/rust#123281 )

* temp: use fixed stabby from branch

* fixed stabby package
@jieyouxu jieyouxu added the T-lang Relevant to the language team, which will review and decide on the PR/issue. label Sep 7, 2024
@Mark-Simulacrum Mark-Simulacrum added regression-from-stable-to-stable Performance or correctness regression from one stable version to another. and removed regression-from-stable-to-beta Performance or correctness regression from stable to beta. labels Oct 11, 2024
@rustbot rustbot added the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Oct 11, 2024
@tgross35 tgross35 removed the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Oct 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-const-eval Area: Constant evaluation (MIR interpretation) regression-from-stable-to-stable Performance or correctness regression from one stable version to another. relnotes Marks issues that should be documented in the release notes of the next release. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

9 participants