Postfix completions trigger on block expressions that are part of other expressions

[Veykril]

if true {}.$0 else {}

Triggering completions at $0 will show postfix completions that affect the if true {} part even though it is indivisible from the else {}. We should check this and not show any postfix completions in this position.

[dqkqd]

Hi, I have 2 solutions, but I'm not sure which one is good.

1. Just checking if dot_receiver is part of if-else statements, then early return from this.
   

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

   Lines 21 to 28 in 8011029

   	pub(crate) fn complete_postfix(
   	acc: &mut Completions,
   	ctx: &CompletionContext<'_>,
   	dot_access: &DotAccess,
   	) {
   	if !ctx.config.enable_postfix_completions {
   	return;
   	}

2. Create new attribute receiver_is_part_of_indivisible_expression and put it into DotAccessKind::Field here.
   

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

   Lines 599 to 614 in 8011029

   	ast::FieldExpr(field) => {
   	let receiver = find_opt_node_in_file(original_file, field.expr());
   	let receiver_is_ambiguous_float_literal = match &receiver {
   	Some(ast::Expr::Literal(l)) => matches! {
   	l.kind(),
   	ast::LiteralKind::FloatNumber { .. } if l.syntax().last_token().map_or(false, |it| it.text().ends_with('.'))
   	},
   	_ => false,
   	};
   	let kind = NameRefKind::DotAccess(DotAccess {
   	receiver_ty: receiver.as_ref().and_then(|it| sema.type_of_expr(it)),
   	kind: DotAccessKind::Field { receiver_is_ambiguous_float_literal },
   	receiver
   	});
   	return Some(make_res(kind));
   	},

I'm leaning toward the second approach, but I'm not sure if it's fine since it will change data structure.

[Veykril]

Neither really works I think, the first will fail due to parser recovery, that is once the . is typed in that odd position the parser will probably recover the syntax tree in a weird way such that the check does not work (might be wrong here though).

The second approach sounds like the wrong way to fix this, we don't really want to calculate any completions in that position, so the completion analysis result should probably not yield anything in this case.

Somewhat related I think #10410

So ye, I think the proper fix is to somehow recognize this "invalid" position in the completions analysis module.

[dqkqd]

Neither really works I think, the first will fail due to parser recovery, that is once the . is typed in that odd position the parser will probably recover the syntax tree in a weird way such that the check does not work (might be wrong here though).

Yes, here's the ast. Look like it's recovered as a FieldExpr with else is a error field name.

    syntax: SOURCE_FILE@0..38
      FN@0..37
        FN_KW@0..2 "fn"
        WHITESPACE@2..3 " "
        NAME@3..7
          IDENT@3..7 "test"
        PARAM_LIST@7..9
          L_PAREN@7..8 "("
          R_PAREN@8..9 ")"
        WHITESPACE@9..10 " "
        BLOCK_EXPR@10..37
          STMT_LIST@10..37
            L_CURLY@10..11 "{"
            WHITESPACE@11..16 "\n    "
            EXPR_STMT@16..27
              FIELD_EXPR@16..27
                IF_EXPR@16..26
                  IF_KW@16..18 "if"
                  WHITESPACE@18..19 " "
                  LITERAL@19..23
                    TRUE_KW@19..23 "true"
                  WHITESPACE@23..24 " "
                  BLOCK_EXPR@24..26
                    STMT_LIST@24..26
                      L_CURLY@24..25 "{"
                      R_CURLY@25..26 "}"
                DOT@26..27 "."
            WHITESPACE@27..28 " "
            ERROR@28..32
              ELSE_KW@28..32 "else"
            WHITESPACE@32..33 " "
            BLOCK_EXPR@33..35
              STMT_LIST@33..35
                L_CURLY@33..34 "{"
                R_CURLY@34..35 "}"
            WHITESPACE@35..36 "\n"
            R_CURLY@36..37 "}"
      WHITESPACE@37..38 "\n"
    ,
}

[dqkqd]

So I think It could be fixed by checking if previous child is IF_EXPR and next non trivia sibling is ELSE_KW wrapped in an ERROR node. If that true, we return None and don't process further.

I could think of 2 places to plug it in.

The first one is as you said, inside completion analysis module.

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

Lines 217 to 227 in 8011029

	if is_in_token_of_for_loop(syntax_element.clone()) {
	// for pat $0
	// there is nothing to complete here except `in` keyword
	// don't bother populating the context
	// FIXME: the completion calculations should end up good enough
	// such that this special case becomes unnecessary
	return None;
	}
	// Overwrite the path kind for derives
	if let Some((original_file, file_with_fake_ident, offset, origin_attr)) = derive_ctx {

The second one is inside CompletionContext constructor

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

Lines 570 to 583 in 8011029

	let original_token = original_file.syntax().token_at_offset(offset).left_biased()?;
	// try to skip completions on path with invalid colons
	// this approach works in normal path and inside token tree
	match original_token.kind() {
	T![:] => {
	// return if no prev token before colon
	let prev_token = original_token.prev_token()?;
	// only has a single colon
	if prev_token.kind() != T![:] {
	return None;
	}

How does that sound?

[Veykril]

I think the check should happen here actually,

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

Lines 599 to 614 in 8011029

	ast::FieldExpr(field) => {
	let receiver = find_opt_node_in_file(original_file, field.expr());
	let receiver_is_ambiguous_float_literal = match &receiver {
	Some(ast::Expr::Literal(l)) => matches! {
	l.kind(),
	ast::LiteralKind::FloatNumber { .. } if l.syntax().last_token().map_or(false, |it| it.text().ends_with('.'))
	},
	_ => false,
	};
	let kind = NameRefKind::DotAccess(DotAccess {
	receiver_ty: receiver.as_ref().and_then(|it| sema.type_of_expr(it)),
	kind: DotAccessKind::Field { receiver_is_ambiguous_float_literal },
	receiver
	});
	return Some(make_res(kind));
	},

(we should land in this place, as completion analysis inserts a fake identifier that will cause us to have a nameref after the .)

[dqkqd]

Thanks @Veykril, I will prepare a PR for this.