[ty] provide import completion when in from <name> <name> statement

[MatthewMckee4]

Summary

Resolves astral-sh/ty#1494

Test Plan

Add a test showing if we are in from <name> <name> we provide the keyword completion "import"

[github-actions]

Diagnostic diff on typing conformance tests

No changes detected when running ty on typing conformance tests ✅

[github-actions]

mypy_primer results

No ecosystem changes detected ✅
No memory usage changes detected ✅

[MichaReiser]

Only showing import in a from <NAME> <NAME2> position does make sense to me. I was surprised to see that Pylance shows all completions, but gives import a higher ranking

[MatthewMckee4]

For some reason i dont get completions in playground locally for import when i have just a

This could be a playground thing though?

[MichaReiser]

I just checked out this PR and built the playground and this works as expected when you have from types i but it doesn't work when you have from types aa.

This could either be because the playground applies its own filtering (not sure if there's a way for us to disable it, do you want to take a look) or because our own "text prefix matching"

[BurntSushi]

Yeah we may not be able to fully control whether import is shown for from module import aa<CURSOR>. LSP clients can and will do their own filtering AIUI. I'm not sure if there is a foolproof way to override that.

[AlexWaygood]

Nice, thank you!

[MatthewMckee4]

Thank you, I will revisit this in a few days, when I am back home.

[AlexWaygood]

Thank you!

[MichaReiser]

The search here seems to be too permissive to me. What's preventing it from going across logical lines?

Can you add some tests with multiple statements? I suspect that we now incorrectly match on import completions if there's any import statement before <CURSOR>.

[MatthewMckee4]

Good point, will add that

[MichaReiser]

I still think this is incorrect as it cross logical line boundaries. E.g. what if

from module impo

a<CURSOR>

We would want to show completions for a and not suggest import

[MatthewMckee4]

I am maybe misunderstanding you. we do still provide completions for a here.

[MichaReiser]

I haven't checked out the PR. What I'm concerned with the current approach is that tokens is anything before <CURSOR> (unless this isn't the case?). That means, it's possible for tokens.iter().rposition() to find the from even though it belongs to an entirely different statement.

[BurntSushi]

Can you make sure comments are wrapped to 99 columns (inclusive) please? There are some other comments below that need to be wrapped too.

[BurntSushi]

I think this routine should be modeled on this existing function:

ruff/crates/ty_ide/src/completion.rs

Lines 667 to 756 in f44598d

	/// Looks for the start of a `from module import <CURSOR>` statement.
	///
	/// If found, one arbitrary token forming `module` is returned.
	fn import_from_tokens(tokens: &[Token]) -> Option<&Token> {
	use TokenKind as TK;
	/// The number of tokens we're willing to consume backwards from
	/// the cursor's position until we give up looking for a `from
	/// module import <CURSOR>` pattern. The state machine below has
	/// lots of opportunities to bail way earlier than this, but if
	/// there's, e.g., a long list of name tokens for something that
	/// isn't an import, then we could end up doing a lot of wasted
	/// work here. Probably humans aren't often working with single
	/// import statements over 1,000 tokens long.
	///
	/// The other thing to consider here is that, by the time we get to
	/// this point, ty has already done some work proportional to the
	/// length of `tokens` anyway. The unit of work we do below is very
	/// small.
	const LIMIT: usize = 1_000;
	/// A state used to "parse" the tokens preceding the user's cursor,
	/// in reverse, to detect a "from import" statement.
	enum S {
	Start,
	Names,
	Module,
	}
	let mut state = S::Start;
	let mut module_token: Option<&Token> = None;
	// Move backward through the tokens until we get to
	// the `from` token.
	for token in tokens.iter().rev().take(LIMIT) {
	state = match (state, token.kind()) {
	// It's okay to pop off a newline token here initially,
	// since it may occur when the name being imported is
	// empty.
	(S::Start, TK::Newline) => S::Names,
	// Munch through tokens that can make up an alias.
	// N.B. We could also consider taking any token here
	// *except* some limited set of tokens (like `Newline`).
	// That might work well if it turns out that listing
	// all possible allowable tokens is too brittle.
	(
	S::Start | S::Names,
	TK::Name
	| TK::Comma
	| TK::As
	| TK::Case
	| TK::Match
	| TK::Type
	| TK::Star
	| TK::Lpar
	| TK::Rpar
	| TK::NonLogicalNewline
	// It's not totally clear the conditions under
	// which this occurs (I haven't read our tokenizer),
	// but it appears in code like this, where this is
	// the entire file contents:
	//
	// from sys import (
	// abiflags,
	// <CURSOR>
	//
	// It seems harmless to just allow this "unknown"
	// token here to make the above work.
	| TK::Unknown,
	) => S::Names,
	(S::Start | S::Names, TK::Import) => S::Module,
	// Munch through tokens that can make up a module.
	(
	S::Module,
	TK::Name | TK::Dot | TK::Ellipsis | TK::Case | TK::Match | TK::Type | TK::Unknown,
	) => {
	// It's okay if there are multiple module
	// tokens here. Just taking the last one
	// (which is the one appearing first in
	// the source code) is fine. We only need
	// this to find the corresponding AST node,
	// so any of the tokens should work fine.
	module_token = Some(token);
	S::Module
	}
	(S::Module, TK::From) => return module_token,
	_ => return None,
	};
	}
	None
	}

Maybe there is a way to factor out the common logic, but even if there isn't, just copying the approach seems like the right move here. I agree with @MichaReiser above. And notice that the function above is careful to not do too much look-behind on the token sequence. It also handles things like ellipsis, non-logical newlines and so on.

[BurntSushi]

For something really short, I'm fine with the \n here. But this starts to make very long lines. I'd rather see this written out using actual new lines:

        let builder = completion_test_builder(
            "from collections.abc import Sequence
             from typing i<CURSOR>"
        );

The test code should automatically handle dedenting.

[BurntSushi]

@MatthewMckee4 I'm going to take over this PR since we'd like to get this fixed. And I'm planning to work on more stuff around keyword completions this week, which will really want to build on top of this PR. Thank you. :-)

[MatthewMckee4]

Cool, apologies for the delays, I've been a little busy. Thank you for the feedback too!

[AlexWaygood]

Cool, apologies for the delays, I've been a little busy.

never feel like you need to apologise for unpaid volunteer work :-) we really appreciate the contribution!!