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

Hover on constants should render the display/debug impl if available #15188

Closed
Veykril opened this issue Jul 1, 2023 · 4 comments · Fixed by #15230
Closed

Hover on constants should render the display/debug impl if available #15188

Veykril opened this issue Jul 1, 2023 · 4 comments · Fixed by #15230
Labels
A-hover hover feature A-mir C-feature Category: feature request

Comments

@Veykril
Copy link
Member

Veykril commented Jul 1, 2023

Might be a crazy idea but I'll take the bitflags crate as an example here (although we fail to calculate the layout for it in const eval due to the projection used I believe, pasting the expansion and fixing that up makes it work):

bitflags::bitflags! {
    #[derive(Default, Debug, Clone, Copy)]
    pub(crate) struct TypeVariableFlags: u8 {
        const DIVERGING = 1 << 0;
        const INTEGER = 1 << 1;
        const FLOAT = 1 << 2;
        const FOO = TypeVariableFlags::FLOAT.bits() | TypeVariableFlags::INTEGER.bits();
    }
}

Hovering FOO renders (again <layout-error> currently) the following:

hir_ty::infer::unify::x
const Foo: TypeVariableFlags = TypeVariableFlags(InternalBitFlags(3))

which is obviously not that useful, meanwhile the debug implementation would render as:

TypeVariableFlags(INTEGER | FLOAT)

Noticed this in a project of mine that works with bitflags where our hover is practically useless
image

@Veykril Veykril added C-feature Category: feature request A-hover hover feature A-mir labels Jul 1, 2023
@Veykril
Copy link
Member Author

Veykril commented Jul 1, 2023

cc @HKalbasi maybe you already had this idea in your head yourself :^) Would be good to know if we can pursue this in the (near) future or not as I think this would be very useful

@HKalbasi
Copy link
Member

HKalbasi commented Jul 1, 2023

I think it is very doable now. We just need to run this:

::core::fmt::format(::core::fmt::Arguments::new_v1(
        &[""],
        &[::core::fmt::ArgumentV1::new(&(THE_CONST), ::core::fmt::Debug::fmt)],
    ));

And show its result. The only challanges are:

  • There is no body containing that code, so we need to manually call the interpreter internals (it won't be much problem I think)
  • Changes in the fmt infrastructure can break us. We can use the current displayer as a fallback, and those changes will break us in other ways anyway.

We also need something similar for displaying panic messages I think.

I can do this, but this looks like a good opportunity for increasing the "bus factor" of mir, maybe?

@Veykril
Copy link
Member Author

Veykril commented Jul 1, 2023

I'm fine with trying to tackle this myself (once I have time, I'm currently somewhat occupied with things).

We should fallback to our current approach no matter what if we have problems running the debug/display implementation.

@Veykril Veykril assigned Veykril and unassigned Veykril Jul 1, 2023
@Veykril
Copy link
Member Author

Veykril commented Jul 6, 2023

I don't think I'll have the capacity to put the work into figuring out our MIR stuff like this in the coming time. So feel free to implement this yourself if you want, I'd look into the PR to catch up on how it was done then (which should teach me more in a shorter time I think)

bors added a commit that referenced this issue Jul 7, 2023
Use debug impl in rendering const eval result

fix #15188
@bors bors closed this as completed in db0add1 Jul 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-hover hover feature A-mir C-feature Category: feature request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants