Stream: t-compiler/wg-rls-2.0

Topic: Properly qualify argument types in generated functions


Timo Freiberg (Apr 22 2020 at 13:32, on Zulip):

I'm trying to tackle the next issue of the add_function assist: properly qualifying the types of the generated arguments! (see currently ignored test case for that)
I guess code_model::Module::find_use_path should be able to handle this, but I have no idea how to get the corresponding ItemInNs for a Type here so I can call that method.
Any help?

Timo Freiberg (Apr 22 2020 at 16:51, on Zulip):

I probably want to use QualifyPaths... the API looks quite usable, I'm doing something wrong though. I'm gonna dbg!-explore it a bit and probably find my mistake soon :)

Timo Freiberg (Apr 22 2020 at 17:15, on Zulip):

Okay, I think I know what the problem is, unfortunately I'm stumped.
self.source_scope.resolve_hir_path(&hir_path?) returns None, so the source_scope I'm passing to QualifyPaths can't resolve the path.
I'm not really sure what SemanticsScope I need to pass.
Right now I'm creating it by calling ctx.sema.scope() with the CallExpr where the assist was triggered.
Do I need to find the scope where the argument type was defined instead?

Florian Diebold (Apr 22 2020 at 17:29, on Zulip):

hmm ideally I think we'd want something that prints a type with qualified paths, but that doesn't exist yet

Florian Diebold (Apr 22 2020 at 17:30, on Zulip):

but probably we can't get around that in this case

Florian Diebold (Apr 22 2020 at 17:31, on Zulip):

i.e. it'd be somewhat like HirDisplay, but qualify paths, only produce things that are valid rust code (i.e. no {unknown} or closure types), and maybe return a syntax node instead of a string

Timo Freiberg (Apr 22 2020 at 17:40, on Zulip):

yeah i would like to have something like that :) i wasn't sure if the existing functionality covered that usecase already - it seems similar to generating qualified paths in the add_missing_impl_members assist (example)

Timo Freiberg (Apr 22 2020 at 17:42, on Zulip):

I'm currently trying to find out why the scope I'm passing to resolve_hir_path doesn't find the path to the type as it's not really obvious to me - I reached path_resolution::resolve_path_fp_with_macro and my brain is full...

Florian Diebold (Apr 22 2020 at 17:48, on Zulip):

the difference is that add_missing_impl_members just copies the written types from the trait declaration and qualifies them, but your assist can't do that

Florian Diebold (Apr 22 2020 at 17:49, on Zulip):

I'm currently trying to find out why the scope I'm passing to resolve_hir_path doesn't find the path to the type as it's not really obvious to me

if you're trying to resolve something you got from HirDisplaying a type, I think that's unlikely to work

Timo Freiberg (Apr 22 2020 at 17:50, on Zulip):

that's unfair :sweat_smile:

Florian Diebold (Apr 22 2020 at 17:50, on Zulip):

I mean, if it resolved then you wouldn't need to qualify it...

Timo Freiberg (Apr 22 2020 at 17:51, on Zulip):

but it even fails in my testcase where the displayed type matches the definition

Timo Freiberg (Apr 22 2020 at 17:51, on Zulip):

hmm that's a very good point

Florian Diebold (Apr 22 2020 at 17:51, on Zulip):

also, the Semantics stuff doesn't really work with syntax nodes you built yourself

Timo Freiberg (Apr 22 2020 at 17:51, on Zulip):

ok the failures start to make sense

Timo Freiberg (Apr 22 2020 at 17:52, on Zulip):

so, there's definitely some missing functionality somewhere in hir for that, right?

Florian Diebold (Apr 22 2020 at 17:53, on Zulip):

yes

Timo Freiberg (Apr 22 2020 at 17:54, on Zulip):

so I want to take a Type, find out where it's defined (does that already exist?) and compute a path from the module where the function will be generated to the type definition.
and display that

Timo Freiberg (Apr 22 2020 at 17:55, on Zulip):

i'm gonna try to get something working, any guidance is very welcome :)

Florian Diebold (Apr 22 2020 at 17:57, on Zulip):

well, the idea would be to either build something similar to HirDisplay, or add a parameter to it that makes it qualify paths

Florian Diebold (Apr 22 2020 at 17:57, on Zulip):

it would need the ModuleId from where it's supposed to qualify the paths

Florian Diebold (Apr 22 2020 at 17:57, on Zulip):

and then it could use find_path to find the proper path

Florian Diebold (Apr 22 2020 at 17:58, on Zulip):

the relevant place for qualifying structs etc. would be here, instead of just taking the name you'd use find_path

Florian Diebold (Apr 22 2020 at 18:00, on Zulip):

this is all behind the "code model" facade, so you're working with Ty and ModuleId and so on instead of Type and Module, and you need to expose it in Type in the end

Florian Diebold (Apr 22 2020 at 18:07, on Zulip):

I think in the end we'll want a separate facility for this, since printing types 'properly' has various ways it can fail (the type isn't reachable from where we are, the type contains unnameable types...), so we'll want it to return Option or maybe a Result in the end (and while we're at it we could have it return a syntax node instead of a string), but as a first step it's probably easier to just hack it into the existing thing

Timo Freiberg (Apr 22 2020 at 18:27, on Zulip):

thanks a lot! i'm going to digest this now :)

Timo Freiberg (Apr 22 2020 at 21:06, on Zulip):

Holy crap it works :) thanks!

Timo Freiberg (Apr 24 2020 at 16:53, on Zulip):

I see what you meant now, returning Option or Result is really hard with the way ToString uses fmt::Display, you can't even go super hacky and abuse fmt::Result :(
How about maybe parametrizing the new fallible parts with a trait where the old implementation has type Error = Infallible and the new implementation (used for rendering code into source files) gets an actual error type? then only HirDisplayWrapper<Old> would implement fmt::Display, and HirDisplayWrapper<New> would get it's own fn render(&self) -> Option<String> (or other return type)
The existing implementations of hir_fmt would still need to be changed of course, but maybe that way most of the printing logic could be defined only once

Timo Freiberg (Apr 27 2020 at 20:48, on Zulip):

I went ahead and tried that approach: https://github.com/rust-analyzer/rust-analyzer/pull/4175

Timo Freiberg (May 02 2020 at 12:40, on Zulip):

Hey Florian,
I'm not sure about your comment about using an enum instead of a trait to customize the HirDisplay behaviour.
Would you recommend to make hir_fmt always return the sum error type that's currently called FormatSourceCodeError and assert that the call to hir_fmt in the Display implementation didn't return any error other than fmt::Error?
That's the only way to use an enum instead of a trait I could think of, did I miss something?

Florian Diebold (May 02 2020 at 13:12, on Zulip):

yeah, that's what I was thinking

Timo Freiberg (May 02 2020 at 13:20, on Zulip):

Alright that makes sense, thanks for the quick reply. I'm gonna change that then :)

Last update: May 29 2020 at 17:50UTC