Stream: t-compiler/wg-rls-2.0

Topic: doing multiple analysis/replacement steps


Florian Diebold (Jan 10 2020 at 15:12, on Zulip):

So... I'm trying to get the "substitute type parameters" and the "qualify paths" steps in the "add missing impl members" assist to work together. The problem is that after we've qualified paths, the syntax node we're editing is of course not the actual syntax node anymore, so most analysis doesn't work on it anymore, in particular resolving type parameters.

One option of course would be to do the type parameter substitution purely on syntax, but I don't like that.

This is made harder by the fact that while the edits don't collide themselves (something is either a path to qualify or a type parameter), the paths we qualify can contain type arguments that need to be substituted. E.g. let's say we have Foo<T>, then when qualifying we replace that whole node by foo::Foo<T>, and the Tis a completely new node.

The only option I can currently think of is to merge both steps into one, which isn't nice but should at least work :thinking:

Any ideas, @matklad ?

matklad (Jan 10 2020 at 15:15, on Zulip):

Tough question.

I think what IntelliJ does in similar situations is that it creates temporary PSI which is attached to some real PSI via a context attributes. On that injected PSI, all the analysis works, so you can just mutate it in place....

matklad (Jan 10 2020 at 15:17, on Zulip):

First, I am confused by the order here.

It seems like we first should do subst, and then qualification? And not the other way around?

Florian Diebold (Jan 10 2020 at 15:18, on Zulip):

I think no, since we don't want to qualify the paths we substituted as if they were in the original module

Florian Diebold (Jan 10 2020 at 15:19, on Zulip):

I noticed this when trying it on chalk_solve::RustIrDatabase, it qualified the TypeFamily parameter to chalk_ir::TypeFamily even though we actually want our own one

matklad (Jan 10 2020 at 15:19, on Zulip):

Hm, but if we don't have to qualify the path, it won't be qualifed, no?

matklad (Jan 10 2020 at 15:19, on Zulip):

Could you show an example? I think I am just confused as two what goes where :D

Florian Diebold (Jan 10 2020 at 15:23, on Zulip):

so, we have in our chalk.rs:

struct TypeFamily {}
impl<'a, DB> chalk_solve::RustIrDatabase<TypeFamily> for ChalkContext<'a, DB> { ... }

but in chalk_solve, the name TypeFamily resolves to chalk_solve::family::TypeFamily. So adding impls results in e.g.

fn custom_clauses(&self) -> Vec<ProgramClause<chalk_solve::family::TypeFamily>> {}

because the TF parameter gets first substituted to TypeFamily, and that then resolves to chalk_solve::family::TypeFamily (because qualifying paths resolves everything from the definition of the trait) and hence gets qualified

Florian Diebold (Jan 10 2020 at 15:24, on Zulip):

basically, the qualifying-paths step doesn't know that the substituted things are already from the perspective of the module we're inserting into

matklad (Jan 10 2020 at 15:24, on Zulip):

Ah, so this is a hygiene issue of sorts?

Florian Diebold (Jan 10 2020 at 15:25, on Zulip):

kind of, yeah

matklad (Jan 10 2020 at 15:26, on Zulip):

Hm, I still have a feeling that we should do substitution first, but maybe we shouldn't subst syntactically?

matklad (Jan 10 2020 at 15:27, on Zulip):

Like, if we were rendering hir to syntax, we would first substitute in the hir, and then render a substituted thing

Florian Diebold (Jan 10 2020 at 15:27, on Zulip):

what do you mean?

Florian Diebold (Jan 10 2020 at 15:27, on Zulip):

ok yeah, but then we'd have to be able to actually render whole method definitions from hir

matklad (Jan 10 2020 at 15:28, on Zulip):

So perhaps substitute_type_params should not return and N, but something like FxHashMap<ast::Type, ast::Type>?

matklad (Jan 10 2020 at 15:28, on Zulip):

Than, the qualify thing would skip they keys of that map?

Florian Diebold (Jan 10 2020 at 15:29, on Zulip):

well, it would have to do the replacements

Florian Diebold (Jan 10 2020 at 15:29, on Zulip):

because after it's done, the ast::Type keys of that map might not actually exist in the new syntax tree anymore

matklad (Jan 10 2020 at 15:30, on Zulip):

But I think qual can also return just a Map<SyntaxNode, SyntaxNode>? And the separate layer would then \cup those two maps?

Florian Diebold (Jan 10 2020 at 15:31, on Zulip):

kind of, but that's where the problem comes in that the type args are a child node of the path

matklad (Jan 10 2020 at 15:31, on Zulip):

FUUUUUUUUUUUUUUUUUUUUUUUUUUUUUUUUUUUUUU

Florian Diebold (Jan 10 2020 at 15:31, on Zulip):

yeah

matklad (Jan 10 2020 at 15:33, on Zulip):

Hm....

It seems overall that the core problem is still that in-place syntax modification just erases essential info

matklad (Jan 10 2020 at 15:34, on Zulip):

So, some sort of a more diff-oriented approach seems more approriate..

matklad (Jan 10 2020 at 15:34, on Zulip):

Perhapse we need

type AstSubst = Box<Fn(&SyntaxNode) -> Option<SyntaxNode>>;

and make qual accept that?

matklad (Jan 10 2020 at 15:35, on Zulip):

Like, the rewsult of qual is actually a similar AstSubst, and looks like one can compose thouse things

matklad (Jan 10 2020 at 15:35, on Zulip):

It just has to be the right solution, because then it will be math :)

Florian Diebold (Jan 10 2020 at 15:36, on Zulip):

hmmm

matklad (Jan 10 2020 at 15:46, on Zulip):

It's also intresing that IntelliJ just adds imoprts, instead of qualifing parhs

matklad (Jan 10 2020 at 15:46, on Zulip):

But it does handle both imports and subts correctly (and that was implemented after I've left :D )

Florian Diebold (Jan 10 2020 at 15:47, on Zulip):

would make things easier, but I think in edge cases we would have to qualify paths anyway to avoid collisions, and I personally like giving the choice of importing things afterwards...

matklad (Jan 10 2020 at 15:48, on Zulip):

This is actually what IntelliJ does: https://github.com/intellij-rust/intellij-rust/blob/d23e13f8a7a9424795e9072f9806e2cbbb997989/src/main/kotlin/org/rust/ide/refactoring/implementMembers/impl.kt#L109

matklad (Jan 10 2020 at 15:51, on Zulip):

Hm, if I am reading this correctly, what IntelliJ does is that it renders types

matklad (Jan 10 2020 at 15:52, on Zulip):

https://github.com/intellij-rust/intellij-rust/blob/55584ccf5159c0369a690f99aa15a309a681049b/src/main/kotlin/org/rust/lang/core/psi/RsPsiFactory.kt#L501-L502

matklad (Jan 10 2020 at 15:52, on Zulip):

RsTypeReference is AST, but type, I think, is Ty

matklad (Jan 10 2020 at 15:53, on Zulip):

And the rendering is here: https://github.com/intellij-rust/intellij-rust/blob/a92d97755f5458a2ea8ceac10407acaf15595cb9/src/main/kotlin/org/rust/ide/presentation/TypeRendering.kt#L39

matklad (Jan 10 2020 at 15:55, on Zulip):

This actually seems like a nice middle ground: we render types, but copy-paste all the rest

Florian Diebold (Jan 10 2020 at 16:18, on Zulip):

Hm... I've got the AstSubst approach working now :sweat_smile:

Florian Diebold (Jan 10 2020 at 16:19, on Zulip):

I think it's actually quite nice, the question would be whether the additional complexity pays off

matklad (Jan 10 2020 at 16:21, on Zulip):

We can always rewrite later :D

Florian Diebold (Jan 10 2020 at 17:53, on Zulip):

see https://github.com/rust-analyzer/rust-analyzer/pull/2727/commits/0d16dbb340aae3ffccdc80132b61e1c8ced89de4 for the result

Last update: Jan 21 2020 at 09:00UTC