Comments (13)
Thank you for opening this issue!
further minimized
pub trait Overlap {}
impl<T: Overlap + ?Sized> Overlap for &'static T {}
impl<T: LabelSet> Overlap for T
where
for<'a> T::Value<'a>: LabelGroup,
{}
pub trait LabelGroup {}
pub trait LabelSet {
type Value<'a>;
}
This pattern is #114061. Coherence now considers for<'a> <&'static ?x as LabelSet>::Value<'a>: LabelGroup
to be unknowable, as downstream crates could implement LabelSet
for &'static LocalType
where there's an impl of LabelGroup
for LocalType
. This should break. The FCP in #117164 (comment) did not mention that this part of the change will immediately be a hard error instead of a future-compat lint.
We use orphan_check_trait_ref
in two places: for the orphan check itself, and when checking whether some trait goal is "knowable" We only added the future compat lint behavior to the orphan check, always using the correct approach in trait_ref_is_knowable
. This is the first regression caused by that.
rust/compiler/rustc_trait_selection/src/traits/coherence.rs
Lines 944 to 948 in d43930d
Sorry for not mentioning this in the original FCP. I was aware of this immediate breakage but did not mention it.
Changing this to also be a future compat lint is annoying and I intentionally did not suggest this in #117164, as that is "inside of the trait solver" and has a smaller impact than the changes to the orphan check itself.
I would like to not revert this change as there have been some small follow-up PRs and I want to close these unsoundnesses asap. I propose accepting this breakage and to close this issue after adding it as a test. However, given that this has slipped through the FCP process, I am open to revert and re-land with another FCP in case there are any concerns by a @rust-lang/types member.
@rfcbot fcp close
from rust.
Yeah, that's understandable. Sorry about that!
#125763 (comment) was more addressed to T-types, not to you specifically.
from rust.
yes, we did no catch the breakage in your crate when running crater, unsure why that was the case 🤔
- https://crater-reports.s3.amazonaws.com/pr-117164-3/index.html
- https://crater-reports.s3.amazonaws.com/pr-121848-2/index.html
Generally we try to open fixes or at least issues to affected projects when landing breaking changes. This relies on us actually detecting these changes however 😅
from rust.
the PR was FCPd, so I expect this to be expected breakage, but @lcnr @fmease in case it's not.
from rust.
Team member @lcnr has proposed to close this. The next step is review by the rest of the tagged team members:
No concerns currently listed.
Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!
See this document for info about what commands tagged team members can give me.
from rust.
I'd like to note that this kind of code would've gotten rejected with the stabilization of -Znext-solver=coherence
(#121848) even without #117164 which was explicitly mentioned in #121848 (Ctrl+F "#114061" or "Unknowable candidates for higher ranked trait goals"). So technically speaking this has already been accepted by FCP (#121848 (comment)).
You can verify this yourself by running rustc +nightly-2024-04-30 mcve.rs --crate-type=lib -Znext-solver=coherence
.
from rust.
I'm fine with the decision but it would have been nicer to have some warning with info
from rust.
yes, we did no catch the breakage in your crate when running crater, unsure why that was the case 🤔
Strange. This particular impl has been on crates.io since February 10 🤔 those crater runs were Feb-27 and Apr-27 respectively and measured
is not in the dataset at all. Very strange.
from rust.
yes, we did no catch the breakage in your crate when running crater, unsure why that was the case 🤔
yeah it's not the first time this happened, there's rust-lang/crater#727 and discussions on zulip IIRC
from rust.
from rust.
@nikomatsakis Your vote didn't register for some reason. #125763 (comment)
edit by @lcnr: I manually checked nikos box now
from rust.
🔔 This is now entering its final comment period, as per the review above. 🔔
from rust.
The final comment period, with a disposition to close, as per the review above, is now complete.
As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.
from rust.
Related Issues (20)
- Words in `<code></code>` in docs make machine translation even weirder than usual HOT 3
- ICE: `assert_eq!(alloc.mutability, mutability)` failed in `validity.rs` HOT 4
- Confusing "the caller chooses a type which can be different" [E308] HOT 4
- Async code causes error "one type is more general than the other" (expected `async` block, found that same `async` block) when demanding an `impl Send` HOT 1
- Extremely un-informative error when a future that capture the environment is used as Send HOT 1
- Async code causes error "implementation of `FnOnce` is not general enough" when demanding an `impl Send` HOT 1
- `tidy` requires `src/tools/rustc-perf`? HOT 2
- 1.79 regression in `const` temporary lifetime extension HOT 10
- diagnostic refers to the wrong object's lifetime
- compile fail when using async_drop HOT 2
- Compiler Error E0530 When Shadowing None with mut keyword HOT 2
- `remove_dir_all` fails removing directores (with subdirectories) on network drives (SFTP) under windows HOT 28
- Two subsequent labels and unnecessary `jmp` with `opt-level=s` HOT 6
- Can the rust/build/x86_64-unknown-linux-gnu/stage0 directory be generated without network? HOT 2
- Strange TcpStream TTL type implementation (set_ttl/ttl) HOT 8
- Lint against using `libc::c_*` types in std if they're also avaliable in `core::ffi`
- `std::process::exit` is not thread-safe HOT 132
- compiletest: `./x test <test-name>.stderr` (or existing non-`.rs` file) should warn or error HOT 1
- rustc hangs / infinite loop on small example with generic_const_exprs and specialization HOT 1
- integer_decode() for float: example usage in docs doesn't make sense HOT 2
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from rust.