fix: quote marks during autocomplete now take care of the existing quotes #1666

Karman-singh15 · 2025-11-22T18:06:57Z

Summary

Fix code completion to avoid inserting extra quotes when cursor is already inside a string literal.

When typing foo(' and triggering completion for a Literal["apple", "pear"] parameter, the completion previously inserted 'apple' (with quotes), resulting in invalid syntax foo(''apple''). Now it correctly inserts just apple, producing valid code foo('apple').

Fixes completion behavior for typing.Literal string values when the user has already typed an opening quote.

Fixes #1086

Test Plan

Automated Testing

Added new test case [completion_literal_quote_test] in [pyrefly/lib/test/lsp/completion_quotes.rs]
Test verifies that completions insert unquoted text when cursor is inside a string
Run with: cargo test -p pyrefly --lib test::lsp::completion_quotes::completion_literal_quote_test

Manual Testing

Create a Python file with:

from typing import Literal
def foo(fruit: Literal["apple", "pear"]) -> None: ...
foo('

…ng into existing strings and enhance error message normalization.

meta-cla · 2025-11-22T18:07:04Z

Hi @Karman-singh15!

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 [email protected]. Thanks!

meta-cla · 2025-11-22T18:11:20Z

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

stroxler

Thanks for the PR! This looks reasonable to me, but @kinto0 should probably do the final review

meta-codesync · 2025-11-24T17:23:54Z

@kinto0 has imported this pull request. If you are a Meta employee, you can view this in D87793904.

kinto0

thank you! looks like there's a merge conflict to fix up and i left a few other suggestions

kinto0 · 2025-11-24T17:27:06Z

pyrefly/lib/error/expectation.rs

+
+fn normalize_message(s: &str) -> String {
+    s.replace("\\'", "'") // unescape single quotes
+        .replace("\"", "\"") // keep double-quote escapes as-is


is it worth including this line? let's keep the comment but remove the no-op

sure i will remove the line 68 but leave the comment there?
is there a problem with the code tho?

no issue with the code. but replacing something with itself (a no-op) isn't worth doing here imo

yeah that makes sense ofc. i will look into this

kinto0 · 2025-11-24T17:28:55Z

pyrefly/lib/test/lsp/completion_quotes.rs

+}
+
+#[test]
+fn completion_literal_quote_test() {


instead of making this new test file, is there a way to include this test in the normal lsp/completion.rs test?

for example, completion_literal_do_not_duplicate_quotes already exists. I notice that test result didn't change. is there a way to update that test instead?

should i write a new test in the completion.rs or i can just rewrite the completion_literal_do_not_duplicate_quotes test?

if rewriting completion_literal_do_not_duplicate_quotes is easier, go for it!

okay great i will work on this rn

Karman-singh15 · 2025-11-24T19:22:14Z

@kinto0 , i will work on your suggestions but can you please tell what are the merge conflicts and how do i go about fixing that. it would be great help if you could guide me through that

kinto0 · 2025-11-24T19:32:47Z

pyrefly/lib/state/lsp.rs

not sure exactly what the conflicts are but it says there are conflicts in pyrefly/lib/state/lsp.rs. if you pull + merge, it should tell you. let me know if you need more help!

…teral values

Summary: I think `cargo` just got a clippy version bump, I was getting pages of linter errors, almost all of them about elided lifetimes. This silences them. Reviewed By: rchen152 Differential Revision: D87653409 fbshipit-source-id: 0e588fb5363e0244226758ded1965b3f4d8a2333

Summary: We want to get rid of RawClassFieldInitialization - it's unnecessary once we've refactored the code to make more sense. As a result, it's important to avoid depending on it where possible; in this case, let's keep the match in `calculate_class_field` (which is where we need to refactor) and just pass a flag down. Reviewed By: rchen152 Differential Revision: D87653414 fbshipit-source-id: 030b4e353d867f6cd9c5696bc5f80ee9610b0f93

Summary: I'm finally ready to combine the first two match statements of `calculate_class_field`, eliminating the `value` variable and `value_storage` in the process because we can just compute the `value_ty` in one shot. This is a major step along the path to being able to represent the cases of class field more clearly - even thought the new match is a bit more complex, it can now be understood by reading it (in linear time) - we no longer have to understand the interactions between two matches (which is in some sense quadratic) to understand what `value` will be for any given case. Reviewed By: grievejia Differential Revision: D87653413 fbshipit-source-id: 272e6bff7345d832f10e82df19135b7c6f6d7023

Summary: There's no reason to do a separate match downstream - this logic is only relevant to one branch of the `field_definition` match. Reviewed By: grievejia Differential Revision: D87653406 fbshipit-source-id: 8e9d5c2555bb929996052df3b8aed8b5893d0ebb

Summary: I noticed in the previous refactor a surprising check that the method is instance-level when we sanitize out type parameters. This seemed wrong, and indeed it is - see the bug, we're inferring an unbound (out-of-scope) `Type::Quantified` type `T` here, which is nonsense. Reviewed By: grievejia Differential Revision: D87653410 fbshipit-source-id: 0b8bbe0e19447d35671585161e146ed129922ff3

Summary: We want `RawClassFieldInitialization` and `initial_value` to go away; these aren't necessary, they are just tech debt accumulated. As a result, we don't want to have a helper function, because that makes it harder to deduplicate and simplify the match statements. In preparation for coming simplifications, let's just inline. This makes things messier in the short term but will pay off as we keep cleaning things up. Reviewed By: grievejia Differential Revision: D87653407 fbshipit-source-id: 79b14889b71534b70bd591f89eca64939a2f7b28

Summary: This is more straightforward to read, it was only used once. Reviewed By: grievejia Differential Revision: D87653408 fbshipit-source-id: c79c800d29e1268fa4abe6090b5111a47056f3ef

Summary: I'm pretty sure the logic here was not intended - it doesn't really make sense to use magic initialization (which implies class-level attribute access semantics) for any attribute defined in an instance method. Cleaning this up lets us move the magic initialization check to somewhere that makes much more sense to me - it's only relevant to the `Uninitialized` branch. Reviewed By: grievejia Differential Revision: D87653411 fbshipit-source-id: b1493f5af08a241e86e3179cc6c3d124cfe76d55

Summary: I want to pull some logic into a helper, but the `if` clause is doing too much work right now; let's minimize the clause by moving the destructuring into the curly brackets. Reviewed By: grievejia Differential Revision: D87653416 fbshipit-source-id: 652ef6c791c8e565b22940c6ac438d6665929643

Summary: It's easier to skim the initialization block if we give this logic a nice name and move it out of the big match. Reviewed By: grievejia Differential Revision: D87653412 fbshipit-source-id: 8f24e532523accb3aa70e484630aa948a34fff62

Summary: This turns JSON schema run Rust structs Differential Revision: D87559335 fbshipit-source-id: c13ca92bbfc5ce8ddf4999fe7d10b97513b8a6a1

Summary: Unpacking a `tuple[*Ts]` hits some fallback code and turns the contents into `object` We fix this by introducing an `ElementOf[Ts]` type, which is similar to `Type::QuantifiedValue` for TypeVar but it represents an element of the TypeVarTuple, instead of the whole thing. So when we try to iterate some `tuple[*Ts]`, we produce an unspecified number of `ElementOf[Ts]`, and when we try to make an unbounded tuple with `ElementOf[Ts]` contents we actually create `tuple[*Ts]`. There are additional guards against unpacking the same type var tuple twice in a tuple expression or function call, since the lengths will not match up unless the typevartuple resolves to empty tuple. fixes facebook#1268 Reviewed By: stroxler Differential Revision: D87694394 fbshipit-source-id: 19be3f7282d6e4dfec13e0df5523e2ade658e63a

Karman-singh15 · 2025-11-24T20:53:29Z

hi i think i made some mistake while trying to resolve the conflict, but it seems fine to me now. sorry for the mess of commits i made, still new to this.
can you please check once
sorry for the inconvenience

Karman-singh15

implemented the suggestions

kinto0 · 2025-11-24T22:50:49Z

looks great now! thank you for working on this

pyrefly/lib/state/lsp.rs

kinto0 · 2025-11-24T22:57:10Z

pyrefly/lib/test/lsp/completion.rs

+
+    // We expect the completion to NOT insert extra quotes if we are already in a quote.
+    // Currently it likely inserts quotes.
+    println!("{}", report);


nit: is there a reason we can't keep the test how it used to be?

no actually this test was written for the specific issue occurring in the sandbox link i think we can use the previous one. should i change it?

if the previous one describes the same problem (I think it does) we should keep it that way to make this PR smaller / easier to review.

but if you want to use the exact example from the sandbox (perfectly good too), I would suggest writing it similar to the existing assert_eq(the entire result.trim(), report.trim()). this entire result makes it much easier to read at a glance and it's already formatted to exclude useless information

pyrefly/lib/test/lsp/mod.rs

Karman-singh15 · 2025-11-25T07:38:01Z

only changing the test is left you can tell me if you want the previous test, i will add that tonight rest changes i have made you can take a look

Karman-singh15 · 2025-11-27T10:45:28Z

hi, sorry for the delay i have reverted the test to the previous test. you can look and tell me if there is any issues

Summary: This is an alternative to D84849556, which aims to solve facebook#1219 Instead of introducing a new type variant, this diff introduces a new field in Type::Union to hold an optional display name. It will be populated when resolving a type alias. Compared to the other approach: benefits - less plumbing - type checking behavior unaffected drawbacks - only applies to type aliases of unions (though arguably this is where readability matters most) - name information is not preserved during union flattening (so if we union a type alias with something else, the result is the full expanded type) Reviewed By: rchen152 Differential Revision: D87558541 fbshipit-source-id: d032c7467a0e7b02a2b20074424113472cc78a74

Summary: X-link: meta-pytorch/monarch#1997 Need to pull in a ruff parser change to fix facebook#1559 This requires a bump for the `inventory` crate, since ruff requires 0.3.20/0.3.21. That version is semver compatible with 0.3.8, so we can't keep both versions around. Reviewed By: dtolnay Differential Revision: D87817697 fbshipit-source-id: 7bb25dc7853161b946283288e40a06dd8d067119

…ng into existing strings and enhance error message normalization.

…teral values

Summary: I think `cargo` just got a clippy version bump, I was getting pages of linter errors, almost all of them about elided lifetimes. This silences them. Reviewed By: rchen152 Differential Revision: D87653409 fbshipit-source-id: 0e588fb5363e0244226758ded1965b3f4d8a2333

Summary: We want to get rid of RawClassFieldInitialization - it's unnecessary once we've refactored the code to make more sense. As a result, it's important to avoid depending on it where possible; in this case, let's keep the match in `calculate_class_field` (which is where we need to refactor) and just pass a flag down. Reviewed By: rchen152 Differential Revision: D87653414 fbshipit-source-id: 030b4e353d867f6cd9c5696bc5f80ee9610b0f93

Summary: There's no reason to do a separate match downstream - this logic is only relevant to one branch of the `field_definition` match. Reviewed By: grievejia Differential Revision: D87653406 fbshipit-source-id: 8e9d5c2555bb929996052df3b8aed8b5893d0ebb

Summary: We want `RawClassFieldInitialization` and `initial_value` to go away; these aren't necessary, they are just tech debt accumulated. As a result, we don't want to have a helper function, because that makes it harder to deduplicate and simplify the match statements. In preparation for coming simplifications, let's just inline. This makes things messier in the short term but will pay off as we keep cleaning things up. Reviewed By: grievejia Differential Revision: D87653407 fbshipit-source-id: 79b14889b71534b70bd591f89eca64939a2f7b28

Summary: Unpacking a `tuple[*Ts]` hits some fallback code and turns the contents into `object` We fix this by introducing an `ElementOf[Ts]` type, which is similar to `Type::QuantifiedValue` for TypeVar but it represents an element of the TypeVarTuple, instead of the whole thing. So when we try to iterate some `tuple[*Ts]`, we produce an unspecified number of `ElementOf[Ts]`, and when we try to make an unbounded tuple with `ElementOf[Ts]` contents we actually create `tuple[*Ts]`. There are additional guards against unpacking the same type var tuple twice in a tuple expression or function call, since the lengths will not match up unless the typevartuple resolves to empty tuple. fixes facebook#1268 Reviewed By: stroxler Differential Revision: D87694394 fbshipit-source-id: 19be3f7282d6e4dfec13e0df5523e2ade658e63a

Summary: This stack introduces a new struct called OutputWithLocations that will be used along with fmt_helper_generic in order to make certain parts of inlay hints clickable. The idea here is that now instead of writing parts of the type directly to a formatter, there is a generic TypeOutput that will be taken in can handle the collection of parts itself. In this case, we will be using an OutputWithLocations in order to collect the parts of the type and also get the location of the types definition if it is relevant. The rest of this stack will handle the actual implementation of this logic. There are a few things that currently are known limitations that will be added as follow ups: 1. Currently tuple types will not be clickable. For example if you have `tuple[int]`, the `int` portion of the type will be clickable but the `tuple` portion will not be. A test has been added that covers this case. 2. Types coming from typing.py will not be clickable. For example, the type `Literal[1]` will not be clickable. This is something that will be addressed in a followup. A test has been added which covers this case. Introduces a new OutputWithLocations struct that implements TypeOutput to capture formatted type strings along with their source code locations. This struct collects type parts as a vector of (String, Option<TextRangeWithModule>) tuples, enabling clickable type references in IDE features. Also exposes fmt_helper_generic as public to support the new location-aware formatting infrastructure. Adds the OutputWithLocations struct that implements the TypeOutput trait. This struct stores type parts as a vector of (String, Option<TextRangeWithModule>) tuples, allowing us to capture both the formatted type string and its source location. The struct provides a new() constructor that takes a TypeDisplayContext and a parts() getter to access the collected parts. This is the first step in enabling clickable type references in IDE features. Reviewed By: ndmitchell Differential Revision: D87708533 fbshipit-source-id: 09d9c705974ff373a291d43880ffa752c198825b

Reviewed By: javabster Differential Revision: D87754773 fbshipit-source-id: 8c0f58e5b9e40be3916a4c74137a9ab4faf37415

…ok#1588 (facebook#1629) Summary: fix facebook#1588 Added a docstring parsing pipeline that normalizes raw literals, extracts `multi-line :param …:` and `Args: sections`, and exposes the results via parse_parameter_documentation, plus unit tests covering both styles. This provides the richer parameter metadata requested in the issue. An extended signature helps to capture the callee range, look up the associated docstring, and parse parameter descriptions and surface them through ParameterInformation documentation, so hover/signature hints show the full multi-line text Added an integration test to ensure signature helps surface the parsed docs and kept the helper coverage in sync Pull Request resolved: facebook#1629 Reviewed By: yangdanny97 Differential Revision: D87673056 Pulled By: kinto0 fbshipit-source-id: 6bea24261d9c27647b629272995d76ecdd176cf8

Summary: This diff introduces the write_type function which is responsible for writing types fall outside of str, qname, targs, etc. This will defer the type back to fmt_helper_generic and use the existing match statement to figure out how to handle this type. Reviewed By: kinto0 Differential Revision: D87642081 fbshipit-source-id: a9846af0aad7b9af7dfd9ffe57fcf0dd811a674a

Summary: fix facebook#1670 an alternative approach could be to use find-references to find unused variables. but if we ever want this diagnostic on the CLI (I think we do), we will likely want it less coupled to the language server. Reviewed By: stroxler Differential Revision: D87785750 fbshipit-source-id: 8f2abc30693714eb5df7523e36eb70c4f6eacd3e

Summary: we should never warn on unused star imports Reviewed By: stroxler Differential Revision: D87788170 fbshipit-source-id: 61c3d914b674d309a44f2f03d3df9e6d628b78bd

Summary: Refactors union type formatting to output individual type components and separators as distinct parts, enabling location tracking for each type in the union. Literal types within unions are still combined into a single "Literal[a, b, c]" for better readability. The refactored code uses the new fmt_type_sequence helper for non-literal union members and manually formats the combined literal, calling write_str and write_type methods on the TypeOutput trait instead of building intermediate strings. This fixes the test_output_with_locations_union_type_splits_properly test. Reviewed By: kinto0 Differential Revision: D87642077 fbshipit-source-id: c7b6f49973be8c25adcddfd663c6a111c9b35cce

Summary: Currently if you have a type like `tuple[int]`, the int portion of the type will have a location but the `tuple` portion does not. This diff adds a test to cover this case and will be updated once that functionality is implemented. Also updates the formatting for tuples so that `tuple` and `[` are separate parts rather than just a single `tuple[` part. Reviewed By: kinto0 Differential Revision: D87642082 fbshipit-source-id: dcd0e44785a87ba608ef2431fb0c8a29cae0a816

Summary: dogscience_hotfix Fixes facebook#1669 Reviewed By: rchen152 Differential Revision: D87801231 fbshipit-source-id: cccdac346683fc91d18954d32755ac88e7b72dbb

Summary: see title Reviewed By: yangdanny97 Differential Revision: D87788532 fbshipit-source-id: b76375ffdacaa2a661c834aca3b6facf10cc71b1

Karman-singh15 added 3 commits November 21, 2025 23:18

Change string formatting in Display implementation

a2fdc89

feat: Improve LSP literal completions by removing quotes when inserti…

26e462d

…ng into existing strings and enhance error message normalization.

Merge branch 'facebook:main' into main

bc16325

Karman-singh15 changed the title ~~quote marks during autocomplete now take care of the existing quotes~~ fix: \quote marks during autocomplete now take care of the existing quotes Nov 22, 2025

meta-cla bot added the cla signed label Nov 22, 2025

Karman-singh15 changed the title ~~fix: \quote marks during autocomplete now take care of the existing quotes~~ fix: quote marks during autocomplete now take care of the existing quotes Nov 22, 2025

Karman-singh15 mentioned this pull request Nov 22, 2025

Literal autocomplete should take care of existing quote marks #1086

Open

stroxler reviewed Nov 23, 2025

View reviewed changes

stroxler requested a review from kinto0 November 23, 2025 16:11

kinto0 requested changes Nov 24, 2025

View reviewed changes

kinto0 self-assigned this Nov 24, 2025

Karman-singh15 and others added 14 commits November 25, 2025 02:02

test: add 'inserting' field to expected LSP completion results for li…

44c9c09

…teral values

combined the quotes test in completion.rs and remove a no-op

61847f4

Inline magic initialization check into the match

309300c

Summary: This is more straightforward to read, it was only used once. Reviewed By: grievejia Differential Revision: D87653408 fbshipit-source-id: c79c800d29e1268fa4abe6090b5111a47056f3ef

Vendor typify

f04479a

Summary: This turns JSON schema run Rust structs Differential Revision: D87559335 fbshipit-source-id: c13ca92bbfc5ce8ddf4999fe7d10b97513b8a6a1

Karman-singh15 commented Nov 24, 2025

View reviewed changes

kinto0 approved these changes Nov 24, 2025

View reviewed changes

kinto0 requested changes Nov 24, 2025

View reviewed changes

Karman-singh15 added 2 commits November 25, 2025 12:20

Merge branch 'facebook:main' into main

b2fe8ee

added missing call

64a4af2

Karman-singh15 added 2 commits November 27, 2025 09:35

updated the duplicate quote test

a4f50d4

Merge branch 'main' into main

1851ee7

yangdanny97 and others added 19 commits November 27, 2025 16:17

feat: Improve LSP literal completions by removing quotes when inserti…

acc00ad

…ng into existing strings and enhance error message normalization.

test: add 'inserting' field to expected LSP completion results for li…

f00a366

…teral values

Update pyrefly version]

6e7d7cb

Reviewed By: javabster Differential Revision: D87754773 fbshipit-source-id: 8c0f58e5b9e40be3916a4c74137a9ab4faf37415

filter star imports from unused detection

ef09f28

Summary: we should never warn on unused star imports Reviewed By: stroxler Differential Revision: D87788170 fbshipit-source-id: 61c3d914b674d309a44f2f03d3df9e6d628b78bd

Fix crashes when yield or yield from appears in type annotations

6e4267d

Summary: dogscience_hotfix Fixes facebook#1669 Reviewed By: rchen152 Differential Revision: D87801231 fbshipit-source-id: cccdac346683fc91d18954d32755ac88e7b72dbb

add tests for last diff

02b24ba

Summary: see title Reviewed By: yangdanny97 Differential Revision: D87788532 fbshipit-source-id: b76375ffdacaa2a661c834aca3b6facf10cc71b1

fix: quote marks during autocomplete now take care of the existing quotes #1666

Are you sure you want to change the base?

fix: quote marks during autocomplete now take care of the existing quotes #1666

Conversation

Karman-singh15 commented Nov 22, 2025

Summary

Test Plan

Automated Testing

Manual Testing

Uh oh!

meta-cla bot commented Nov 22, 2025

Action Required

Process

Uh oh!

meta-cla bot commented Nov 22, 2025

Uh oh!

stroxler left a comment

Choose a reason for hiding this comment

Uh oh!

meta-codesync bot commented Nov 24, 2025

Uh oh!

kinto0 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Karman-singh15 commented Nov 24, 2025

Uh oh!

kinto0 commented Nov 24, 2025

Uh oh!

Karman-singh15 commented Nov 24, 2025

Uh oh!

Karman-singh15 left a comment

Choose a reason for hiding this comment

Uh oh!

kinto0 commented Nov 24, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kinto0 Nov 25, 2025 • edited 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!

Karman-singh15 commented Nov 25, 2025

Uh oh!

Karman-singh15 commented Nov 27, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

kinto0 Nov 25, 2025 •

edited

Loading