Skip to content
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

lazy_normalization_consts breaks unsize coercion in rare cases #78369

Open
lcnr opened this issue Oct 25, 2020 · 4 comments
Open

lazy_normalization_consts breaks unsize coercion in rare cases #78369

lcnr opened this issue Oct 25, 2020 · 4 comments
Assignees
Labels
A-lazy-normalization Area: Lazy normalization (tracking issue: #60471) C-bug Category: This is a bug. F-generic_const_exprs `#![feature(generic_const_exprs)]` requires-nightly This issue requires a nightly compiler in some way. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@lcnr
Copy link
Contributor

lcnr commented Oct 25, 2020

#![feature(lazy_normalization_consts)]
struct P<T: ?Sized>([u8; 1 + 4], T);

fn main() {
    let x: Box<P<[u8; 0]>> = Box::new(P(Default::default(), [0; 0]));
    let _: Box<P<[u8]>> = x;
}

This works on stable but fails with the following error when using lazy_normalization_consts

error[E0308]: mismatched types
 --> src/main.rs:6:27
  |
6 |     let _: Box<P<[u8]>> = x;
  |            ------------   ^ expected slice `[u8]`, found array `[u8; 0]`
  |            |
  |            expected due to this
  |
  = note: expected struct `Box<P<[u8]>>`
             found struct `Box<P<[u8; 0]>>`

For this to fail the struct has to contain a type referencing an unevaluated constant, struct P<T: ?Sized>([u8; 5], T) does work.

assigning to myself, but won't get to this right away. If you are interested in working on this before that feel free to do so.

cc @nikomatsakis @varkor

@lcnr lcnr added C-bug Category: This is a bug. A-lazy-normalization Area: Lazy normalization (tracking issue: #60471) F-lazy_normalization_consts labels Oct 25, 2020
@lcnr lcnr self-assigned this Oct 25, 2020
@JohnTitor JohnTitor added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Oct 26, 2020
@lcnr
Copy link
Contributor Author

lcnr commented Nov 16, 2020

The issue is here

for field in prefix_fields {
for arg in tcx.type_of(field.did).walk() {
if let Some(i) = maybe_unsizing_param_idx(arg) {
if unsizing_params.contains(i) {
return Err(Unimplemented);
}
}
}
}

in combination with

stack.extend(substs.iter().rev());

We currently look at all substs while we should only look at the generic params which are actually mentioned by the constant.
The correct solution would be to instead walk the AbstractConst in case it exists, but for that we would have to move that to rustc_middle and also need to access the TyCtxt everywhere which seems difficult.

I am currently thinking about moving AbstractConsts directly into ConstKind as the current ConstKind::Unevaluated is really unhelpful when dealing with anonymous constants. That would simplify dealing with them as we can then just treat them pretty much exactly like other projections, 🤔 would be a big change though, so it might make sense allocate some time to discuss this before doing anything stupid here...

cc @oli-obk

@lcnr lcnr added the requires-nightly This issue requires a nightly compiler in some way. label Nov 16, 2020
@oli-obk
Copy link
Contributor

oli-obk commented Nov 16, 2020

I don't think we can remove Unevaluated. There are cases where we can neither build an abstract const nor evaluate it to a concrete constant. And once we have an abstract const, we can't turn that abstract const into a concrete one, so we'd need to keep the information about the uneavluated source around anyway. Can you elaborate on the changes you were thinking about?

@lcnr
Copy link
Contributor Author

lcnr commented Nov 16, 2020

I am not too sure how to actually do this yet... probably by adding an optional reference to &'tcx [Node<'tcx>] to Unevaluated 🤔

we might not be able to just evaluate mir_abstract_const eagerly as that can cause query cycles though. I forgot what actually causes the cycles without lazy normalization

@oli-obk
Copy link
Contributor

oli-obk commented Nov 16, 2020

At that point you're just talking about caching, so one thing that we could also do is change the Unevaluated variant to be a tuple variant around an UnevaluatedConst which has methods for evaluating or converting to an abstract const, without exposing the internal fields anymore. The evaluation is already cached via the query system, and I think the abstract const building, too? So this would just be an API change to make it less painful to use.

bors added a commit to rust-lang-ci/rust that referenced this issue Aug 26, 2021
…r=nikomatsakis

lazily "compute" anon const default substs

Continuing the work of rust-lang#83086, this implements the discussed solution for the [unused substs problem](https://github.com/rust-lang/project-const-generics/blob/master/design-docs/anon-const-substs.md#unused-substs). As of now, anonymous constants inherit all of their parents generics, even if they do not use them, e.g. in `fn foo<T, const N: usize>() -> [T; N + 1]`, the array length has `T` as a generic parameter even though it doesn't use it. These *unused substs* cause some backwards incompatible, and imo incorrect behavior, e.g. rust-lang#78369.

---
We do not actually filter any generic parameters here and the `default_anon_const_substs` query still a dummy which only checks that
- we now prevent the previously existing query cycles and are able to call `predicates_of(parent)` when computing the substs of anonymous constants
- the default anon consts substs only include the typeflags we assume it does.

Implementing that filtering will be left as future work.

---

The idea of this PR is to delay the creation of the anon const substs until after we've computed `predicates_of` for the parent of the anon const. As the predicates of the parent can however contain the anon const we still have to create a `ty::Const` for it.

We do this by changing the substs field of `ty::Unevaluated` to an option and modifying accesses to instead call the method `unevaluated.substs(tcx)` which returns the substs as before. If the substs - now `substs_` -  of `ty::Unevaluated` are `None`, it means that the anon const currently has its default substs, i.e. the substs it has when first constructed, which are the generic parameters it has available. To be able to call `unevaluated.substs(tcx)` in a `TypeVisitor`, we add the non-defaulted method `fn tcx_for_anon_const_substs(&self) -> Option<TyCtxt<'tcx>>`. In case `tcx_for_anon_const_substs` returns `None`, unknown anon const default substs are skipped entirely.

Even when `substs_` is `None` we still have to treat the constant as if it has its default substs. To do this, `TypeFlags` are modified so that it is clear whether they can still change when *exposing* any anon const default substs. A new flag, `HAS_UNKNOWN_DEFAULT_CONST_SUBSTS`, is added in case some default flags are missing.

The rest of this PR are some smaller changes to either not cause cycles by trying to access the default anon const substs too early or to be able to access the `tcx` in previously unused locations.

cc `@rust-lang/project-const-generics`
r? `@nikomatsakis`
@BoxyUwU BoxyUwU added F-generic_const_exprs `#![feature(generic_const_exprs)]` and removed F-lazy_normalization_consts labels Sep 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-lazy-normalization Area: Lazy normalization (tracking issue: #60471) C-bug Category: This is a bug. F-generic_const_exprs `#![feature(generic_const_exprs)]` requires-nightly This issue requires a nightly compiler in some way. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

4 participants