mGCA: Validate const literal against expected type#152001
mGCA: Validate const literal against expected type#152001reddevilmidzy wants to merge 2 commits intorust-lang:mainfrom
Conversation
This comment has been minimized.
This comment has been minimized.
| tcx.at(span).lit_to_const(input) | ||
| } | ||
|
|
||
| fn const_lit_matches_ty(&self, kind: &LitKind, ty: Ty<'tcx>, neg: bool) -> bool { |
There was a problem hiding this comment.
I think in most cases we actually don't want to emit errors during HIR ty lowering for type mismatches here. E.g. if we have a true literal it doesn't really matter if the expected type is u32, we can still lower to a 0x1 valtree of type bool and then get a type checking error later.
Separately from this, we already do these checks in fn lit_to_const (just ICEing if they don't hold instead of a proper error) so it probably doesn't make sense to duplicate them here too 🤔
I think it would make sense for lit_to_const to return an Option<ty::Value<'tcx>> and stop ICEing/erroring altogether.
You could then:
- Move
const_lit_matches_tyto THIR building for patterns (the only place other than const-generics that callslit_to_const) and create aty::ConstKind::Errorif it'sfalse(or iflit_to_constreturnsNone) - Here in HIR ty lowering we can emit a proper error about needing more type information for the literal if
lit_to_constreturnsNone. And we can entirely ignore the possibility of type mismatches because type checking should take care of that for us
|
very cool :) thanks for working on this |
This comment has been minimized.
This comment has been minimized.
…=oli-obk Dont strip const blocks in array lengths r? oli-obk mGCA now handles const blocks by *always* handling them during `lower_expr_to_const_arg_direct` instead of *sometimes* stripping them out at parse time. This is just generally a lot clearer/nicer but also means parsing isn't lossy which is just straight up wrong. We now use `MgcaDisambiguation::Direct` for const blocks because we "directly" represent a const block as `hir::ConstArgKind::Anon` :> The only time that an anon const for const generics uses `MgcaDisambiguation::AnonConst` is for unbraced literals. Once we properly support literals in `hir::ConstArgKind` (see rust-lang#152139 rust-lang#152001) then `MgcaDisambiguation` can be renamed to `AnonConstKind` with `TypeSystem` and `NonTypeSystem` variants. We can also get rid of `mgca_direct_lit_hack`. I expect this to be a very nice cleanup :) Fixes rust-lang/rustfmt#6788 The diff relating to passing spans around is to avoid a bunch of mGCA diagnostics changing from `const {}` to `{}`. I'm not entirely sure why this was happening. cc @rust-lang/rustfmt How do I run the tests in the rustfmt repo from here? `x test rustfmt` only seems to run like 100 tests and doesn't result in a `target/issue-6788.rs` getting created. I've verified locally that this formats correctly though
…=oli-obk Dont strip const blocks in array lengths r? oli-obk mGCA now handles const blocks by *always* handling them during `lower_expr_to_const_arg_direct` instead of *sometimes* stripping them out at parse time. This is just generally a lot clearer/nicer but also means parsing isn't lossy which is just straight up wrong. We now use `MgcaDisambiguation::Direct` for const blocks because we "directly" represent a const block as `hir::ConstArgKind::Anon` :> The only time that an anon const for const generics uses `MgcaDisambiguation::AnonConst` is for unbraced literals. Once we properly support literals in `hir::ConstArgKind` (see rust-lang#152139 rust-lang#152001) then `MgcaDisambiguation` can be renamed to `AnonConstKind` with `TypeSystem` and `NonTypeSystem` variants. We can also get rid of `mgca_direct_lit_hack`. I expect this to be a very nice cleanup :) Fixes rust-lang/rustfmt#6788 The diff relating to passing spans around is to avoid a bunch of mGCA diagnostics changing from `const {}` to `{}`. I'm not entirely sure why this was happening. cc @rust-lang/rustfmt How do I run the tests in the rustfmt repo from here? `x test rustfmt` only seems to run like 100 tests and doesn't result in a `target/issue-6788.rs` getting created. I've verified locally that this formats correctly though
…=oli-obk Dont strip const blocks in array lengths r? oli-obk mGCA now handles const blocks by *always* handling them during `lower_expr_to_const_arg_direct` instead of *sometimes* stripping them out at parse time. This is just generally a lot clearer/nicer but also means parsing isn't lossy which is just straight up wrong. We now use `MgcaDisambiguation::Direct` for const blocks because we "directly" represent a const block as `hir::ConstArgKind::Anon` :> The only time that an anon const for const generics uses `MgcaDisambiguation::AnonConst` is for unbraced literals. Once we properly support literals in `hir::ConstArgKind` (see rust-lang#152139 rust-lang#152001) then `MgcaDisambiguation` can be renamed to `AnonConstKind` with `TypeSystem` and `NonTypeSystem` variants. We can also get rid of `mgca_direct_lit_hack`. I expect this to be a very nice cleanup :) Fixes rust-lang/rustfmt#6788 The diff relating to passing spans around is to avoid a bunch of mGCA diagnostics changing from `const {}` to `{}`. I'm not entirely sure why this was happening. cc @rust-lang/rustfmt How do I run the tests in the rustfmt repo from here? `x test rustfmt` only seems to run like 100 tests and doesn't result in a `target/issue-6788.rs` getting created. I've verified locally that this formats correctly though
…=oli-obk Dont strip const blocks in array lengths r? oli-obk mGCA now handles const blocks by *always* handling them during `lower_expr_to_const_arg_direct` instead of *sometimes* stripping them out at parse time. This is just generally a lot clearer/nicer but also means parsing isn't lossy which is just straight up wrong. We now use `MgcaDisambiguation::Direct` for const blocks because we "directly" represent a const block as `hir::ConstArgKind::Anon` :> The only time that an anon const for const generics uses `MgcaDisambiguation::AnonConst` is for unbraced literals. Once we properly support literals in `hir::ConstArgKind` (see rust-lang#152139 rust-lang#152001) then `MgcaDisambiguation` can be renamed to `AnonConstKind` with `TypeSystem` and `NonTypeSystem` variants. We can also get rid of `mgca_direct_lit_hack`. I expect this to be a very nice cleanup :) Fixes rust-lang/rustfmt#6788 The diff relating to passing spans around is to avoid a bunch of mGCA diagnostics changing from `const {}` to `{}`. I'm not entirely sure why this was happening. cc @rust-lang/rustfmt How do I run the tests in the rustfmt repo from here? `x test rustfmt` only seems to run like 100 tests and doesn't result in a `target/issue-6788.rs` getting created. I've verified locally that this formats correctly though
Rollup merge of #152234 - BoxyUwU:dont_strip_const_blocks, r=oli-obk Dont strip const blocks in array lengths r? oli-obk mGCA now handles const blocks by *always* handling them during `lower_expr_to_const_arg_direct` instead of *sometimes* stripping them out at parse time. This is just generally a lot clearer/nicer but also means parsing isn't lossy which is just straight up wrong. We now use `MgcaDisambiguation::Direct` for const blocks because we "directly" represent a const block as `hir::ConstArgKind::Anon` :> The only time that an anon const for const generics uses `MgcaDisambiguation::AnonConst` is for unbraced literals. Once we properly support literals in `hir::ConstArgKind` (see #152139 #152001) then `MgcaDisambiguation` can be renamed to `AnonConstKind` with `TypeSystem` and `NonTypeSystem` variants. We can also get rid of `mgca_direct_lit_hack`. I expect this to be a very nice cleanup :) Fixes rust-lang/rustfmt#6788 The diff relating to passing spans around is to avoid a bunch of mGCA diagnostics changing from `const {}` to `{}`. I'm not entirely sure why this was happening. cc @rust-lang/rustfmt How do I run the tests in the rustfmt repo from here? `x test rustfmt` only seems to run like 100 tests and doesn't result in a `target/issue-6788.rs` getting created. I've verified locally that this formats correctly though
e08830e to
cd5b323
Compare
| let bits = parse_float_into_scalar(n, *fty, neg)?; | ||
| ty::ValTree::from_scalar_int(tcx, bits) | ||
| } | ||
| (ast::LitKind::Char(c), ty::Char) => ty::ValTree::from_scalar_int(tcx, c.into()), |
There was a problem hiding this comment.
can you change most of the type patterns to be _
| (ast::LitKind::Char(c), ty::Char) => ty::ValTree::from_scalar_int(tcx, c.into()), | |
| (ast::LitKind::Char(c), _) => ty::ValTree::from_scalar_int(tcx, c.into()), |
I think we should produce a valtree regardless of the expected type if we can. i think the only arms that actually need to know the expected type would be unsuffixed integer literals. ah I guess byte strings also care
each arm should also return a Ty for what the type of the valtree is. I think we should rename the ty local to expected_ty or something to make it clear it could be different from the type of the valtree once you change this
This comment has been minimized.
This comment has been minimized.
|
Haha, ICE occurs in other tests... |
ac69206 to
ca77eab
Compare
| if preceding_args.is_empty() | ||
| && tcx.type_of(param.def_id).skip_binder().has_param() | ||
| { | ||
| let guar = self.lowerer.dcx().span_delayed_bug( | ||
| self.span, | ||
| "default type has params but no args", | ||
| ); | ||
| return Ty::new_error(tcx, guar).into(); | ||
| } |
There was a problem hiding this comment.
Added logic to avoid ICE in https://github.com/rust-lang/rust/blob/main/tests/ui/const-generics/generic_const_exprs/lit_type_mismatch.rs
The change in compiler/rustc_trait_selection/src/traits/wf.rs is for the same reason.
#![feature(adt_const_params)]
#![feature(generic_const_exprs)]
#![allow(incomplete_features)]
struct A<const B: () = 1, C>(C);
//~^ ERROR: generic parameters with a default must be trailing
//~| ERROR: mismatched types
struct Cond<const B: bool>;
struct Thing<T = Cond<0>>(T);
//~^ ERROR: mismatched types
impl Thing {}
fn main() {}| const FREE2: isize = FREE; | ||
| //~^ ERROR the constant `5` is not of type `isize` | ||
|
|
There was a problem hiding this comment.
I'm not sure if this error will go away.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
In ten crash tests, the ICE disappeared; however, in tests/crashes/117460.rs, tests/crashes/114212-2.rs and tests/crashes/114212.rs, adding the main function caused the ICE to occur again.
| @@ -0,0 +1,23 @@ | |||
| //! Regression test for <https://github.com/rust-lang/rust/issues/116554> | |||
There was a problem hiding this comment.
This issue was closed; is there a need to add further testing?
| @@ -1,14 +0,0 @@ | |||
| //@ known-bug: #101036 | |||
| #![feature(generic_const_exprs)] | |||
|
|
|||
This comment has been minimized.
This comment has been minimized.
Co-authored-by: Boxy <rust@boxyuwu.dev>
75e7e18 to
2b5c8b7
Compare
close: #151625
close: #150983