-
-
Notifications
You must be signed in to change notification settings - Fork 14.4k
MGCA: require #[type_const] on free consts too #152129
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
base: main
Are you sure you want to change the base?
MGCA: require #[type_const] on free consts too #152129
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pointing to the definition site of the non-type const is rly nice :3 thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok im fancy and did a suggestion :3
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i like it!
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+ |
…e-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 rust-lang#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