[ty] Add "Find references" support for pytest fixtures - #28075
Conversation
…8071) <!-- Thank you for contributing to Ruff/ty! To help us out with reviewing, please consider the following: - Does this pull request include a summary of the change? (See below.) - Does this pull request include a descriptive title? (Please prefix with `[ty]` for ty pull requests.) - Does this pull request include references to any relevant issues? - Does this PR follow our AI policy (https://github.com/astral-sh/.github/blob/main/AI_POLICY.md)? --> ## Summary In anticipation of ["find references" for pytest fixtures](#28075), which introduces many more speculative attempts at resolving pytest fixtures, this PR helps preserve efficiency by short-circuiting a few Salsa queries: - Return from `fixture_declaration` before calling `function_known_decorators` when a function has no decorators. - Return from `fixture_candidates_from_definition` before calling `exists_at_runtime` when the definition kind cannot expose a fixture. - Return from `directly_parametrized` before calling `function_known_decorators` when the test function has no decorators (while still checking parametrization from an enclosing class). These are all behavior-preserving early exits. ## Test Plan This is a refactor that relies on existing test coverage. <!-- How was it tested? -->
595146d to
7791fb7
Compare
…28073) <!-- Thank you for contributing to Ruff/ty! To help us out with reviewing, please consider the following: - Does this pull request include a summary of the change? (See below.) - Does this pull request include a descriptive title? (Please prefix with `[ty]` for ty pull requests.) - Does this pull request include references to any relevant issues? - Does this PR follow our AI policy (https://github.com/astral-sh/.github/blob/main/AI_POLICY.md)? --> ## Summary In anticipation of ["find references" for pytest fixtures](#28075), which enumerates possible fixture requests across every parameter of a function, this PR factors the shared, function-level work in `fixture_request_for_parameter` into `FixtureRequestContext`. This lets the follow-up construct one context per function and reuse it for each candidate parameter, rather than repeatedly calling: - `fixture_declaration`, `is_collected_test`, and `is_unittest_test_case` to determine whether the function can request fixtures. - `infer_definition_types` to retrieve the function type. - `mock_patch_count` to determine how many parameters are supplied by `unittest.mock.patch`. The existing `fixture_request_for_parameter` entry point now constructs a context and delegates to it, preserving behavior for existing callers. This is a behavior-preserving refactor. ## Test Plan This is a refactor that relies on existing test coverage (although I have backfilled a new test for some pre-existing behaviour). <!-- How was it tested? -->
7791fb7 to
4e6a752
Compare
4e6a752 to
8d91d99
Compare
8d91d99 to
d9bfeb3
Compare
d9bfeb3 to
784d39b
Compare
Typing conformance resultsNo changes detected ✅Current numbersThe percentage of diagnostics emitted that were expected errors held steady at 97.79%. The percentage of expected errors that received a diagnostic held steady at 94.33%. The number of fully passing files held steady at 112/136. |
Memory usage reportMemory usage unchanged ✅ |
|
| let minimum_job_len = minimum_parallel_job_len(files.len(), MAX_MIN_FILES_PER_PARALLEL_JOB); | ||
| let references = files | ||
| .into_par_iter() | ||
| .with_min_len(minimum_job_len) | ||
| .map_with_db(db, |db, file| { | ||
| pytest_fixture_references_for_file(db, file, &fixture_identities, mode) | ||
| }) | ||
| .flat_map_iter(|references| references) | ||
| .collect::<Vec<_>>(); |
There was a problem hiding this comment.
It seems unfortunate to iterate over all files here, only to then iterate over all files again to find ordinary references. On large projects, this requires repeating a lot of work. It can also trash Salsa caches because most eviction algorithms aren't scan-resistent.
- Clone the db for each batch: The cloning itself is cheap, but droping a clone isn't because it requires some coordination work.
- Read the
source_textof each file. This is where we start trashing our cache. This isn't the case today, but we could have a compressed source text and a recent-used uncompressed cache. Iterating twice will kill any form of recency - Run a memchr search across the source
- Run a local reference finder
Can you say more why you decided for this split?
| assert_snapshot!(test.references(), @" | ||
| info[references]: Found 5 references | ||
| --> src/fixtures.py:5:5 | ||
| | | ||
| 5 | def resource(): ... | ||
| | -------- | ||
| | | ||
| ::: src/test_example.py:2:22 | ||
| | | ||
| 2 | from fixtures import resource as alias | ||
| | -------- ----- | ||
| 3 | | ||
| 4 | def test_use(alias): | ||
| | ----- | ||
| 5 | print(alias) | ||
| | ----- | ||
| "); |
There was a problem hiding this comment.
Sorry for being a bit late on picking up on this. I think the implementation you have here makes sense, given how the pytest go-to-definition is defined, but it's inconsistent with how find references and go-to-definition work outside pytest. Reviewing the PRs in isolation never fully consider the LSP experience.
If we use this example (https://play.ty.dev/85c8eeb6-b0b5-4f4a-8779-b1928bacda63) without pytest support and search all references of def resource, it only returns def resource and the from fixtures import resource as alias. That makes sense to me, because alias is a new and independent definition. Renaming resource shouldn't rename alias, and the opposite is also true, renaming alias shouldn't rename resource.
The same behavior applies to goto definition. Clicking on alias in print(alias) jumps to the import statement. This is where the name is defined. You can then jump to resource, which jumps to the def resource(...).
Now, I can see that pytest fixtures are a bit different, but only if we enforce that alias has to point to a proper fixture for go to definition to offer it as a target. And I wonder if this is something we should do in the first place. Let's say you import a name which isn't a fixture and you run your test. Pytest then fails because you forgot to mark the imported symbol as a fixture. It would then be rather annoying if goto definition doesn't work.
That's why I think we should actually simplify pytest go to definition by simply implementing Pytest's name lookup. For functions that look like pytest functions, use it's name lookup to find the name in the enclosing scope and jump to it regardless on whether it is a pytest fixture or not. Don't resolve through aliases, matching our normal go to behavior. Clicking on an imported symbol also implements pytest resolution in test files.
This then also allows find references to work more like normal find references. And rename should also fit more naturally.
This might be worth talking about in person.
Summary
This adds "Find References" support for pytest fixtures. It connects fixture declarations to imports and reexports that expose them, fixture requests in tests and fixture functions, and the requests' lexical uses. Searches can start from any of those locations and respect fixture visibility and shadowing across class hierarchies, modules, applicable
conftest.pyfiles, and fixtures registered by pytest's installed core plugins.For example, given:
Finding references on
implementationreports its ordinary Python alias together with requests for its public fixture name,resource, and the requests' uses:Closes astral-sh/ty#4115.
Approach
The fixture reference search proceeds as follows:
Fixture references can involve several identifier names for the same fixture, but walking a file once per name would be expensive. As a result, this change introduce a
LocalReferenceSearchesdata type which groups those searches such that each file's syntax tree is still traversed only once. ItsSinglevariant keeps ordinary one-name reference searches on the existing direct-comparison path without allocating a hash map, but fixture searches that need multiple names useMultiplefor fast lookup during the shared traversal.Test Plan
See included tests.