When hovering on an attribute name like 'value' in 'instance.value',
the minimal covering node is the Identifier, but we need the Attribute
expression to get the type. Update find_covering_node to track both
the minimal node and minimal expression, preferring the expression.
This fixes hover on attribute accesses. All hover tests now pass.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <noreply@anthropic.com>
When hovering, if we find a statement node (like StmtExpr), extract the
expression from within it. This allows hover assertions to work on
standalone expressions like variable references.
Also add a simple working mdtest to demonstrate hover assertions.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <noreply@anthropic.com>
Update SortedCheckOutputs to store references to CheckOutput instead
of owned values, matching the design of the previous SortedDiagnostics
implementation. This avoids unnecessary cloning and makes the API more
consistent.
Changes:
- SortedCheckOutputs now stores Vec<&CheckOutput>
- new() takes IntoIterator<Item = &CheckOutput>
- LineCheckOutputs.outputs is now &[&CheckOutput]
- Implement Unmatched and UnmatchedWithColumn for &CheckOutput
- Update match_line to take &[&CheckOutput]
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <noreply@anthropic.com>
- Elide unnecessary explicit lifetimes in find_covering_node
- Add backticks around CheckOutputs in doc comment
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <noreply@anthropic.com>
Update match_line signature to accept &[CheckOutput] instead of
&LineCheckOutputs for consistency with the previous API that took
&[Diagnostic].
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <noreply@anthropic.com>
Store the hover assertion column as OneIndexed since that's the type
returned by line_column() and required by SourceLocation. This
eliminates unnecessary conversions between zero-based and one-based
indexing.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <noreply@anthropic.com>
Instead of manually calculating character offsets, use line_column()
which does all the work for us:
1. Find the byte offset of the arrow in the comment text
2. Add that to the comment's TextRange to get the arrow's absolute position
3. Call line_column() on that position to get the character column
This is much simpler and lets line_column handle all the UTF-8/UTF-32
conversion complexity.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <noreply@anthropic.com>
The column field was being treated inconsistently - calculated as a
character offset but used as a byte offset. This would break on any
line with multi-byte UTF-8 characters before the hover position.
Fixes:
1. Use chars().position() instead of find() to get character offset
2. Use LineIndex::offset() with PositionEncoding::Utf32 to properly
convert character offset to byte offset (TextSize)
3. Document that column is a UTF-32 character offset
This ensures hover assertions work correctly with Unicode text.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <noreply@anthropic.com>
Convert the Hover variant from tuple-style to named fields for better
clarity and self-documentation.
Before: Hover(&'a str, &'a str, TextRange)
After: Hover { expected_type, full_comment, range }
This makes the code more readable and easier to maintain.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <noreply@anthropic.com>
Move the column calculation from generate_hover_outputs into
HoverAssertion::from_str() by passing LineIndex and SourceText to
the parse() method.
This simplifies the code and centralizes the column calculation logic
in one place during parsing, rather than spreading it across multiple
locations. The HoverAssertion now directly stores the final column
position in the line, ready to use.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <noreply@anthropic.com>
The previous implementation calculated the down arrow position within
the comment text, but then incorrectly used that as an offset into the
target line. This only worked if the comment started at column 0.
Now we properly calculate the column position by:
1. Finding the arrow offset within the comment text
2. Getting the comment's column position in its line
3. Adding these together to get the arrow's column in the line
4. Using that column to index into the target line
This fixes hover assertions when comments are indented or don't start
at the beginning of the line.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <noreply@anthropic.com>
Change the column field in HoverAssertion from OneIndexed to usize,
storing it as a zero-based index. This matches how it's used and
eliminates unnecessary conversions between zero-based and one-based
indexing.
The arrow position from find() is already zero-based, and TextSize
uses zero-based offsets, so this is more natural.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <noreply@anthropic.com>
Import InlineFileAssertions, ParsedAssertion, and UnparsedAssertion
at the module level instead of using fully qualified crate::assertion
names throughout the code.
This makes the code cleaner and more idiomatic.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <noreply@anthropic.com>
Move the column calculation from generate_hover_outputs to the
HoverAssertion parsing logic. This makes better use of the existing
column field in HoverAssertion.
Changes:
- UnparsedAssertion::Hover now stores both the expected type and the
full comment text
- HoverAssertion::from_str() now takes both parameters and calculates
the column from the down arrow position in the full comment
- generate_hover_outputs() now reads the column from the parsed
assertion instead of recalculating it
This eliminates redundant calculations and makes the column field
actually useful.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <noreply@anthropic.com>
Implement UnmatchedWithColumn for CheckOutput and update the column()
method to work with CheckOutput instead of just Diagnostic.
This allows eliminating the match statement when formatting unmatched
outputs - we can now just call unmatched_with_column() on all outputs
uniformly.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <noreply@anthropic.com>
Move the HoverOutput type from check_output.rs to hover.rs where it
logically belongs. The check_output module should only contain the
CheckOutput enum and sorting infrastructure, while hover-specific
types belong in the hover module.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <noreply@anthropic.com>
Create a dedicated HoverOutput struct to hold hover result data,
replacing the inline fields in CheckOutput::Hover variant.
This allows implementing Unmatched and UnmatchedWithColumn traits
directly on HoverOutput, simplifying the CheckOutput implementations
to simple delegation.
Benefits:
- Better separation of concerns
- Cleaner trait implementations
- More consistent with Diagnostic handling
- Easier to extend HoverOutput in the future
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <noreply@anthropic.com>
SortedDiagnostics is now replaced by SortedCheckOutputs, which handles
both diagnostics and hover results. This refactoring:
- Renames diagnostic.rs to check_output.rs to better reflect its purpose
- Moves CheckOutput and SortedCheckOutputs definitions from matcher.rs
to check_output.rs where they belong
- Removes the now-unused SortedDiagnostics infrastructure
- Ports the test to use SortedCheckOutputs instead of SortedDiagnostics
- Updates all imports throughout the codebase
The check_output module now serves as the central location for sorting
and grouping all types of check outputs (diagnostics and hover results)
by line number.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <noreply@anthropic.com>
Replace nested if-let blocks with let-else + continue to reduce
indentation and improve readability.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <noreply@anthropic.com>
The previous implementation incorrectly assumed the target line was
always line_number + 1, which breaks when multiple assertion comments
are stacked on consecutive lines.
Now generate_hover_outputs accepts the parsed InlineFileAssertions,
which already correctly associates each assertion with its target line
number (accounting for stacked comments).
For the column position, we extract it directly from the down arrow
position in the UnparsedAssertion::Hover text, avoiding the need to
parse the assertion ourselves (parsing happens later in the matcher).
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <noreply@anthropic.com>
Replace the leave_node() approach with a simpler strategy: just compare
the range lengths and keep the smallest node that covers the offset.
This is more direct and easier to understand than tracking when we
leave nodes. The minimal covering node is simply the one with the
shortest range that contains the offset.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <noreply@anthropic.com>
The previous implementation had a bug: it would overwrite `found` for
every matching node in source order, which could incorrectly select a
sibling node instead of the minimal covering node.
Now use the same approach as ty_ide's covering_node:
- Use leave_node() to detect when we've finished traversing a subtree
- Set a `found` flag when leaving the minimal node to prevent further
updates
- This ensures we return the deepest (most specific) node that covers
the offset, not just the last one visited in source order
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <noreply@anthropic.com>
Replace the large match statement over all AnyNodeRef expression variants
with a simple call to as_expr_ref(). This helper method handles all
expression-related variants automatically, reducing the function from
~60 lines to just 6 lines.
Also removes statement-related variants (StmtFunctionDef, StmtClassDef,
StmtExpr) to focus only on expression nodes, which is the primary use
case for hover assertions.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <noreply@anthropic.com>
Move hover-related functions from lib.rs into a new hover.rs module
to improve code organization:
- find_covering_node() - locate AST nodes at specific offsets
- infer_type_at_position() - get inferred types using SemanticModel
- generate_hover_outputs() - scan for hover assertions and generate results
This keeps lib.rs focused on the main test execution flow.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <noreply@anthropic.com>
- Add find_covering_node() to locate AST nodes at specific positions
- Add infer_type_at_position() to get type information using ty_python_semantic
- Add generate_hover_outputs() to scan for hover assertions and generate results
- Integrate hover outputs into check flow in run_test()
- Implement hover matching logic in matcher.rs to compare types
- Avoid adding ty_ide dependency; instead use ty_python_semantic directly
The implementation extracts hover assertions from comments, computes the target
position from the down arrow location, infers the type at that position using
the AST and SemanticModel, and matches it against the expected type in the
assertion.
Hover assertions now work end-to-end in mdtest files!
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <noreply@anthropic.com>
- Create CheckOutput enum with Diagnostic and Hover variants
- Replace SortedDiagnostics with SortedCheckOutputs in matcher
- Update match_file to accept &[CheckOutput] instead of &[Diagnostic]
- Update matching logic to handle both diagnostics and hover results
- Implement Unmatched trait for CheckOutput
- Convert diagnostics to CheckOutput in lib.rs before matching
This infrastructure allows hover assertions to be matched against hover
results without polluting the DiagnosticId enum with test-specific IDs.
The hover matching logic will be implemented in the next commit.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <noreply@anthropic.com>
- Add Hover variant to UnparsedAssertion and ParsedAssertion enums
- Create HoverAssertion struct with column and expected_type fields
- Add HoverAssertionParseError enum for validation errors
- Update from_comment() to recognize '# hover:' and '# ↓ hover:' patterns
- Simplified design: down arrow must appear immediately before 'hover' keyword
- Add placeholder matching logic in matcher.rs (to be completed)
- Add PLAN.md to track implementation progress
The hover assertion syntax uses a down arrow to indicate column position:
# ↓ hover: expected_type
expression_to_hover
This will enable testing hover functionality in mdtest files, similar to
how ty_ide tests work with <CURSOR> markers.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <noreply@anthropic.com>
## Summary
Bump the latest supported Python version of ty to 3.14 and updates some
references from 3.13 to 3.14.
This also fixes a bug with `dataclasses.field` on 3.14 (which adds a new
keyword-only parameter to that function, breaking our previously naive
matching on the parameter structure of that function).
## Test Plan
A `ty check` on a file with template strings (without any further
configuration) doesn't raise errors anymore.
This PR adds two new `ty_extensions` functions,
`reveal_when_assignable_to` and `reveal_when_subtype_of`. These are
closely related to the existing `is_assignable_to` and `is_subtype_of`,
but instead of returning when the property (always) holds, it produces a
diagnostic that describes _when_ the property holds. (This will let us
construct mdtests that print out constraints that are not always true or
always false — though we don't currently have any instances of those.)
I did not replace _every_ occurrence of the `is_property` variants in
the mdtest suite, instead focusing on the generics-related tests where
it will be important to see the full detail of the constraint sets.
As part of this, I also updated the mdtest harness to accept the shorter
`# revealed:` assertion format for more than just `reveal_type`, and
updated the existing uses of `reveal_protocol_interface` to take
advantage of this.
## Summary
Resolves https://github.com/astral-sh/ty/issues/1103
## Test Plan
Edited `crates/ty_python_semantic/resources/mdtest/literal/boolean.md`
with:
```
# Boolean literals
```python
reveal_type(True) # revealed: Literal[False]
reveal_type(False) # revealed: Literal[False]
```
Ran `cargo test -p ty_python_semantic --test mdtest -- mdtest__literal_boolean`
And we get a test failure:
```
running 1 test
test mdtest__literal_boolean ... FAILED
failures:
---- mdtest__literal_boolean stdout ----
boolean.md - Boolean literals (c336e1af3d538acd)
crates/ty_python_semantic/resources/mdtest/literal/boolean.md:4
unmatched assertion: revealed: Literal[False]
crates/ty_python_semantic/resources/mdtest/literal/boolean.md:4
unexpected error: 13 [revealed-type] "Revealed type: `Literal[True]`"
To rerun this specific test, set the environment variable:
MDTEST_TEST_FILTER='boolean.md - Boolean literals (c336e1af3d538acd)'
MDTEST_TEST_FILTER='boolean.md - Boolean literals (c336e1af3d538acd)'
cargo test -p ty_python_semantic --test mdtest --
mdtest__literal_boolean
--------------------------------------------------
thread 'mdtest__literal_boolean' panicked at
crates/ty_test/src/lib.rs:138:5:
Some tests failed.
note: run with `RUST_BACKTRACE=1` environment variable to display a
backtrace
failures:
mdtest__literal_boolean
test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 263
filtered out; finished in 0.18s
error: test failed, to rerun pass `-p ty_python_semantic --test mdtest`
```
As expected.
And when i checkout main and keep the same mdtest file all tests pass (as the repro).
## Summary
We use the `System` abstraction in ty to abstract away the host/system
on which ty runs.
This has a few benefits:
* Tests can run in full isolation using a memory system (that uses an
in-memory file system)
* The LSP has a custom implementation where `read_to_string` returns the
content as seen by the editor (e.g. unsaved changes) instead of always
returning the content as it is stored on disk
* We don't require any file system polyfills for wasm in the browser
However, it does require extra care that we don't accidentally use
`std::fs` or `std::env` (etc.) methods in ty's code base (which is very
easy).
This PR sets up Clippy and disallows the most common methods, instead
pointing users towards the corresponding `System` methods.
The setup is a bit awkward because clippy doesn't support inheriting
configurations. That means, a crate can only override the entire
workspace configuration or not at all.
The approach taken in this PR is:
* Configure the disallowed methods at the workspace level
* Allow `disallowed_methods` at the workspace level
* Enable the lint at the crate level using the warn attribute (in code)
The obvious downside is that it won't work if we ever want to disallow
other methods, but we can figure that out once we reach that point.
What about false positives: Just add an `allow` and move on with your
life :) This isn't something that we have to enforce strictly; the goal
is to catch accidental misuse.
## Test Plan
Clippy found a place where we incorrectly used `std::fs::read_to_string`
This basically splits `list_modules` into a higher level "aggregation"
routine and a lower level "get modules for one search path" routine.
This permits Salsa to cache the lower level components, e.g., many
search paths refer to directories that rarely change. This saves us
interaction with the system.
This did require a fair bit of surgery in terms of being careful about
adding file roots. Namely, now that we rely even more on file roots
existing for correct handling of cache invalidation, there were several
spots in our code that needed to be updated to add roots (that we
weren't previously doing). This feels Not Great, and it would be better
if we had some kind of abstraction that handled this for us. But it
isn't clear to me at this time what that looks like.
This ensures there is some level of consistency between the APIs.
This did require exposing a couple more things on `Module` for good
error messages. This also motivated a switch to an interned struct
instead of a tracked struct. This ensures that `list_modules` and
`resolve_modules` reuse the same `Module` values when the inputs are the
same.
Ref https://github.com/astral-sh/ruff/pull/19883#discussion_r2272520194
by using essentially the same logic for system site-packages, on the
assumption that system site-packages are always a subdir of the stdlib
we were looking for.
## Summary
When seeing a failed test like
```bash
is_subtype_of.md - Subtype relation - Callable - Class literals - Classes with `__new_… (1e9782853227c019)
crates/ty_python_semantic/resources/mdtest/type_properties/is_subtype_of.md:1810 unexpected error: [unresolved-reference] "Name `Aa` used when not defined"
To rerun this specific test, set the environment variable: MDTEST_TEST_FILTER='is_subtype_of.md - Subtype relation - Callable - Class literals - Classes with `__new_… (1e9782853227c019)'
MDTEST_TEST_FILTER='is_subtype_of.md - Subtype relation - Callable - Class literals - Classes with `__new_… (1e9782853227c019)' cargo test -p ty_python_semantic --test mdtest -- mdtest__type_properties_is_subtype_of
```
running the following now works
```bash
MDTEST_TEST_FILTER='is_subtype_of.md - Subtype relation - Callable - Class literals - Classes with `__new_… (1e9782853227c019)' cargo test -p ty_python_semantic --test mdtest -- mdtest__type_properties_is_subtype_of
```
## Test Plan
Do we have tests for the test runner? :)
## Summary
This PR updates the server to keep track of open files both system and
virtual files.
This is done by updating the project by adding the file in the open file
set in `didOpen` notification and removing it in `didClose`
notification.
This does mean that for workspace diagnostics, ty will only check open
files because the behavior of different diagnostic builder is to first
check `is_file_open` and only add diagnostics for open files. So, this
required updating the `is_file_open` model to be `should_check_file`
model which validates whether the file needs to be checked based on the
`CheckMode`. If the check mode is open files only then it will check
whether the file is open. If it's all files then it'll return `true` by
default.
Closes: astral-sh/ty#619
## Test Plan
### Before
There are two files in the project: `__init__.py` and `diagnostics.py`.
In the video, I'm demonstrating the old behavior where making changes to
the (open) `diagnostics.py` file results in re-parsing the file:
https://github.com/user-attachments/assets/c2ac0ecd-9c77-42af-a924-c3744b146045
### After
Same setup as above.
In the video, I'm demonstrating the new behavior where making changes to
the (open) `diagnostics.py` file doesn't result in re-parting the file:
https://github.com/user-attachments/assets/7b82fe92-f330-44c7-b527-c841c4545f8f
## Summary
We noticed that all files get reparsed when workspace diagnostics are
enabled.
I realised that this is because `check_file_impl` access the parsed
module but itself isn't a salsa query.
This pr makes `check_file_impl` a salsa query, so that we only access
the `parsed_module` when the file actually changed. I decided to remove
the salsa query from `check_types` because most functions it calls are
salsa queries itself and having both `check_types` and `check_file` as
salsa querise has the downside that we double cache the diagnostics.
## Test Plan
**Before**
```
2025-07-10 12:54:16.620766000 WARN request{id=19 method="workspace/diagnostic"}:Project::check:check_file{file=file(Id(c0c))}: File `/Users/micha/astral/test/yaml/yaml-stubs/__init__.pyi` was reparsed after being collected in the current Salsa revision
2025-07-10 12:54:16.621942000 WARN request{id=19 method="workspace/diagnostic"}:Project::check:check_file{file=file(Id(c13))}: File `/Users/micha/astral/test/ignore2 2/nested-repository/main.py` was reparsed after being collected in the current Salsa revision
2025-07-10 12:54:16.622107000 WARN request{id=19 method="workspace/diagnostic"}:Project::check:check_file{file=file(Id(c09))}: File `/Users/micha/astral/test/notebook.ipynb` was reparsed after being collected in the current Salsa revision
2025-07-10 12:54:16.622357000 WARN request{id=19 method="workspace/diagnostic"}:Project::check:check_file{file=file(Id(c04))}: File `/Users/micha/astral/test/no-trailing.py` was reparsed after being collected in the current Salsa revision
2025-07-10 12:54:16.622634000 WARN request{id=19 method="workspace/diagnostic"}:Project::check:check_file{file=file(Id(c02))}: File `/Users/micha/astral/test/simple.py` was reparsed after being collected in the current Salsa revision
2025-07-10 12:54:16.623056000 WARN request{id=19 method="workspace/diagnostic"}:Project::check:check_file{file=file(Id(c07))}: File `/Users/micha/astral/test/open/more.py` was reparsed after being collected in the current Salsa revision
2025-07-10 12:54:16.623254000 WARN request{id=19 method="workspace/diagnostic"}:Project::check:check_file{file=file(Id(c11))}: File `/Users/micha/astral/test/ignore-bug/backend/src/subdir/log/some_logging_lib.py` was reparsed after being collected in the current Salsa revision
2025-07-10 12:54:16.623450000 WARN request{id=19 method="workspace/diagnostic"}:Project::check:check_file{file=file(Id(c0f))}: File `/Users/micha/astral/test/yaml/tomllib/__init__.py` was reparsed after being collected in the current Salsa revision
2025-07-10 12:54:16.624599000 WARN request{id=19 method="workspace/diagnostic"}:Project::check:check_file{file=file(Id(c05))}: File `/Users/micha/astral/test/create.py` was reparsed after being collected in the current Salsa revision
2025-07-10 12:54:16.624784000 WARN request{id=19 method="workspace/diagnostic"}:Project::check:check_file{file=file(Id(c00))}: File `/Users/micha/astral/test/lib.py` was reparsed after being collected in the current Salsa revision
2025-07-10 12:54:16.624911000 WARN request{id=19 method="workspace/diagnostic"}:Project::check:check_file{file=file(Id(c0a))}: File `/Users/micha/astral/test/sub/test.py` was reparsed after being collected in the current Salsa revision
2025-07-10 12:54:16.625032000 WARN request{id=19 method="workspace/diagnostic"}:Project::check:check_file{file=file(Id(c12))}: File `/Users/micha/astral/test/ignore2/nested-repository/main.py` was reparsed after being collected in the current Salsa revision
2025-07-10 12:54:16.625101000 WARN request{id=19 method="workspace/diagnostic"}:Project::check:check_file{file=file(Id(c08))}: File `/Users/micha/astral/test/open/test.py` was reparsed after being collected in the current Salsa revision
2025-07-10 12:54:16.625227000 WARN request{id=19 method="workspace/diagnostic"}:Project::check:check_file{file=file(Id(c03))}: File `/Users/micha/astral/test/pseudocode_with_bom.py` was reparsed after being collected in the current Salsa revision
2025-07-10 12:54:16.625353000 WARN request{id=19 method="workspace/diagnostic"}:Project::check:check_file{file=file(Id(c0b))}: File `/Users/micha/astral/test/yaml/yaml-stubs/loader.pyi` was reparsed after being collected in the current Salsa revision
2025-07-10 12:54:16.625543000 WARN request{id=19 method="workspace/diagnostic"}:Project::check:check_file{file=file(Id(c01))}: File `/Users/micha/astral/test/test_trailing.py` was reparsed after being collected in the current Salsa revision
2025-07-10 12:54:16.625616000 WARN request{id=19 method="workspace/diagnostic"}:Project::check:check_file{file=file(Id(c0d))}: File `/Users/micha/astral/test/yaml/tomllib/_re.py` was reparsed after being collected in the current Salsa revision
2025-07-10 12:54:16.625667000 WARN request{id=19 method="workspace/diagnostic"}:Project::check:check_file{file=file(Id(c06))}: File `/Users/micha/astral/test/yaml/main.py` was reparsed after being collected in the current Salsa revision
2025-07-10 12:54:16.625779000 WARN request{id=19 method="workspace/diagnostic"}:Project::check:check_file{file=file(Id(c10))}: File `/Users/micha/astral/test/yaml/tomllib/_types.py` was reparsed after being collected in the current Salsa revision
2025-07-10 12:54:16.627526000 WARN request{id=19 method="workspace/diagnostic"}:Project::check:check_file{file=file(Id(c0e))}: File `/Users/micha/astral/test/yaml/tomllib/_parser.py` was reparsed after being collected in the current Salsa revision
2025-07-10 12:54:16.627959000 DEBUG request{id=19 method="workspace/diagnostic"}:Project::check: Checking all files took 0.007s
```
Now, no more logs regarding reparsing
## Summary
This PR addresses some additional feedback on #19053:
- Renaming the `syntax_error` methods to `invalid_syntax` to match the
lint id
- Moving the standalone `diagnostic_from_violation` function to
`Violation::into_diagnostic`
- Removing the `Ord` and `PartialOrd` implementations from `Diagnostic`
in favor of `Diagnostic::start_ordering`
## Test Plan
Existing tests
## Additional Follow-ups
Besides these, I also put the following comments on my todo list, but
they seemed like they might be big enough to have their own PRs:
- [Use `LintId::IOError` for IO
errors](https://github.com/astral-sh/ruff/pull/19053#discussion_r2189425922)
- [Move `Fix` and
`Edit`](https://github.com/astral-sh/ruff/pull/19053#discussion_r2189448647)
- [Avoid so many
unwraps](https://github.com/astral-sh/ruff/pull/19053#discussion_r2189465980)
## Summary
This PR is a collaboration with @AlexWaygood from our pairing session
last Friday.
The main goal here is removing `ruff_linter::message::OldDiagnostic` in
favor of
using `ruff_db::diagnostic::Diagnostic` directly. This involved a few
major steps:
- Transferring the fields
- Transferring the methods and trait implementations, where possible
- Converting some constructor methods to free functions
- Moving the `SecondaryCode` struct
- Updating the method names
I'm hoping that some of the methods, especially those in the
`expect_ruff_*`
family, won't be necessary long-term, but I avoided trying to replace
them
entirely for now to keep the already-large diff a bit smaller.
### Related refactors
Alex and I noticed a few refactoring opportunities while looking at the
code,
specifically the very similar implementations for
`create_parse_diagnostic`,
`create_unsupported_syntax_diagnostic`, and
`create_semantic_syntax_diagnostic`.
We combined these into a single generic function, which I then copied
into
`ruff_linter::message` with some small changes and a TODO to combine
them in the
future.
I also deleted the `DisplayParseErrorType` and `TruncateAtNewline` types
for
reporting parse errors. These were added in #4124, I believe to work
around the
error messages from LALRPOP. Removing these didn't affect any tests, so
I think
they were unnecessary now that we fully control the error messages from
the
parser.
On a more minor note, I factored out some calls to the
`OldDiagnostic::filename`
(now `Diagnostic::expect_ruff_filename`) function to avoid repeatedly
allocating
`String`s in some places.
### Snapshot changes
The `show_statistics_syntax_errors` integration test changed because the
`OldDiagnostic::name` method used `syntax-error` instead of
`invalid-syntax`
like in ty. I think this (`--statistics`) is one of the only places we
actually
use this name for syntax errors, so I hope this is okay. An alternative
is to
use `syntax-error` in ty too.
The other snapshot changes are from removing this code, as discussed on
[Discord](https://discord.com/channels/1039017663004942429/1228460843033821285/1388252408848847069):
34052a1185/crates/ruff_linter/src/message/mod.rs (L128-L135)
I think both of these are technically breaking changes, but they only
affect
syntax errors and are very narrow in scope, while also pretty
substantially
simplifying the refactor, so I hope they're okay to include in a patch
release.
## Test plan
Existing tests, with the adjustments mentioned above
---------
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
Douglas Creager
David Peter
renovate[bot]
Matthew Mckee
Micha Reiser
Andrew Gallant
Aria Desires
Dhruv Manilawala
Charlie Marsh
Brent Westbrook