[red-knot] Add inlay type hints by MatthewMckee4 · Pull Request #17214 · astral-sh/ruff

MatthewMckee4 · 2025-04-04T21:49:07Z

Summary

Fixes #17205

Test Plan

Add tests to red_knot_ide/src/inlay_hints.rs

simplescreenrecorder-2025-04-09_22.14.38.mp4

simplescreenrecorder-2025-04-09_22.21.35.mp4

github-actions · 2025-04-04T21:52:21Z

`mypy_primer` results

No ecosystem changes detected ✅

MatthewMckee4 · 2025-04-04T21:53:04Z

Currently this adds type hints to almost everything, need to work on that

MatthewMckee4 · 2025-04-04T21:59:22Z

Also need to make sure we dont add type hints when there is already one there

MatthewMckee4 · 2025-04-05T15:10:13Z

@MichaReiser suggested the following

I think I'd take an approach closer to rust analyzer:

Write a custom SourceOrderVisitor that traverses the AST
enter_node returns TraverseSignal::Skip if the node isn't in the queried range
Compute the hints in visit_* methods but only for the nodes you want by using node.inferred_type (because we never want to show declared types?)

https://github.com/rust-lang/rust-analyzer/blob/15d87419f1a123d8f888d608129c3ce3ff8f13d4/crates/ide/src/inlay_hints.rs#L224-L283

MatthewMckee4 · 2025-04-05T15:21:33Z

Im not sure how to work with settings to be able to enable/disable this

MichaReiser · 2025-04-05T15:31:39Z

Did you push your latest changes? The current approach doesn't use a custom visitor. Could you add a test plan so that I get a sense for how the feature works?

MatthewMckee4 · 2025-04-05T15:37:22Z

Ive not implemented a custom visitor yet, ill get on that. red_knot_python_semantic/src/visitor.rs?

MatthewMckee4 · 2025-04-05T22:44:26Z

Does the first option look good?

InSyncWithFoo · 2025-04-05T22:59:28Z

@MatthewMckee4 The second is more pleasing to the eyes, but I guess the hints won't be as nice for something like (a1, *a2) = a.

MatthewMckee4 · 2025-04-05T23:07:56Z

@InSyncWithFoo Yeah the hints dont change from the second image with (a1, *a2) = a

This reverts commit bdb75b1.

MichaReiser · 2025-04-06T17:12:33Z

I find rust-analyzer's inlay type hints useful in two situations:

I don't think this PR implements 2.

From the LSP specification

/**
* Optional text edits that are performed when accepting this inlay hint.
*
* Note that edits are expected to change the document so that the inlay
* hint (or its nearest variant) is now part of the document and the inlay
* hint itself is now obsolete.
*
* Depending on the client capability inlayHint.resolveSupport clients
* might resolve this property late using the resolve request.
*/
textEdits?: TextEdit[];

That means, we could generalize the type. Although I'm not quiet sure what the rules would be ;) But I don't think this should block this PR

crates/red_knot_ide/src/inlay_hints.rs

Co-authored-by: Micha Reiser <[email protected]>

crates/red_knot_server/src/server/api/requests/inlay_hints.rs

crates/red_knot_ide/src/inlay_hints.rs

MichaReiser

This is great. We should respect the range provided by the editor and I don't think the options approach is what we want, let's remove that for now and always enable inlay hints.

Could you update the test plan with a recording of the playground and LSP?

crates/red_knot_server/src/server/api/requests/inlay_hints.rs

MichaReiser · 2025-04-07T07:55:52Z

crates/red_knot_server/src/server/api/requests/inlay_hints.rs

+                    tooltip: None,
+                    padding_left: None,
+                    padding_right: None,
+                    data: None,
+                    text_edits: None,


Nit: Not for this PR but we want to use the InlayHintResolveRequest to lazily resolve the tooltip and text edits. It may be worth adding a comment, see https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#inlayHint_resolve

crates/red_knot_server/src/server/api.rs

crates/red_knot_wasm/src/lib.rs

crates/red_knot_ide/src/inlay_hints.rs

crates/red_knot_server/src/server/api/requests/inlay_hints.rs

crates/red_knot_ide/src/inlay_hints.rs

MatthewMckee4 · 2025-04-07T08:51:46Z

I might not be able to get to this for the rest of the day, if you want to have a go at updating the tests or the range then go for it!

MichaReiser · 2025-04-07T09:19:19Z

I might not be able to get to this for the rest of the day, if you want to have a go at updating the tests or the range then go for it!

There's no hurry and the chances for merge conflicts are very low. Take your time.

MichaReiser

This looks good. I've a few smaller nit comments that would be great to address beforehand.

Please, also updated your PR summary with a test plan for both playground and the vs code extension (record a short video, similar to what I did with on hover)

MichaReiser · 2025-04-09T06:52:12Z

crates/red_knot_ide/src/inlay_hints.rs

+    fn handle_assign_expr(&mut self, expr: &Expr) {
+        match expr {
+            Expr::Tuple(tuple) => {
+                for element in &tuple.elts {
+                    self.handle_assign_expr(element);
+                }
+            }
+            _ => {
+                let ty = expr.inferred_type(&self.model);
+                self.add_hint(expr.range(), ty);
+            }
+        }
+    }


I wonder if we could use the same approach as in SemanticIndexBuilder where we have a state variable on InlayHintVisitor indicating if it is currently visiting an assignment.

Then, inside visit_expr, add an inlay hint for every ExprName in a Store context.

This has the benefit that the visitor doesn't traverse the assignment targets multiple times.

I believe we only traverse (and call inferred_type) on assignments like

a = b = 1

Then, inside visit_expr, add an inlay hint for every ExprName in a Store context.

How would we get the type in visit_expr?

crates/red_knot_ide/src/inlay_hints.rs

MichaReiser · 2025-04-09T06:55:03Z

crates/red_knot_ide/src/inlay_hints.rs

+            for hint in hints {
+                let end_position = (hint.range.end().to_u32() as usize) + offset;
+                let hint_str = format!("[{}]", hint.display(&self.db));
+                buf.insert_str(end_position, &hint_str);
+                offset += hint_str.len();
+            }


Oh, I like how you render the "hints" inline!

I'm happy to go with this for now. An alternative would be to use the Diagnostic again and render an annotation at hint.range.end where the primary message is the hinted type.

I think I've a slight preference to using diagnostics but that doesn't need to block this PR and can be done as a follow-up. My main reasoning is that other developers would need to look at both the test and the implementation to understand that the test infra adds the brackets ([]) and the implementation adds the : and ->. The diagnostics has the benefit to say that "here is where this hint will be added".

crates/red_knot_ide/src/inlay_hints.rs

crates/red_knot_server/src/server/api/requests/inlay_hints.rs

MichaReiser · 2025-04-09T07:02:54Z

crates/red_knot_wasm/src/lib.rs


+#[wasm_bindgen]
+impl Range {
+    #[wasm_bindgen(constructor)]
+    pub fn new(start: Position, end: Position) -> Self {
+        Self { start, end }
+    }
+}
+


I don't think this is needed, unless you want the JS client to create Range instances.

For the rust code, using Range { start, end } should do it

i dont think it was working as it was doing some sort of instance check on range

crates/red_knot_wasm/src/lib.rs

MatthewMckee4 added 2 commits April 4, 2025 22:48

Initial progress in playground

51f1bc4

Remove redundant calls

9a98afb

Fix build and filter some more types

b110361

AlexWaygood added the ty Multi-file analysis & type inference label Apr 4, 2025

MatthewMckee4 added 3 commits April 4, 2025 23:36

Refactory and add more filter logic

978cc48

Fix build

cb898fe

Add knot server functionality for inlay hints

41f583e

MatthewMckee4 marked this pull request as ready for review April 5, 2025 15:10

MatthewMckee4 requested review from AlexWaygood, MichaReiser, carljm, dcreager and sharkdp as code owners April 5, 2025 15:10

MatthewMckee4 marked this pull request as draft April 5, 2025 15:46

MatthewMckee4 added 2 commits April 5, 2025 23:09

Add InlayHintVisitor to visit only the statements we want

0da803d

Add tests

135d8a6

Merge branch 'main' into inlay-hints

00eeaf5

MatthewMckee4 added 3 commits April 6, 2025 00:08

Fix build

bdb75b1

Revert "Fix build"

52106bc

This reverts commit bdb75b1.

Fix build

a135a6d

run generate-all

c1c5434

MichaReiser reviewed Apr 7, 2025

View reviewed changes

crates/red_knot_ide/src/inlay_hints.rs Outdated Show resolved Hide resolved

Update inlay_hints.rs

08cfce7

Co-authored-by: Micha Reiser <[email protected]>

MichaReiser reviewed Apr 7, 2025

View reviewed changes

crates/red_knot_server/src/server/api/requests/inlay_hints.rs Outdated Show resolved Hide resolved

MichaReiser reviewed Apr 7, 2025

View reviewed changes

crates/red_knot_ide/src/inlay_hints.rs Outdated Show resolved Hide resolved

MichaReiser reviewed Apr 7, 2025

View reviewed changes

crates/red_knot_ide/src/inlay_hints.rs Show resolved Hide resolved

Merge branch 'main' into inlay-hints

d1d071a

MichaReiser requested changes Apr 7, 2025

View reviewed changes

MatthewMckee4 added 2 commits April 7, 2025 09:37

Apply changes as per review

313f76f

run generate-all

350ff11

MatthewMckee4 changed the title ~~[red-knot] Add (optional) inlay type hints~~ [red-knot] Add inlay type hints Apr 7, 2025

MatthewMckee4 added 3 commits April 9, 2025 01:29

Changes per review

85377eb

Merge branch 'main' into inlay-hints

808a03a

Fix playground

b901496

MichaReiser reviewed Apr 9, 2025

View reviewed changes

Update per review

b2bc7d2

carljm removed their request for review April 9, 2025 15:00

sharkdp removed their request for review April 9, 2025 17:02

MatthewMckee4 and others added 4 commits April 9, 2025 21:47

Merge branch 'main' into inlay-hints

9e66656

Simplify tests

e438ffa

Show hint for all ExprName::Store locations

70fb3e1

Extract helper in wasm code

59f3c7f

MichaReiser approved these changes Apr 10, 2025

View reviewed changes

MichaReiser enabled auto-merge (squash) April 10, 2025 09:19

MichaReiser merged commit 10e4412 into astral-sh:main Apr 10, 2025
21 checks passed

MatthewMckee4 deleted the inlay-hints branch April 19, 2025 13:58

AlexWaygood mentioned this pull request Aug 14, 2025

[ty] Default ty.inlayHints.* server settings to true #19910

Merged

Comments

Conversation

MatthewMckee4 commented Apr 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test Plan

Uh oh!

github-actions bot commented Apr 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

mypy_primer results

Uh oh!

MatthewMckee4 commented Apr 4, 2025

Uh oh!

MatthewMckee4 commented Apr 4, 2025

Uh oh!

MatthewMckee4 commented Apr 5, 2025

Uh oh!

MatthewMckee4 commented Apr 5, 2025

Uh oh!

MichaReiser commented Apr 5, 2025

Uh oh!

MatthewMckee4 commented Apr 5, 2025

Uh oh!

MatthewMckee4 commented Apr 5, 2025

Uh oh!

InSyncWithFoo commented Apr 5, 2025

Uh oh!

MatthewMckee4 commented Apr 5, 2025

Uh oh!

MichaReiser commented Apr 6, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

MichaReiser left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

MichaReiser Apr 7, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

MatthewMckee4 commented Apr 7, 2025

Uh oh!

MichaReiser commented Apr 7, 2025

Uh oh!

MichaReiser left a comment

Choose a reason for hiding this comment

Uh oh!

MichaReiser Apr 9, 2025

Choose a reason for hiding this comment

Uh oh!

MatthewMckee4 Apr 9, 2025 • edited by AlexWaygood Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

MichaReiser Apr 9, 2025

Choose a reason for hiding this comment

Uh oh!

dhruvmanila Apr 9, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

MichaReiser Apr 9, 2025

Choose a reason for hiding this comment

Uh oh!

MatthewMckee4 Apr 9, 2025

Choose a reason for hiding this comment

MatthewMckee4 commented Apr 4, 2025 •

edited

Loading

github-actions bot commented Apr 4, 2025 •

edited

Loading

`mypy_primer` results

MatthewMckee4 Apr 9, 2025 •

edited by AlexWaygood

Loading