Fix LSP keyword rename on constructors; FailedParse config source#3033
Open
Louisvranderick wants to merge 4 commits intofacebook:mainfrom
Open
Fix LSP keyword rename on constructors; FailedParse config source#3033Louisvranderick wants to merge 4 commits intofacebook:mainfrom
Louisvranderick wants to merge 4 commits intofacebook:mainfrom
Conversation
Keyword arguments at call sites could resolve definitions to the callee when refinement failed, which made prepare_rename advertise a symbol that produced no edits (e.g. dataclass constructor keywords vs the class name). Add optional callee fallback for go-to-definition only, refine constructor keywords against __init__ and positional-only parameters, and return null from prepare_rename when a keyword argument has no definitions. Invalid pyrefly.toml on disk was classified as File after parse failure, which broke discovery that distinguishes real config paths. Introduce ConfigSource::FailedParse, treat it like File for roots and messaging, and record the path when read_path fails. Tests cover dataclass rename, FailedParse config, and definition on pos-only params for invalid keyword calls. Made-with: Cursor
Louisvranderick
force-pushed
the
fix/lsp-dataclass-keyword-rename
branch
from
569e5ae to
238004d
Compare
Scrut config.md tests expected the old WARN line for invalid pyrefly.toml; project mode now logs INFO like a normal config path. Remove duplicate FailedParse patterns in dump-config, loader, and LSP so clippy stays clean and semantics stay consistent (failed parse uses file-style paths, not marker-style messaging). Made-with: Cursor
This reverts commit 350bac2.
test_enum_class_value (issue facebook#1982) expected f(E.X) to match Literal[E.X] after literal member typing. Reverting that feature restores the general E branch; update the assertion accordingly. Made-with: Cursor
Contributor
|
Hey @Louisvranderick, thanks for the hard work on this #2496 fix. I see you’re actively pushing commits and working through the logic as we speak, so I’ll stay tuned while the CI settles! Just as a heads-up: I’m not a maintainer or official contributor yet just a developer following the project closely and looking to provide some extra eyes on the LSP side of things. I did notice the recent revert on the enum literal member types; it’ll be interesting to see how that interacts with the new keyword_argument_fallback_to_callee flow. If that change was to avoid a regression elsewhere, I'm happy to help verify it against some complex TypedDict or Enum edge cases once the branch is stable. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #2496
Renaming a keyword at a call site (e.g. dataclass
Foo(a=...)) could resolve the rename symbol to the class instead of the field parameter, so the client offered a rename that produced no edits. When load/parse failed for an on-diskpyrefly.toml, we still labeled the source asFile, which misclassified config roots.The LSP path adds
keyword_argument_fallback_to_callee(on for go-to-definition, off for rename and find-refs), refines keyword targets against callee signatures (positional-only params and explicit__init__on class calls), and returns null fromprepare_renamewhen a keyword argument has no definitions. Config addsConfigSource::FailedParseand treats it like a real config path for roots, discovery, and messaging.Test Plan
test_failed_parse_on_invalid_tomlinpyrefly_config.python3 test.py --no-test --no-conformance --no-jsonschemaand targetedcargo testfor the new cases locally.