Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
40 commits
Select commit Hold shift + click to select a range
37effea
Add hover assertion type to mdtest framework
dcreager Oct 8, 2025
be47fbe
Add CheckOutput enum to support hover results in mdtest
dcreager Oct 8, 2025
11e5ecb
Implement hover type inference and matching for mdtest
dcreager Oct 8, 2025
6e73b85
Update PLAN.md - mark steps 4 & 5 complete
dcreager Oct 8, 2025
5c75e91
[ty_test] Refactor hover logic into separate module
dcreager Oct 8, 2025
9d8e35b
[ty_test] Simplify infer_type_at_position using as_expr_ref()
dcreager Oct 8, 2025
6370aea
[ty_test] Fix find_covering_node to correctly find minimal node
dcreager Oct 8, 2025
319f5be
[ty_test] Simplify find_covering_node by comparing range lengths
dcreager Oct 8, 2025
19600ec
[ty_test] Fix hover assertion line number calculation
dcreager Oct 8, 2025
ab26136
[ty_test] Use let-else pattern in generate_hover_outputs
dcreager Oct 8, 2025
ac1b68c
[ty_test] Refactor: rename diagnostic.rs to check_output.rs
dcreager Oct 8, 2025
35a5fd7
[ty_test] Extract HoverOutput type from CheckOutput enum
dcreager Oct 8, 2025
35b568d
[ty_test] Move HoverOutput to hover module
dcreager Oct 8, 2025
0198857
[ty_test] Simplify unmatched output handling
dcreager Oct 8, 2025
51d5bc7
[ty_test] Calculate hover column at parse time
dcreager Oct 8, 2025
93db883
[ty_test] Add use statements to hover.rs
dcreager Oct 8, 2025
8221450
[ty_test] Store HoverAssertion.column as zero-based
dcreager Oct 8, 2025
c3626a6
[ty_test] Fix hover column calculation to use line position
dcreager Oct 8, 2025
a2eaf7c
[ty_test] Calculate hover column at parse time
dcreager Oct 8, 2025
8c12fcb
[ty_test] Use named fields for UnparsedAssertion::Hover
dcreager Oct 8, 2025
c574dff
[ty_test] Fix column units: use character offset not byte offset
dcreager Oct 8, 2025
6d1c549
[ty_test] Simplify column calculation using line_column
dcreager Oct 8, 2025
b683da8
[ty_test] Store hover column as OneIndexed
dcreager Oct 8, 2025
adf58b6
[ty_test] Change match_line to take slice of CheckOutput
dcreager Oct 8, 2025
180d9de
[ty_test] Fix clippy warnings in hover module
dcreager Oct 8, 2025
4177246
[ty_test] Store CheckOutput references in SortedCheckOutputs
dcreager Oct 8, 2025
847e5f0
[ty] Handle expression statements in hover type inference
dcreager Oct 8, 2025
b18d213
[ty] Update PLAN.md with testing progress
dcreager Oct 8, 2025
eaba8bc
[ty] Add comprehensive hover.md mdtest (partial)
dcreager Oct 8, 2025
6ddf729
clean up mdtest
dcreager Oct 8, 2025
c08a2d6
[ty] Fix hover to prefer expression nodes over identifiers
dcreager Oct 8, 2025
8426cc6
use ty_ide
dcreager Oct 8, 2025
7b88440
update tests
dcreager Oct 9, 2025
39353da
remove finished plan
dcreager Oct 9, 2025
1810b7c
precommit
dcreager Oct 9, 2025
fbc211f
update tests
dcreager Oct 9, 2025
b438631
clean up the diff
dcreager Oct 9, 2025
34b463d
reword some docs
dcreager Oct 9, 2025
0817a36
spelling
dcreager Oct 9, 2025
eb95e49
clean up some claude-isms
dcreager Oct 9, 2025
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

9 changes: 3 additions & 6 deletions crates/ty_ide/src/goto.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ use ty_python_semantic::{
};

#[derive(Clone, Debug)]
pub(crate) enum GotoTarget<'a> {
pub enum GotoTarget<'a> {
Expression(ast::ExprRef<'a>),
FunctionDef(&'a ast::StmtFunctionDef),
ClassDef(&'a ast::StmtClassDef),
Expand Down Expand Up @@ -269,7 +269,7 @@ impl<'db> DefinitionsOrTargets<'db> {
}

impl GotoTarget<'_> {
pub(crate) fn inferred_type<'db>(&self, model: &SemanticModel<'db>) -> Option<Type<'db>> {
pub fn inferred_type<'db>(&self, model: &SemanticModel<'db>) -> Option<Type<'db>> {
let ty = match self {
GotoTarget::Expression(expression) => expression.inferred_type(model),
GotoTarget::FunctionDef(function) => function.inferred_type(model),
Expand Down Expand Up @@ -820,10 +820,7 @@ fn definitions_to_navigation_targets<'db>(
}
}

pub(crate) fn find_goto_target(
parsed: &ParsedModuleRef,
offset: TextSize,
) -> Option<GotoTarget<'_>> {
pub fn find_goto_target(parsed: &ParsedModuleRef, offset: TextSize) -> Option<GotoTarget<'_>> {
let token = parsed
.tokens()
.at_offset(offset)
Expand Down
2 changes: 1 addition & 1 deletion crates/ty_ide/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ pub use all_symbols::{AllSymbolInfo, all_symbols};
pub use completion::{Completion, CompletionKind, CompletionSettings, completion};
pub use doc_highlights::document_highlights;
pub use document_symbols::document_symbols;
pub use goto::{goto_declaration, goto_definition, goto_type_definition};
pub use goto::{find_goto_target, goto_declaration, goto_definition, goto_type_definition};
pub use goto_references::goto_references;
pub use hover::hover;
pub use inlay_hints::{InlayHintKind, InlayHintLabel, InlayHintSettings, inlay_hints};
Expand Down
140 changes: 140 additions & 0 deletions crates/ty_python_semantic/resources/mdtest/hover.md
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should these tests be in ty_ide? I'd find it confusing that a change in ty_ide can break a test in ty_python_semantic. It would probably take me a while to realize that a test in ty_python_semantic isn't failing because of a change in ty_python_semantic (which would be the first place where I go look), but because of a change in ty_ide.

Original file line number Diff line number Diff line change
@@ -0,0 +1,140 @@
# Hover type assertions

You can use the `hover` assertion to test the inferred type of an expression. This exercises the
same logic as the hover LSP action.

Typically, you will not need to use the `hover` action to test the behavior of our type inference
code, since you can also use `reveal_type` to display the inferred type of an expression. Since
`reveal_type` is part of the standard library, we prefer to use it when possible.

However, there are certain situations where `reveal_type` and `hover` will give different results.
In particular, `reveal_type` is not transparent to bidirectional type checking, as seen in the
"Different results" section below.

## Syntax

### Basic syntax

The `hover` assertion operates on a specific location in the source text. We find the "inner-most"
expression at that position, and then query the inferred type of that expression. The row to query
is identified just like any other mdtest assertion. The column to query is identified by a down
arrow (↓) in the assertion. (Note that the down arrow should always appear immediately before the
`hover` keyword in the assertion.)

```py
def test_basic_types(parameter: int) -> None:
# ↓ hover: int
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I feel like the makes it a little hard to write hover assertions in mdtest, because I'd end up having to copy and paste it every time or open up a Unicode pallette 😄 is there an ASCII character we could use instead? One of +, ! or | might work?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is the year 2025, Alex. Let us have some nice things.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I had worried about that part. I went with the down arrow partly because I agree with David's half-snark, and partly because I thought that it was more important to optimize here for readability than writability, and the down arrow is visually much clearer.

Copy link
Member

@AlexWaygood AlexWaygood Oct 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

more important to optimize here for readability than writability

Hmmm, I'm not sure I agree. One of the best things about mdtest is how readable it makes our tests, but another of the best things about mdtest is how low-friction it is to write new tests. It's so easy to add new assertions to an existing mdtest suite; it doesn't come with nearly as much boilerplate as writing all our unit tests in Rust or even Python would. And I think that's resulted in us writing lots and lots of assertions, and having a very robustly tested type checker -- great things happen when you make it easy to write tests!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still prefer the down arrow over an ASCII character for this. It is much clearer in intent, and even if you have to fall back on copy/paste, I don't feel like that would be as much of a deterrent to writing these assertions as you suggest. (I completely agree with your argument when talking about mdtest existing in the first place — the marginal ease of writing these over the previous Rust tests is huge! But I don't think that argument applies with the same force when considering something like this.)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is the year 2025, Alex. Let us have some nice things.

Shouldn't it be an emoji then? ⬇️

parameter

# ↓ hover: Literal[10]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have a good suggestion as an alternative, but a downside of the arrow is that I'd always have to copy paste it from somewhere because I don't know how to type it on a keyboard.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

number = 10

# ↓ hover: Literal["hello"]
text = "hello"
```

### Multiple hovers on the same line

We can have multiple hover assertions for different positions on the same line:

```py
# ↓ hover: Literal[1]
# ↓ hover: Literal[2]
# ↓ hover: Literal[3]
total = 1 + 2 + 3

# ↓ hover: Literal[5]
# ↓ hover: Literal[3]
result = max(5, 3)
```

### Hovering works on every character in an expression

```py
def _(param: bool) -> None:
# ↓ hover: bool
# ↓ hover: bool
# ↓ hover: bool
# ↓ hover: bool
# ↓ hover: bool
result = param
```

### Hovering with unicode characters

```py
def _(café: str) -> None:
# ↓ hover: str
# ↓ hover: str
# ↓ hover: str
# ↓ hover: str
result = café
```

## Different results for `reveal_type` and `hover`

```py
from typing import overload

def f(x: dict[str, int]) -> None: ...

# revealed: dict[Unknown, Unknown]
f(reveal_type({}))

# ↓ hover: dict[str, int]
f({})
```
Comment on lines +74 to +86
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This example confuses me somewhat, because the introduction to this mdtest suite states that

You can use the hover assertion to test the infered type of an expression

But dict[str, int] is not the inferred type of {} here. The inferred type of {} here is dict[Unknown, Unknown] (the answer reveal_type gave), but we allow {} to be passed into the x parameter of f because dict[Unknown, Unknown] is assignable to dict[str, int], the annotation for the x parameter.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As of #20635, we're doing bidirectional type checking for call arguments, and using the parameter annotation as the type context. And as of #20523, we're using the type context to infer a more precise type for dictionary literals. So dict[str, int] really is the infered type of {} here, when it's passed directly in as the argument. But when passing it to reveal_type, the parameter annotation is just _T, and so we don't have the more precise type context.

That might suggest that we want to propagate the type context through these kinds of inner calls (not just for reveal_type), but even then, with bidi checking it's now the case that we can have different infered types for the same expression depending on how its used, and it's useful to have an assertion that cannot influence that type context in any way.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That might suggest that we want to propagate the type context through these kinds of inner calls (not just for reveal_type)

That sounds like it might make sense to me... It seems quite counterintuitive to me that hovering over the first [""] in the playground for this example gives a different type than hovering over the second [""]:

from typing import reveal_type

def needs_list_of_strings(x: list[str | None]): ...

def identity[T](x: T) -> T:
    return x

needs_list_of_strings([""])
needs_list_of_strings(identity([""]))

https://play.ty.dev/53299738-b406-4e46-ba40-c0240225f0ae

with bidi checking it's now the case that we can have different infered types for the same expression depending on how its used, and it's useful to have an assertion that cannot influence that type context in any way

Fair enough! The additional code in ty_test to make it work also isn't free, though -- there's a maintenance burden there, and a small cognitive burden (I now have to think about whether a reveal_type assertion or a hover assertion will be more appropriate when I'm writing my tests!). I still feel like I'd be interested in a few more examples of where the current reveal_type behaviour is causing issues for your generics work.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As of #20635, we're doing bidirectional type checking for call arguments, and using the parameter annotation as the type context. And as of #20523, we're using the type context to infer a more precise type for dictionary literals. So dict[str, int] really is the infered type of {} here, when it's passed directly in as the argument. But when passing it to reveal_type, the parameter annotation is just _T, and so we don't have the more precise type context.

Thanks BTW -- I hadn't fully internalised that this was the consequence of those changes! Makes sense.


## Hovering on different expression types

### Literals

```py
# ↓ hover: Literal[42]
int_value = 42

# ↓ hover: Literal["test"]
string_value = "test"

# ↓ hover: Literal[True]
bool_value = True
```

### Names and attributes

```py
class MyClass:
value: int

def test_attributes(instance: MyClass) -> None:
# ↓ hover: MyClass
instance

# ↓ hover: int
instance.value
```

### Function definitions

```py
def f(x: int) -> None: ...

# ↓ hover: def f(x: int) -> None
result = f
```

### Binary operations

```py
# ↓ hover: Literal[10]
# ↓ hover: Literal[20]
result = 10 + 20
```

### Comprehensions

```py
# List comprehension
# ↓ hover: list[@Todo(list comprehension element type)]
result = [x for x in range(5)]
```
1 change: 1 addition & 0 deletions crates/ty_test/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ ruff_source_file = { workspace = true }
ruff_text_size = { workspace = true }
ruff_python_ast = { workspace = true }
ty_python_semantic = { workspace = true, features = ["serde", "testing"] }
ty_ide = { workspace = true }
ty_static = { workspace = true }
ty_vendored = { workspace = true }

Expand Down
Loading
Loading