Complete only missing fields in pats

[SomeoneToIgnore]

A follow-up for #3694

Same name vs string issue persists here, now I'm able to obtain ast::Name, but I see no way to convert it into hir::Name or vice versa.

[flodiebold]

I think a method on Semantics that takes an ast::Name and returns a hir::Name might make sense. It would be able to do hygiene properly when that's necessary/implemented.

[flodiebold]

On the other hand, going to the hir::Pat for the whole pattern first might actually be the better approach here. (When there's no need to deal with exact text, going to HIR as early as possible should be preferred, I think.)

[Veetaha]

I had thoughts on making hir::Pat, but haven't quite approached it due to the lack of time.
I described my concerns in here. I think we should make it smth like:

struct ra_hir::Pat {
    id: Idx<ra_hir_def::Pat>,
    parent: ra_hir_def::DefWithBodyId,
}

[SomeoneToIgnore]

going to the hir::Pat for the whole pattern first might actually be the better approach here.

Hm, I see that ctx has sema: Semantics and it has a way to get the SourceAnalyzer which has a way to get a PatId for the ast::RecordPat given, same as it has the Body to derive the Pat based on that id.
All the methods and fields I need are private, but it looks like I can hack around things and get the Pat for the given ast:RecordPat.
Or is there a simpler way?

Next, I have to match the Pat contents and descend down to the Path::Bind to extract the name and add it to the vec of an existing name.

Did I get the idea right?
It looks doable, but what is the actual benefit compared to the ast approach?
If I got that right, the issue I've mentioned in the PR message was related to the fact that we do unnecessary allocations, yet I'm not sure that we avoid them by going this path.

[Veetaha]

@SomeoneToIgnore I may be wrong, but we shouldn't expose ra_hir_def::Pat to the wild ra_ide. This enum contains PatId inside of some of its variants and I remember matklad implyin' (not sure) that the ra_ide crate should not interface with anything id-based, so that this is hidden by the ra_hir OO layer (citation from him).

[SomeoneToIgnore]

They are not available in ra_ide currently, good point, thanks.

going to the hir::Pat for the whole pattern first might actually be the better approach here.

So apparently I've understood it wrong and need clarification :)

[flodiebold]

So apparently I've understood it wrong and need clarification :)

You didn't misunderstand, I just imagined APIs that don't exist yet ;)

Next, I have to match the Pat contents and descend down to the Path::Bind to extract the name and add it to the vec of an existing name.

Not Pat::Bind, Pat::Record, which has a list of RecordFieldPats :)

It looks doable, but what is the actual benefit compared to the ast approach?

It should be the simplest and cleanest way to do this, if the APIs actually existed ;) For one thing, you wouldn't have to deal with field shorthand.

[matklad]

I think a method on Semantics that takes an ast::Name and returns a hir::Name might make sense. It would be able to do hygiene properly when that's necessary/implemented.

I think for this specifically, we should have something more high-level

impl Semantics {
    fn missing_fields(&self, src: RecordLiteral | CallExpr | TupleStructPat | RecordPat) -> Vec<StructField>
}

In general, I think we should strive for broad, but shallow API, a-la BindingContext.

[matklad]

Specific action items for this PR are probably:

• add initial implementation and tests using the current approach
• based on this experience, move the logic from here to and complete_record_literal to semantics (via a couple of functions which return a Vec of missing StructField).

[flodiebold]

I think for this specifically, we should have something more high-level

impl Semantics {
    fn missing_fields(&self, src: RecordLiteral | CallExpr | TupleStructPat | RecordPat) -> Vec<StructField>
}

In general, I think we should strive for broad, but shallow API, a-la BindingContext.

Hmm to me, 'missing fields' seems like an IDE-level concern and doesn't really fit into my understanding of Semantics 🤔 (It's not a 'diagnostic' either since we would want to consider all fields 'missing' in Foo { .. }.)

[matklad]

Yeah, it's true that it is primarily for IDEs (although I expect there will be some impl intersection with relevant diagnostics). But in my mind it's not really about IDE vs compiler. Like we only use Semantics from an IDE anyway, and the whole source_do_def machinery is IDE-specific.

Rather, it's about the level of abstraction. Stuff below semantics works with relative ids, stuff above works with OO-ish code-model, and Semantics is a facade. So, I wouldn't worry about Semantics turning into a kitchen sink: as semantics mehtods mostly return result directly and do not compos with each other, adding or removing Semantics method should be cheap. OTOH, if we expose more of the internals (like ids), than we'll get the API where you need to compose several methods to get the desired result, and in this situation changing a single method would be costly.

Yeah, I sound like I know what I am talking about, but I actually don't, it's just my gut feeling that it's better to expose sepecific rather than general APIs here.

[SomeoneToIgnore]

And I just wanted to submit a small useful PR 😀
Thank you for mapping the path, I'll try to implement it and see what comes out of it.

[matklad]

To clarify, doing just

add initial implementation and tests using the current approach

is totally fine, I can then look into refactoring things :)

[SomeoneToIgnore]

Just to clarify more: what is the current approach?
The ast one or the #3779 (comment) one?

[Veetaha]

That BindingContext thing requires quite some digging...

[matklad]

The ast one, or, really, whichever simpler that works in the common case :)

[SomeoneToIgnore]

I've fiddled with the code and eventually decided to unite both files into one: whenever I extract the common logic, the rest of it in both files becomes so small and similar, that it feels like the right way.
Wdyt?

Few nonrelated findings along the way:

• we don't create a runner for a top-level test mod now, so we cannot run all tests at once, we might want to fix that

• cannot autoimport ra_syntax::ast::NameOwner trait, I'll investigate this further

[matklad]

Yeah, I do think merging the two makes sense.

we don't create a runner for a top-level test mod now, so we cannot run all tests at once, we might want to fix that

Yes please, this have been bugging me for a long time

bors r+

[SomeoneToIgnore]

cannot autoimport ra_syntax::ast::NameOwner trait, I'll investigate this further

That is explained by the fact that all closure parameters are {unknown} currently:

.filter_map(|fild_pat| fild_pat.name())
.chain(pat_list.bind_pats().filter_map(|bind_pat| bind_pat.name()))

hence the trait checks in iterate_method_candidates do not pass.

[bors]

Build succeeded

• Rust (macos-latest)
• Rust (ubuntu-latest)
• Rust (windows-latest)
• TypeScript

[flodiebold]

That is explained by the fact that all closure parameters are {unknown} currently

Not all of them, just many 😬