Skip to content

Resolve identifiers inside quoted annotations in the LSP. - #5243

Open
Arham-Begani wants to merge 2 commits into
meta:mainfrom
Arham-Begani:lsp-quoted-annotation-identifiers
Open

Arham-Begani wants to merge 2 commits into
meta:mainfrom
Arham-Begani:lsp-quoted-annotation-identifiers

Conversation

@Arham-Begani

Copy link
Copy Markdown

Summary

Part of #4740: hover, go-to-definition, references and rename inside quoted annotations. Semantic highlighting is not covered here (see #3615).

The rest of this description was drafted with an AI assistant (Claude Code).

The binder already parses forward-reference strings such as x: "list[Foo]" and binds the names inside them at their real source ranges. The LSP, however, located the cursor in the original AST, where the annotation is a single string literal. As a result:

  • hover showed only a bare type[Foo], without the name or docstring
  • go-to-definition returned nothing, and in list["Foo"] it jumped to list.__getitem__
  • references and rename skipped every occurrence inside a string

The PR has two commits:

  1. Tests that show the current behavior, with // BUG: comments.
  2. The fix, which updates those expectations.

Approach (all in pyrefly/lib/state/lsp.rs):

  • forward_ref_at parses the string under the cursor. It uses the parse only if a name in it has a Key::BoundName. Only strings the binder parsed as types have such keys, so ordinary strings, Literal["..."] values and malformed annotations stay strings.

  • identifier_at and find_definition resolve against that parsed expression. This fixes hover, definition, declaration and type definition, including TYPE_CHECKING imports and qualified names like "lib.Helper".

  • The local variable reference scan also descends into strings that mention the name. This fixes references, rename and document highlight. References to imported names already included string occurrences through the solver's index.

  • Only strings whose contents map exactly onto the source (AnnotationKind::Simple) are handled. Strings with escapes or implicit concatenation are skipped, because positions inside them can't be mapped exactly.

  • I am a human, and I fully understand the code I am submitting.

Test Plan

New tests:

  • definition::goto_def_inside_quoted_annotation: simple, TYPE_CHECKING import, nested subscript, list["Model"], and both parts of "lib.Helper"
  • definition::goto_def_ignores_strings_that_are_not_type_expressions: x = "Model", Literal["Model"] and a malformed "list[Model" resolve to nothing, before and after the fix
  • hover::hover_inside_quoted_annotation: after the fix, hover inside quotes is identical to the unquoted hover
  • local_find_refs::references_inside_quoted_annotations
  • rename::test_rename_from_inside_quoted_annotation

Commands run:

  • python3 test.py --no-test --no-tensor-shapes --no-conformance --no-jsonschema --no-extension
  • cargo test -p pyrefly --lib
  • cargo test -p pyrefly --test pyrefly_laziness_tests
  • cargo clippy --release

Type-checker behavior is unchanged, so conformance output is unaffected.

Arham-Begani and others added 2 commits October 15, 2026 04:41
Adds tests that show the current behavior for names inside quoted
annotations such as `x: "list[Foo]"`. The checker resolves these names,
but the language server treats the whole annotation as a string: hover
shows only the bare type, go-to-definition finds nothing (and the name in
`list["Foo"]` resolves to `list.__getitem__`), and references and rename
skip occurrences inside strings. This is the bug in meta#4740. The next
commit fixes it.

Also covers strings that must stay plain strings: ordinary string values,
`Literal["..."]` values and malformed annotations.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01769iN16mLYKGKoWiouacg6
Part of meta#4740.

The binder parses forward-reference strings such as `x: "list[Foo]"` and
binds the names inside them at their real source ranges, but the language
server located the symbol under the cursor in the original AST, where the
annotation is a single string literal. Hover, go-to-definition, references
and rename therefore saw a string instead of `Foo`, and `list["Foo"]` even
jumped to `list.__getitem__`.

When the cursor is inside a string the binder parsed as a type expression,
`identifier_at` and `find_definition` now resolve against the parsed
expression, and the local reference scan also looks inside such strings. A
string only counts if a name in its parse has a binding key, so ordinary
strings, `Literal["..."]` values and malformed annotations stay plain
strings. Strings with escapes or implicit concatenation are skipped because
their contents don't map exactly onto the source.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01769iN16mLYKGKoWiouacg6
@meta-cla

meta-cla Bot commented Oct 15, 2026

Copy link
Copy Markdown

Hi @Arham-Begani!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks!

@Arham-Begani Arham-Begani changed the title Lsp quoted annotation identifiers Resolve identifiers inside quoted annotations in the LSP. Oct 15, 2026
@meta-cla

meta-cla Bot commented Oct 15, 2026

Copy link
Copy Markdown

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks!

@meta-cla meta-cla Bot added the cla signed label Oct 15, 2026
@github-actions
github-actions Bot requested a review from maggiemoss October 15, 2026 09:10
@rchen152 rchen152 added size/s and removed size/xl labels Nov 2, 2026
@rchen152
rchen152 requested review from rchen152 and removed request for maggiemoss November 3, 2026 02:31
@rchen152 rchen152 assigned rchen152 and unassigned maggiemoss Nov 3, 2026
@github-actions

github-actions Bot commented Nov 3, 2026

Copy link
Copy Markdown

According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅

@rchen152 rchen152 left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks! This looks pretty good to me. Unfortunately, our PR auto-import hook doesn't run for PRs that start out with the CLA unsigned; could you rebase? That should trigger an import so I can merge.

Since this needs a rebase anyway, here are some suggestions to reduce some minor code duplication (AI-generated, so feel free to push back on anything that doesn't make sense):

  • type_from_expression_at_impl already finds a string literal among the nodes at the cursor, re-parses it with Ast::parse_type_literal (the binder's own helper), and calls locate_node_in. The PR adds a second copy of that pattern in forward_ref_at instead of sharing it.
  • It also adds parse_forward_ref, which calls ruff's parse_type_annotation directly. The "simple only" restriction is a real reason to differ, but it leaves two forward-reference parsers in one file.

This branch has not been deployed

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants