Skip to content

fix type source not show on field #3898#3937

Open
asukaminato0721 wants to merge 2 commits into
facebook:mainfrom
asukaminato0721:3898
Open

fix type source not show on field #3898#3937
asukaminato0721 wants to merge 2 commits into
facebook:mainfrom
asukaminato0721:3898

Conversation

@asukaminato0721

Copy link
Copy Markdown
Contributor

Summary

Fixes #3898

Attribute hovers like c.x collect narrowing source information from the containing facet expression’s base binding.

Attribute hovers return only narrowing sources, avoiding unrelated first-use info from the base variable.

Test Plan

add test

@github-actions

This comment has been minimized.

@NathanTempest NathanTempest 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 @asukaminato0721 looks like the core fix is right, attribute hovers should surface the base variable's flow-binding narrowing (and skip its unrelated first-use), and the test captures #3898 well.

Two things before import:

  • The diff looks like it also changes the ExprContext::Store arm from Key::Definition to Key::BoundName. If that's intended, could you explain why and add a store-context test? It changes non-attribute hovers and risks regressing them (a store-only identifier may have no valid BoundName, which would drop the type source entirely). If it's unintentional, let's revert that line and keep this PR scoped to the field-hover fix.
  • IdentifierContext::Attribute already carries base_range, so the get_ast + locate_node + find_map + attr.clone() dance to recover the base identifier looks avoidable. Per our style guide we try to avoid re-walking the AST / cloning Expr nodes. Can the base identifier come from base_range (or be captured when the Attribute context is constructed)?

Otherwise the approach and test look good. Thanks!

@github-actions

Copy link
Copy Markdown

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

@NathanTempest NathanTempest 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 @asukaminato0721 for addressing my follow-ups, the base_identifier field is exactly the right move and extracting the base name when the Attribute context is built (instead of re-walking the AST and cloning the Expr at hover time) is much cleaner and matches our style guide.

One thing still looks off. The ExprContext::Store arm from Key::Definition to Key::BoundName. That's unrelated to the field-hover fix and changes plain assignment-target hovers, it's risky since BoundName may not be a valid key at a definition site, which would drop the type source entirely (and there's no store-context test). Could you confirm? If it's intentional, let's add a store-context test and call it out; if not, revert that one line so the PR stays scoped. Everything else looks ready to import.

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.

type source not show on field

2 participants