Skip to content

Conversation

@MatthewMckee4
Copy link
Contributor

@MatthewMckee4 MatthewMckee4 commented Nov 17, 2025

Summary

Resolves astral-sh/ty#1078.

Screencast_20251117_212227.webm.

To find valid identifiers in the display string we do the following.

  1. iterate over each char
  2. start with max length string (current char to end of string)
  3. reduce string length until valid identifier is found
  4. If no valid identifier is found, we append the character to the label parts.
  5. if we do find one we check all global symbols (this should be sped up) to find the symbol we want.
  6. Add inlay label part and navigation target.

Test Plan

Our current snapshots for inlay hint locations are very hard to understand and frankly arent useful at all.

@astral-sh-bot
Copy link

astral-sh-bot bot commented Nov 17, 2025

Diagnostic diff on typing conformance tests

No changes detected when running ty on typing conformance tests ✅

@astral-sh-bot
Copy link

astral-sh-bot bot commented Nov 17, 2025

mypy_primer results

No ecosystem changes detected ✅

No memory usage changes detected ✅

@astral-sh-bot
Copy link

astral-sh-bot bot commented Nov 17, 2025

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

Formatter (stable)

✅ ecosystem check detected no format changes.

Formatter (preview)

✅ ecosystem check detected no format changes.

@MatthewMckee4
Copy link
Contributor Author

MatthewMckee4 commented Nov 17, 2025

There are a lot of false positives (random targets) here.

  • In "Todo" annotations
  • In literal string annotations.

I think this is a good test but probably not a good solution.

Interested to see people's thoughts.

@MatthewMckee4 MatthewMckee4 marked this pull request as ready for review November 17, 2025 21:42
@amyreese amyreese changed the title Support inlay hint type hint location [ty] Support inlay hint type hint location Nov 17, 2025
@MichaReiser MichaReiser added the server Related to the LSP server label Nov 18, 2025
@MichaReiser
Copy link
Member

Thanks to work on this.

It feels wrong to me to render the type to a string, and then trying to reparse the information. I think we should either:

  • Accept the fact that the LSP needs more flexibility and use a separate rendering pipeline all together
  • Use a visitor approach where we don't write to a std::fmt::Formatter but to our own formatter instead, which is a trait with visit_type(&mut self, ty), write_str etc. methods. I'd have to play with the problem myself to get a good sense for how an ergonomic API for this would look like.

We do have a similar problem for showing go to targets on hover. It's slightly different than inlay but we would want to collect all types, resolve their definitions, and render the definitions in a separate list. That's why I think a visitor approach where one implementation writes to a normal formatter, another that creates the inlay hint parts, and a third that writes to a normal formatter and collects the go to definition targets seems best?

But curious to hear what @Gankra thinks. She worked on this code more than I

@AlexWaygood
Copy link
Member

  • Use a visitor approach where we don't write to a std::fmt::Formatter but to our own formatter instead, which is a trait with visit_type(&mut self, ty), write_str etc. methods. I'd have to play with the problem myself to get a good sense for how an ergonomic API for this would look like.

For reference, we have a TypeVisitor trait at

pub(crate) trait TypeVisitor<'db> {
, but I'm not totally sure from your description whether it would work for your purposes here

@MichaReiser
Copy link
Member

I think we would want something more specific to rendering types. I very much doubt that we need separate visitor methods for each type. We're more interested in getting "hook-in-points" to know when we enter a new type and when we exit it, as well as having a way to write the label for the current type.

@MatthewMckee4
Copy link
Contributor Author

MatthewMckee4 commented Nov 18, 2025

It feels wrong to me to render the type to a string

I agree

It also feels wrong to duplicate display logic from display.rs.

Could we make changes in display.rs? Possibly making some intermediate struct that collects all the parts of the string, importantly parts that have a definition (or just TextRange) that we can goto. Then implement Display for that.

Meaning we can get all the information we need from the intermediate struct.

@AlexWaygood AlexWaygood added the ty Multi-file analysis & type inference label Nov 18, 2025
@MichaReiser
Copy link
Member

Could we make changes in display.rs? Possibly making some intermediate struct that collects all the parts of the string, importantly parts that have a definition (or just TextRange) that we can goto. Then implement Display for that.

I think this would work but has two downsides:

  • We collect a lot of unnecessary information for the common case where we just want to render a type to a string
  • It's not enough for go to in hover (Go-to definition in hover ty#1575), requiring another refactor.

The approach I would try is to define a new trait:

pub trait DisplayTypeFormatter<'db> {
	fn enter_type(&mut self, ty: Type<'db>) {}
	fn exit_type(&mut self) {}
	fn write_str(&mut self, part: &str);
}

I think we can even implement std::fmt::Write for DisplayVisitor so that we can keep using write!(f, "text{interpolation}"); by either implementing it for &dyn DisplayTypeFormatter or generic over T

impl std::fmt::Write for dyn DisplayTypeFormatter<'_> {
	fn write_str(&mut self, text: &str) -> std::fmt::Result<()> {
		DisplayFormatter::write_str(self, text);
		Ok(())
	}
}

The implementation to formatting the type to a string is:

struct DisplayToString(String);

impl DisplayTypeFormatter<'_> for DisplayToString {
	fn write_str(&mut self, part: &str) {
		self.0.push_str(part);
	}
}

Extracting the inlay is a bit tricky. It will require maintaining a stack of inlay parts where you push to the stack when entering a new type and popping from the stack when exiting (the stack is your scratch pad and you move them to a finalized state once finished.

The hover implementation wraps a DisplayTypeFormatter and also keeps a HashSet of all seen Definition.

Note: I haven't tried any of this, so this might not work or require more methods or different signatures but I think this is worth exploring.

@Gankra
Copy link
Contributor

Gankra commented Nov 18, 2025

Oh how serendipitous, I was just thinking about implementing this. We already have the visitor pattern in our display code, it's used by signature_help. I would absolutely implement this feature by expanding upon this design:

impl DisplaySignature<'_> {
/// Get detailed display information including component ranges
pub(crate) fn to_string_parts(&self) -> SignatureDisplayDetails {
let mut writer = SignatureWriter::Details(SignatureDetailsWriter::new());
self.write_signature(&mut writer).unwrap();
match writer {
SignatureWriter::Details(details) => details.finish(),
SignatureWriter::Formatter(_) => unreachable!("Expected Details variant"),
}
}

// Write parameter with range tracking
let param_name = parameter.display_name();
writer.write_parameter(
&parameter.display_with(self.db, self.settings.singleline()),
param_name.as_deref(),
)?;

@MatthewMckee4
Copy link
Contributor Author

Cool, I'll close this for now and let you do your thing, I'm happy to link it up to the inlay hints if you don't get to that.

I will also think about a better way to snapshot the targets.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

server Related to the LSP server ty Multi-file analysis & type inference

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support goto definition on inlay hints

4 participants