Lower paths to functions in const args as ConstKind::Error

[Shourya742]

View all comments

closes: #138088

Lowering these to recovered FnDef consts currently interacts poorly with WF checking: WF of a FnDef walks the function signature, so a signature that mentions the same function item as a const arg can recurse until it overflows/segfaults. So, for now we are emitting ConstKind::Error

r? @BoxyUwU

[rustbot]

HIR ty lowering was modified

cc @fmease

[rustbot]

BoxyUwU is currently at their maximum review capacity.
They may take a while to respond.

[rustbot]

This PR changes a file inside tests/crashes. If a crash was fixed, please move into the corresponding ui subdir and add 'Fixes #' to the PR description to autoclose the issue upon merge.

[BoxyUwU]

in theory it should be possible to properly lower the path in a way that doesnt ICE and use that for recovering. I would prefer to do that if possible since eventually we do actually want to support function item types in const generics

can you try and figure out the right way to paths to lower associated functions? thx :3

[BoxyUwU]

@rustbot author

[rustbot]

changes to the core type system

cc @lcnr

[Shourya742]

I am lowering this via lower_generic_args_of_assoc_item. There are a couple of caveats here. First, it doesn't support explicit generics on paths. Also, when I initially used it, I got a segfault because of the recursive nature of this case:

trait Bar {
    fn x(&self) -> [i32; Bar::x];
    //~^ ERROR the constant `<Self as Bar>::x` is not of type `usize`
}

With my caveman debugging skills, I found that WF checking for a recovered FnDef could recursively walk the same function signature. The function output contained the same const argument, which lowered back to the same function item.

I added a guard to avoid re-walking the FnDef output when its def_id is the same as the current body_def_id, since that signature is already being checked. You can see this in the changes to the wf method.

@BoxyUwU, let me know what you think about this and whether this is even the correct way to approach it. I didn't try to make lower_generic_args_of_assoc_item smarter, considering the scope of this PR, but that can be done if needed.

View changes since the review

[BoxyUwU]

Oh my god that segfault is incredibly funny I hadn't considered that 🤔 definitely think that wf requiring wf(sig(fndef)) to hold for wf(fndef) to hold is wrong/should be changed long term but that's not something we can do right now or as part of this PR... lol

I'm not sure that the check for body_def_id is correct, in theory you could have a usage of Bar::x from within a different body, e.g. just doing fn main() { let _: [(); Bar::x] } probably circumvents that? I think body_def_id is also really supposed to be a diagnostics thing not something that affects the semantics of the type system (which changing wf requirements does :>)

I think this honestly just kinda kills the possibility of properly lowering this. I think this is also a problem for just DefKind::Fn below since this segfaults on current nightly before your PR:

#![feature(min_generic_const_args, macroless_generic_const_args)]

fn foo() -> [(); foo] { todo!() }

I think unfortunately we should probably just be handling DefKind::Fn and DefKind::AssocFn by returning ConstKind::Error and forgetting about trying to recover this properly. We should add a FIXME explaining the segfault you ran into and track this work somewhere in the project-const-generics repo.

---

If we were to actually lower this correctly I think in theory you want to call lower_resolved_const_path instead of lower_generic_args_of_assoc_item (which will handle calling the latter for you). The AssocConst arm above is probably a good thing to base this off.

But, no point in lowering this correctly because of the segfault 😓 Thank u for discovering this and explaining it to me :3

[Shourya742]

If I understand correctly, we now want to stop trying to recover function items in const paths for both DefKind::Fn and DefKind::AssocFn, and instead lower them to ConstKind::Error to avoid the overflow/segfault cases.

I have made that change in this commit: ff7f240. I also added a FIXME explaining the issue.

If I misunderstood and we only want to change the associated function case, I can adjust it. Otherwise, if the FIXME looks okay to you, I can open an issue in the project-const-generics repo to track the proper lowering work.

[Shourya742]

@rustbot ready

[BoxyUwU]

@rustbot author

[BoxyUwU]

sorry for taking so long to get to this :>

[rust-bors]

💔 Test for 46208a3 failed: CI

[JonathanBrouwer]

@bors retry
Spurious