MGCA: require #[type_const] on free consts too#152129
MGCA: require #[type_const] on free consts too#152129rust-bors[bot] merged 1 commit intorust-lang:mainfrom
Conversation
|
HIR ty lowering was modified cc @fmease |
2802eb1 to
63e1045
Compare
63e1045 to
0db0759
Compare
| note: the declaration must be marked with `#[type_const]` | ||
| --> $DIR/unmarked-free-const.rs:6:1 | ||
| | | ||
| LL | const N: usize = 4; |
There was a problem hiding this comment.
pointing to the definition site of the non-type const is rly nice :3 thanks
There was a problem hiding this comment.
something I was thinking of: what happens with non-local spans?
err.span_note(
self.tcx().def_span(def_id),
"the declaration must be marked with `#[type_const]`",
);I wasn't able to make it not print the source location - referring to a compiletest auxiliary crate printed the true source location, as did referring to std::path::MAIN_SEPARATOR
- does def_span return DUMMY_SP on unknown spans? does span_note handle DUMMY_SP cleanly?
- or is source just, always available, I guess? (I'm used to C# where it's not, haha)
Relatedly, printing "hey you need #[type_const] on this item" when the item is in std isn't super helpful, and also when it's in a crate you don't control - but if it's in a crate you do control, maybe you do want that message, so idk. In any case, I think if we decide to stabilize #[type_const] (maybe we don't), this error needs additional work (make it a proper autofix suggestion, etc.) so maybe it's fine to leave it for now.
There was a problem hiding this comment.
At the end of compiling a crate we'll serialize a bunch of information to disk to allow for accessing information in cross-crate circumstances. Spans of DefIds are one such case.
The source of other crates isnt available only some of the information we computed from it.
I agree that we probably want to customise the diagnostic here for cases when the DefId is non-local 🤔
There was a problem hiding this comment.
you can use DefId::is_local/DefId::as_local (https://doc.rust-lang.org/nightly/nightly-rustc/rustc_hir/def_id/struct.DefId.html#method.as_local) to figure out if a DefId is from the current crate or not
There was a problem hiding this comment.
ok im fancy and did a suggestion :3
0db0759 to
aa4441e
Compare
This comment has been minimized.
This comment has been minimized.
aa4441e to
6b927d7
Compare
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
6b927d7 to
e6f5b97
Compare
|
@bors r+ |
Rollup merge of #152129 - khyperia:require-type_const-on-free-consts, r=BoxyUwU MGCA: require #[type_const] on free consts too When investigating another issue, I discovered that following ICEs (the `const_of_item` query doesn't support non-type_const-marked constants and does a `span_delayed_bug`): ```rust #![feature(min_generic_const_args)] #![allow(incomplete_features)] const N: usize = 4; fn main() { let x = [(); N]; } ``` My initial thought of "only require `#[type_const]` on places that stable doesn't currently accept" ran into the issue of this compiling on stable today: ```rust trait Trait { const N: usize; } impl<const PARAM: usize> Trait for [(); PARAM] { const N: usize = PARAM; } fn main() { let x = [(); <[(); 4] as Trait>::N]; } ``` Figuring out which specific cases are not currently accepted by stable is quite hairy. Upon discussion with @BoxyUwU, she suggested that *all* consts, including free consts, should require `#[type_const]` to be able to be referred to. This is what this PR does. --- ~~The change to `tests/ui/const-generics/generic_const_exprs/non-local-const.rs` is unfortunate, reverting the fix in #143106 no longer fails the test. Any suggestions to test it more appropriately would be most welcome!~~ edit: never mind, figured out how compiletests work :) - verified that the new test setup correctly ICEs when that PR's fix is reverted. r? @BoxyUwU
When investigating another issue, I discovered that following ICEs (the
const_of_itemquery doesn't support non-type_const-marked constants and does aspan_delayed_bug):My initial thought of "only require
#[type_const]on places that stable doesn't currently accept" ran into the issue of this compiling on stable today:Figuring out which specific cases are not currently accepted by stable is quite hairy.
Upon discussion with @BoxyUwU, she suggested that all consts, including free consts, should require
#[type_const]to be able to be referred to. This is what this PR does.The change totests/ui/const-generics/generic_const_exprs/non-local-const.rsis unfortunate, reverting the fix in #143106 no longer fails the test. Any suggestions to test it more appropriately would be most welcome!edit: never mind, figured out how compiletests work :) - verified that the new test setup correctly ICEs when that PR's fix is reverted.
r? @BoxyUwU