fix Goto-definition on import shouldn't use a transaction #1331

[asukaminato0721]

Summary

Fixes #1331

import module names now has a server-side fast path that resolves the import directly from the current file contents and import lookup state, before creating or validating a transaction.

That means requests like import bar or from .bar import value still return the target module file even while a recheck is in flight.

Test Plan

add test

[Copilot]

Pull request overview

Adds a server-side fast path for go-to-definition on import module names so navigation can still succeed even when a recheck/transaction is blocked (fixes #1331).

Changes:

• Adds a GotoDefinition pre-transaction fast path that parses the current file contents and resolves imported modules via the loader without creating/validating a transaction.
• Factors import-module target resolution into Transaction::target_imported_module_name and exposes identifier_from_covering_nodes for reuse.
• Adds an LSP interaction test covering go-to-definition on an import while a recheck is blocked.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.

File	Description
pyrefly/lib/lsp/non_wasm/server.rs	Adds fast-path handling for import-module goto-definition before transaction creation.
pyrefly/lib/state/lsp.rs	Exposes identifier extraction and factors module-name targeting logic into a reusable helper.
pyrefly/lib/test/lsp/lsp_interaction/definition.rs	Adds a regression test for goto-definition on imports during blocked recheck.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The fast-path short-circuits the whole GotoDefinition request whenever it detects an imported-module identifier, even if the fast-path fails (it converts the error into an empty response and returns early). That can regress existing behavior by preventing the normal transaction-backed goto-definition from running in cases where the loader/path conversion fails but the full analysis could still resolve the definition. Consider only short-circuiting on a successful fast-path resolution, and falling back to the existing transaction path on fast-path errors.

Suggested change

	let response = match response {
	Ok(response) => response,
	Err(reason) => {
	telemetry_event.set_empty_response_reason(reason);
	None
	}
	};
	self.send_response(new_response(x.id, Ok(response)));
	return Ok(ProcessEvent::Continue);
	if let Ok(response) = response {
	self.send_response(new_response(x.id, Ok(response)));
	return Ok(ProcessEvent::Continue);
	}

[Copilot]

LoaderFindCache::new is constructed on every goto-definition fast-path invocation, which discards the cache and can make repeated import go-to-def requests unnecessarily expensive. Consider reusing a cached LoaderFindCache keyed by config (or exposing a shared loader cache from state) so import resolution benefits from memoization across requests.

[Copilot]

This edit relies on replace("foo = 3", "foo = 4") to change the file contents, but it doesn't assert that the replacement actually happened. If the fixture changes (or the string appears multiple times), this can become a no-op and the test may stop exercising the blocked-recheck path. Consider asserting the substring exists exactly once (or constructing the edited contents more explicitly) before calling edit_file.

[Copilot]

The PR description mentions fast-path support for both import bar and from .bar import value while a recheck is in flight, but this test only covers the plain import case. Adding a companion assertion for a from ... import ... (including a relative-dots case) would help ensure the new imported-module parsing and dots/position logic stays correct.

[github-actions]

According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅