fix(ide-completion): fix handling of for in impl T for A in function body

[rami3l]

Closes #17787.

[rami3l]

The couple of failed tests I checked seem fine, they need a baseline update.

@lnicola Thanks! The fix per se looks good to me, but I wonder if this is in line with other stuff, such as:

rust-analyzer/crates/ide-completion/src/tests/special.rs

Lines 1266 to 1299 in 13ac53e

	#[test]
	fn completes_flyimport_with_doc_alias_in_another_mod() {
	check(
	r#"
	mod foo {
	#[doc(alias = "Qux")]
	pub struct Bar();
	}
	fn here_we_go() {
	let foo = Bar$0
	}
	"#,
	expect![[r#"
	fn here_we_go() fn()
	md foo
	st Bar (alias Qux) (use foo::Bar) Bar
	bt u32 u32
	kw crate::
	kw false
	kw for
	kw if
	kw if let
	kw loop
	kw match
	kw return
	kw self::
	kw true
	kw unsafe
	kw while
	kw while let
	"#]],
	);
	}

Do we really need all those suggestions? This PR eliminates for which seems reasonable to me, but what about other stuff like loop and match? Maybe in_block_expr is not the correct predicate here?

[rami3l]

@lnicola The key issue here seems to be that impl T for A is local to the function, otherwise I cannot reproduce #17787. I'll try finding a smarter way of doing things, and adding a test case if possible.

[Veykril]

We incorrectly land in a PathKind::Expr here in completion analysis likely due to parser recovery

[Veykril]

Given

fn foo() {
    struct X;
    impl X fo { }
}

we get a

FN@48489..48557
  FN_KW@48489..48491 "fn"
  WHITESPACE@48491..48492 " "
  NAME@48492..48495
    IDENT@48492..48495 "foo"
  PARAM_LIST@48495..48497
    L_PAREN@48495..48496 "("
    R_PAREN@48496..48497 ")"
  WHITESPACE@48497..48498 " "
  BLOCK_EXPR@48498..48557
    STMT_LIST@48498..48557
      L_CURLY@48498..48499 "{"
      WHITESPACE@48499..48512 "\n            "
      STRUCT@48512..48521
        STRUCT_KW@48512..48518 "struct"
        WHITESPACE@48518..48519 " "
        NAME@48519..48520
          IDENT@48519..48520 "X"
        SEMICOLON@48520..48521 ";"
      WHITESPACE@48521..48534 "\n            "
      IMPL@48534..48540
        IMPL_KW@48534..48538 "impl"
        WHITESPACE@48538..48539 " "
        PATH_TYPE@48539..48540
          PATH@48539..48540
            PATH_SEGMENT@48539..48540
              NAME_REF@48539..48540
                IDENT@48539..48540 "X"
      WHITESPACE@48540..48541 " "
      RECORD_EXPR@48541..48547
        PATH@48541..48543
          PATH_SEGMENT@48541..48543
            NAME_REF@48541..48543
              IDENT@48541..48543 "fo"
        WHITESPACE@48543..48544 " "
        RECORD_EXPR_FIELD_LIST@48544..48547
          L_CURLY@48544..48545 "{"
          WHITESPACE@48545..48546 " "
          R_CURLY@48546..48547 "}"
      WHITESPACE@48547..48556 "\n        "
      R_CURLY@48556..48557 "}"

(there is a show syntax tree command in vscode to get this view)
as youcan see, we parse the fo outside of the impl but within the block expression of the function, so we parse it as an expression. From a recovery perspective this does make sense, so we should probably fix that up in the completion analysis? Not sure,
In the non local item position we g et

  IMPL@71872..71878
    IMPL_KW@71872..71876 "impl"
    WHITESPACE@71876..71877 " "
    PATH_TYPE@71877..71878
      PATH@71877..71878
        PATH_SEGMENT@71877..71878
          NAME_REF@71877..71878
            IDENT@71877..71878 "X"
  WHITESPACE@71878..71879 " "
  MACRO_CALL@71879..71885
    PATH@71879..71881
      PATH_SEGMENT@71879..71881
        NAME_REF@71879..71881
          IDENT@71879..71881 "fo"
    WHITESPACE@71881..71882 " "
    TOKEN_TREE@71882..71885
      L_CURLY@71882..71883 "{"
      WHITESPACE@71883..71884 " "
      R_CURLY@71884..71885 "}"

so there its a macro call. We should already be handling that case (I get a for completion there), so might be good to check how we handle that in completion analysis

[rami3l]

@Veykril Thanks for your pointers! I think I've found a way to be less disruptive, let's see how the CI goes.

Update: Checking impl_.is_some() is still not right. A counter-example would be:

fn foo() {
    struct X;
    impl X { fn bar() { $0 } }
}

[Veykril]

The proper fix will require changes in crates\ide-completion\src\context\analysis.rs as we land in the wrong branch somewhere (due to not accounting for parser recovery)

[Veykril]

rust-analyzer/crates/ide-completion/src/completions/keyword.rs

Lines 7 to 31 in 13ac53e

	pub(crate) fn complete_for_and_where(
	acc: &mut Completions,
	ctx: &CompletionContext<'_>,
	keyword_item: &ast::Item,
	) {
	let mut add_keyword = |kw, snippet| acc.add_keyword_snippet(ctx, kw, snippet);
	match keyword_item {
	Item::Impl(it) => {
	if it.for_token().is_none() && it.trait_().is_none() && it.self_ty().is_some() {
	add_keyword("for", "for $0");
	}
	add_keyword("where", "where $0");
	}
	Item::Enum(_)
	| Item::Fn(_)
	| Item::Struct(_)
	| Item::Trait(_)
	| Item::TypeAlias(_)
	| Item::Union(_) => {
	add_keyword("where", "where $0");
	}
	_ => (),
	}
	}

is where we should be landing in this snippet instead of the expression completion

[rami3l]

rust-analyzer/crates/ide-completion/src/completions/keyword.rs

Lines 7 to 31 in 13ac53e

	pub(crate) fn complete_for_and_where(
	acc: &mut Completions,
	ctx: &CompletionContext<'_>,
	keyword_item: &ast::Item,
	) {
	let mut add_keyword = |kw, snippet| acc.add_keyword_snippet(ctx, kw, snippet);
	match keyword_item {
	Item::Impl(it) => {
	if it.for_token().is_none() && it.trait_().is_none() && it.self_ty().is_some() {
	add_keyword("for", "for $0");
	}
	add_keyword("where", "where $0");
	}
	Item::Enum(_)
	| Item::Fn(_)
	| Item::Struct(_)
	| Item::Trait(_)
	| Item::TypeAlias(_)
	| Item::Union(_) => {
	add_keyword("where", "where $0");
	}
	_ => (),
	}
	}

is where we should be landing in this snippet instead of the expression completion

@Veykril I'm aware of this, it's just that the only possible context of this call is:

rust-analyzer/crates/ide-completion/src/completions.rs

Lines 775 to 777 in 13ac53e

	NameRefKind::Keyword(item) => {
	keyword::complete_for_and_where(acc, ctx, item);
	}

Clearly it's not the same as in a fn, where we mostly expect statements (including items, macro calls and expression statements).

In this case, given how the function is written, I guess PathKind::Expr stands for exactly that: something expression-like. Most of the time you'd want an expression.

Let me see what I can do to improve

rust-analyzer/crates/ide-completion/src/context/analysis.rs

Line 1066 in 13ac53e

PathKind::Expr {

...

[Veykril]

If the preceding token to the cursor is an impl Foo then the only things thats may follow that Foo is a for token, a where token or a curly brace, not an expression. So the completion analysis should compute a NameRefKind::Keyword here, not a NameRefKind::Path

[rami3l]

If the preceding token to the cursor is an impl Foo then the only things thats may follow that Foo is a for token, a where token or a curly brace, not an expression. So the completion analysis should compute a NameRefKind::Keyword here, not a NameRefKind::Path

I see. I believe it works outside of functions because you special-cased it:

rust-analyzer/crates/ide-completion/src/context/analysis.rs

Lines 1157 to 1161 in 13ac53e

	ast::MacroCall(it) => {
	// A macro call in this position is usually a result of parsing recovery, so check that
	if let Some(kind) = inbetween_body_and_decl_check(it.syntax().clone()) {
	return Some(make_res(NameRefKind::Keyword(kind)));
	}

... maybe I can apply a similar hack to this case.

[Veykril]

Yes thats the special case handling it outside of bodies, would be neat if that can be re-used though its probably a bit more tricky

[rami3l]

Yes thats the special case handling it outside of bodies, would be neat if that can be re-used though its probably a bit more tricky

@Veykril So by adding the same hack to records in 3d13c7f, I was able to address the original issue. However, I'm still more interested in the fn foo() { impl X for $0 } case.

[Veykril]

What exaclty is with fn foo() { impl X for $0 }?

[rami3l]

What exaclty is with fn foo() { impl X for $0 }?

The inbetween_body_and_decl_check() hack on records has worked with fn foo() { impl X $0 {} } because $0 {} was resolved to a record, according to your first example in #18005 (comment). However I'm not sure how to handle the case without {}.

fn foo() { impl X fo } gives something like:

 FN@9570..9592
    FN_KW@9570..9572 "fn"
    WHITESPACE@9572..9573 " "
    NAME@9573..9576
      IDENT@9573..9576 "foo"
    PARAM_LIST@9576..9578
      L_PAREN@9576..9577 "("
      R_PAREN@9577..9578 ")"
    WHITESPACE@9578..9579 " "
    BLOCK_EXPR@9579..9592
      STMT_LIST@9579..9592
        L_CURLY@9579..9580 "{"
        WHITESPACE@9580..9581 " "
        IMPL@9581..9587
          IMPL_KW@9581..9585 "impl"
          WHITESPACE@9585..9586 " "
          PATH_TYPE@9586..9587
            PATH@9586..9587
              PATH_SEGMENT@9586..9587
                NAME_REF@9586..9587
                  IDENT@9586..9587 "X"
        WHITESPACE@9587..9588 " "
        PATH_EXPR@9588..9590
          PATH@9588..9590
            PATH_SEGMENT@9588..9590
              NAME_REF@9588..9590
                IDENT@9588..9590 "fo"
        WHITESPACE@9590..9591 " "
        R_CURLY@9591..9592 "}"
  WHITESPACE@9592..9593 "\n"

[Veykril]

Isn't that this case? https://github.com/rust-lang/rust-analyzer/pull/18005/files#diff-9cef2bb04b7b1bc7d74e0d5822c991876dec30dce47004692a99369f18651442R166-R180
I'm sorry I'm confused, fn foo() { impl X for $0 {} } already works fine today for me, your change ought to make fn foo() { impl X fo$0 {} } / fn foo() { impl X $0 {} } work from what I can tell

[rami3l]

I'm sorry I'm confused, fn foo() { impl X for $0 {} } already works fine today for me, your change ought to make fn foo() { impl X fo$0 {} } / fn foo() { impl X $0 {} } work from what I can tell

@Veykril No worries. I believe now I've fixed both of them.

[Veykril]

Thanks!
@bors r+

[bors]

📌 Commit 0b28126 has been approved by Veykril

It is now in the queue for this repository.

[bors]

⌛ Testing commit 0b28126 with merge 2890b10...

[bors]

☀️ Test successful - checks-actions
Approved by: Veykril
Pushing 2890b10 to master...