Make #[fundamental] only apply to the first argument of Box - #160162
Conversation
This comment has been minimized.
This comment has been minimized.
This comment was marked as resolved.
This comment was marked as resolved.
|
that's a problem ^^ ill check it out, ty. also realised there's another potential issue here wrt tuples since we definitely want those to be fundamental in all prams - oops. I'll just special case box i think instead. @rustbot author |
#[fundamental] only apply to the first argument#[fundamental] only apply to the first argument of Box
|
seems like the tuple thing was a false flag, this should be okay after all (tuples get their own path in coherence checking). and the error there was bc @rustbot ready |
|
AFAICT tuples aren't supposed to be fundamental: https://play.rust-lang.org/?version=nightly&mode=debug&edition=2024&gist=5a585806691e3be45b6be4e3d540089f |
|
Tuples are not listed here. https://doc.rust-lang.org/reference/glossary.html#fundamental-type-constructors |
This comment has been minimized.
This comment has been minimized.
|
|
|
There is a PR (#160181) about making Tuple fundamental. |
That's the tuple trait, the tuple types are what would cause issues here |
| ControlFlow::Break(OrphanCheckEarlyExit::LocalTy(ty)) | ||
| } else if def.is_fundamental() { | ||
| args.visit_with(self) | ||
| args.type_at(0).visit_with(self) |
There was a problem hiding this comment.
This is a pretty suble, easy-to-miss thing. Although it's a much bigger change, I would expect that we would do one of:
- Get rid of
#[fundamental]entirely and instead just match on theDefId(since all uses are lang items) - Mark args as fundamental and track those somewhere
There was a problem hiding this comment.
would an acceptable intermediary be special-casing Box in particular here & matching on the AdtLangItem?
I'm also willing to go ahead and do a per-arg implementation, but that would be more involved as you say & I'm not sure if it will be much help given that there are no users of that anywhere on the horizon
There was a problem hiding this comment.
A per arg impl implies attribute on the generic, like struct Box<#[fundamental] T, A> { .. }, right?
There was a problem hiding this comment.
- Get rid of
#[fundamental]entirely and instead just match on theDefId(since all uses are lang items)
I don't think this is a good idea, it would remove our ability to integration test this stuff.
We've also pointed the rust for linux folks at this (#136979 (comment)) for a potential solution to their orphan rule problems. And while we can of course change this thing however we want, I'd feel bad about removing the ability to unstably use it or even experiment with it, at least without discussing it first.
There was a problem hiding this comment.
I'd be okay to match on Box specifically (and/or the couple other known lang items, if we want to limit the fundamental there too), but just fall back to the current behavior for all other uses of #[fundamental]. That seems like the smallest and easiest solution here that is also clear.
(i.e. within the def.is_fundamental() arm, do an additional match on lang items)
There was a problem hiding this comment.
latest commit should address this ^^
this is why i shouldn't submit PRs on zero sleep :D |
|
Is it backwards-compatible to later make Box be fundamental on both its type parameters? |
|
Yes, though I strongly doubt that will be the case. The opinions in the lang meeting were unanimously in favour of making fundamentalness only apply to the T param, an opinion with which I entirely concur ^^ |
| @@ -0,0 +1,24 @@ | |||
| //@ compile-flags:--crate-name=test | |||
| //@ aux-build:coherence_lib.rs | |||
|
|
|||
There was a problem hiding this comment.
This test should likely be named for box, and there should be a comment.
|
cc @rust-lang/types We're reducing the scope of |
|
@bors squash |
This comment has been minimized.
This comment has been minimized.
* fundamental only on first arg * woo this made error messages nicer * oh the test was misnamed lol * special case only box * rename tests Co-authored-by: Nia Deckers <nia-e@haecceity.cc>
|
🔨 5 commits were squashed into 4af1e41. |
bab642f to
4af1e41
Compare
|
@bors r=jackh726 |
Is this the case? I vaguely remember that adding and removing fundamental was a breaking change. I think with this PR |
|
We'll need lang signoff to stabilise that field in the first place, so I'll note that point when I ask for FCP on that - I doubt it's something they'd want to go back on, but ty for raising it ^^ |
|
@nia-e can you ensure that question makes its way to the tracking issue for later consideration? |
Make `#[fundamental]` only apply to the first argument of `Box` Per a [discussion with lang](https://rust-lang.zulipchat.com/#narrow/channel/213817-t-lang/topic/.60.23.5Bfundamental.5D.60.20and.20.60Box.3CT.2C.20A.3E.60/with/613236766), it was decided that we want `Box<T, A>` to only be fundamental in `T`. ~~Given that `Box` is the only fundamental type with two generic params, simply make it so only the first param is considered for fundamentalness.~~ ~~`Tuple` is also fundamental, but has its own path in coherence checking and so isn't impacted by this; `Box` is the only thing that is.~~ Since this is just an internal detail and `#[fundamental]` is not in a rush to get stabilised, lang's opinion was that t-compiler has free rein on how to implement this so I figured I'd go with the most straightforward implementation for now. The main alternative I see would be supporting param-position `#[fundamental]` & have the semantics that: ```rust #[fundamental] struct Foo<T, U>(T, U); ``` is equivalent to ```rust struct Foo<#[fundamental] T, #[fundamental] U>(T, U); ``` though given that there's no usecase for this right now, it seemed somewhat unnecessary. r? compiler
Make `#[fundamental]` only apply to the first argument of `Box` Per a [discussion with lang](https://rust-lang.zulipchat.com/#narrow/channel/213817-t-lang/topic/.60.23.5Bfundamental.5D.60.20and.20.60Box.3CT.2C.20A.3E.60/with/613236766), it was decided that we want `Box<T, A>` to only be fundamental in `T`. ~~Given that `Box` is the only fundamental type with two generic params, simply make it so only the first param is considered for fundamentalness.~~ ~~`Tuple` is also fundamental, but has its own path in coherence checking and so isn't impacted by this; `Box` is the only thing that is.~~ Since this is just an internal detail and `#[fundamental]` is not in a rush to get stabilised, lang's opinion was that t-compiler has free rein on how to implement this so I figured I'd go with the most straightforward implementation for now. The main alternative I see would be supporting param-position `#[fundamental]` & have the semantics that: ```rust #[fundamental] struct Foo<T, U>(T, U); ``` is equivalent to ```rust struct Foo<#[fundamental] T, #[fundamental] U>(T, U); ``` though given that there's no usecase for this right now, it seemed somewhat unnecessary. r? compiler
Rollup of 14 pull requests Successful merges: - #159245 (Emit retags in codegen to support BorrowSanitizer (part 5)) - #159864 (Report "capacity overflow" for oversized Rc<[T]>/Arc<[T]>) - #160079 (make atomic operations const) - #160124 (Structurally prevent zero-count `BackendRepr::SimdVector`s) - #160162 (Make `#[fundamental]` only apply to the first argument of `Box`) - #160210 (Remove an outdated FIXME) - #160282 (Improve diagnostic for patterns in function pointer types) - #157928 (Eagerly fetch typeck results when linting) - #159672 (Improve suggestions when multiples tuples implement the same trait) - #159861 (Add documentation for the `non_exhaustive` attribute) - #159907 (Fix `hidden_glob_reexports` in `rustc_ast`) - #159998 (Align expect messages with guidance) - #160145 (Expand checks for register_tool) - #160307 (Update `minifier` version to `0.4.0`)
Rollup merge of #160162 - nia-e:per-arg-fundamental, r=jackh726 Make `#[fundamental]` only apply to the first argument of `Box` Per a [discussion with lang](https://rust-lang.zulipchat.com/#narrow/channel/213817-t-lang/topic/.60.23.5Bfundamental.5D.60.20and.20.60Box.3CT.2C.20A.3E.60/with/613236766), it was decided that we want `Box<T, A>` to only be fundamental in `T`. ~~Given that `Box` is the only fundamental type with two generic params, simply make it so only the first param is considered for fundamentalness.~~ ~~`Tuple` is also fundamental, but has its own path in coherence checking and so isn't impacted by this; `Box` is the only thing that is.~~ Since this is just an internal detail and `#[fundamental]` is not in a rush to get stabilised, lang's opinion was that t-compiler has free rein on how to implement this so I figured I'd go with the most straightforward implementation for now. The main alternative I see would be supporting param-position `#[fundamental]` & have the semantics that: ```rust #[fundamental] struct Foo<T, U>(T, U); ``` is equivalent to ```rust struct Foo<#[fundamental] T, #[fundamental] U>(T, U); ``` though given that there's no usecase for this right now, it seemed somewhat unnecessary. r? compiler
View all comments
Per a discussion with lang, it was decided that we want
Box<T, A>to only be fundamental inT.Given thatBoxis the only fundamental type with two generic params, simply make it so only the first param is considered for fundamentalness.Tupleis also fundamental, but has its own path in coherence checking and so isn't impacted by this;Boxis the only thing that is.Since this is just an internal detail and
#[fundamental]is not in a rush to get stabilised, lang's opinion was that t-compiler has free rein on how to implement this so I figured I'd go with the most straightforward implementation for now. The main alternative I see would be supporting param-position#[fundamental]& have the semantics that:is equivalent to
though given that there's no usecase for this right now, it seemed somewhat unnecessary.
r? compiler