add version support to textDocument/publishDiagnostics #1382

dearfl · 2025-10-25T03:07:57Z

close #794

PS: I know little about LSP or this project

kinto0

Thanks for contributing! A few tests for this behavior would make us confident it works really well.

Could you add some tests in the lsp_interaction folder where each version for a diagnostics response is correct?

Thanks!!

kinto0 · 2025-10-27T23:30:26Z

pyrefly/lib/lsp/non_wasm/server.rs

            }
-            self.connection.publish_diagnostics(diags);
+            self.connection
+                .publish_diagnostics(diags, &*self.version_info.lock());


nit: by aqcuiring the lock here, we won't be able to make updates to the version info. maybe we should clone it instead?

cloning the hashtable might be kind of heavy too? I prefer lock. let me know if otherwise.

publish_diagnostics_for_uri takes an i32 so maybe we can change the API to only call these functions with that?

I'm not sure I follow you, publish_diagnostics_for_uri takes an Option<i32> as version, also the change API part.

pyrefly/lib/lsp/non_wasm/server.rs

dearfl · 2025-10-28T07:54:03Z

Thanks for contributing! A few tests for this behavior would make us confident it works really well.

Could you add some tests in the lsp_interaction folder where each version for a diagnostics response is correct?

Thanks!!

I added a small test for this, however I can't figure out why it won't work, might need a little help here. : (
It seems Server::did_change happens after Interation::Client::expect_message? I don't know why, is there a way to synchronize? Or maybe I'm using it wrong?

kinto0

could you paste the test failure? it should show the messages back/forth.

kinto0 · 2025-10-28T16:13:15Z

pyrefly/lib/lsp/non_wasm/server.rs

            }
-            self.connection.publish_diagnostics(diags);
+            self.connection
+                .publish_diagnostics(diags, &*self.version_info.lock());


publish_diagnostics_for_uri takes an i32 so maybe we can change the API to only call these functions with that?

pyrefly/lib/test/lsp/lsp_interaction/diagnostic.rs

dearfl · 2025-10-28T16:31:03Z

could you paste the test failure? it should show the messages back/forth.

running 1 test
client--->server {"id":1,"method":"initialize","params":{"capabilities":{"textDocument":{"publishDiagnostics":{"codeDescriptionSupport":true,"dataSupport":true,"relatedInformation":true,"tagSupport":{"valueSet":[1,2]},"versionSupport":true}},"workspace":{"configuration":true}},"clientInfo":{"name":"debug"},"processId":64851,"rootPath":"/","trace":"verbose"}}
client<---server {"id":1,"result":{"capabilities":{"codeActionProvider":{"codeActionKinds":["quickfix"]},"completionProvider":{"triggerCharacters":["."]},"definitionProvider":true,"documentHighlightProvider":true,"documentSymbolProvider":true,"hoverProvider":true,"inlayHintProvider":true,"positionEncoding":"utf-16","signatureHelpProvider":{"triggerCharacters":["(",","]},"textDocumentSync":2,"typeDefinitionProvider":true,"workspace":{"fileOperations":{"willRename":{"filters":[{"pattern":{"glob":"**/*.{py,pyi}","matches":"file"},"scheme":"file"}]}},"workspaceFolders":{"changeNotifications":true,"supported":true}},"workspaceSymbolProvider":true},"serverInfo":{"name":"pyrefly-lsp","version":"pyrefly-lsp-test-version"}}}
client--->server {"method":"initialized","params":{}}
Reading messages
DEBUG Running with 3 threads (5 MiB stack size)
client<---server {"id":1,"method":"workspace/configuration","params":{"items":[{"section":"python"}]}}
client--->server {"id":1}
client--->server {"method":"textDocument/didOpen","params":{"textDocument":{"languageId":"python","text":"# Copyright (c) Meta Platforms, Inc. and affiliates.\n#\n# This source code is licensed under the MIT license found in the\n# LICENSE file in the root directory of this source tree.\n\nprint(\"Did you catch the change?\")\n","uri":"file:///tmp/pyrefly_lsp_testiT0Ha4/text_document.py","version":1}}}
client--->server {"id":2,"method":"textDocument/diagnostic","params":{"textDocument":{"uri":"file:///tmp/pyrefly_lsp_testiT0Ha4/text_document.py"}}}
 WARN While finding Python interpreter: Python environment (version, platform, or site-package-path) has value unset, but no Python interpreter could be found to query for values. Falling back to Pyrefly defaults for missing values.
DEBUG Running epoch 1 of run 0
 WARN While finding Python interpreter: Python environment (version, platform, or site-package-path) has value unset, but no Python interpreter could be found to query for values. Falling back to Pyrefly defaults for missing values.
Handling non-canceled request textDocument/diagnostic (2)
client<---server {"method":"textDocument/publishDiagnostics","params":{"diagnostics":[],"uri":"file:///tmp/pyrefly_lsp_testiT0Ha4/text_document.py","version":1}}
client--->server {"method":"textDocument/didChange","params":{"contentChanges":[{"text":"# test"}],"textDocument":{"languageId":"python","uri":"file:///tmp/pyrefly_lsp_testiT0Ha4/text_document.py","version":2}}}
client--->server {"id":3,"method":"textDocument/diagnostic","params":{"textDocument":{"uri":"file:///tmp/pyrefly_lsp_testiT0Ha4/text_document.py"}}}
DEBUG Running epoch 1 of run 1
DEBUG Committing transaction
client<---server {"method":"textDocument/publishDiagnostics","params":{"diagnostics":[],"uri":"file:///tmp/pyrefly_lsp_testiT0Ha4/text_document.py","version":1}}

thread 'test::lsp::lsp_interaction::diagnostic::test_version_support_publish_diagnostics' panicked at pyrefly/lib/test/lsp/lsp_interaction/object_model.rs:531:17:
assertion failed: `(left == right)`: Response mismatch

Diff < left / right > :
<{"method":"textDocument/publishDiagnostics","params":{"diagnostics":[],"uri":"file:///tmp/pyrefly_lsp_testiT0Ha4/text_document.py","version":2}}
>{"method":"textDocument/publishDiagnostics","params":{"diagnostics":[],"uri":"file:///tmp/pyrefly_lsp_testiT0Ha4/text_document.py","version":1}}


note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
Connection closed.
DEBUG Running epoch 1 of run 2
test test::lsp::lsp_interaction::diagnostic::test_version_support_publish_diagnostics ... FAILED

failures:

failures:
    test::lsp::lsp_interaction::diagnostic::test_version_support_publish_diagnostics

test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 2554 filtered out; finished in 0.21s

error: test failed, to rerun pass `-p pyrefly --lib`

kinto0 · 2025-10-29T20:13:58Z

could you paste the test failure? it should show the messages back/forth.

running 1 test
client--->server {"id":1,"method":"initialize","params":{"capabilities":{"textDocument":{"publishDiagnostics":{"codeDescriptionSupport":true,"dataSupport":true,"relatedInformation":true,"tagSupport":{"valueSet":[1,2]},"versionSupport":true}},"workspace":{"configuration":true}},"clientInfo":{"name":"debug"},"processId":64851,"rootPath":"/","trace":"verbose"}}
client<---server {"id":1,"result":{"capabilities":{"codeActionProvider":{"codeActionKinds":["quickfix"]},"completionProvider":{"triggerCharacters":["."]},"definitionProvider":true,"documentHighlightProvider":true,"documentSymbolProvider":true,"hoverProvider":true,"inlayHintProvider":true,"positionEncoding":"utf-16","signatureHelpProvider":{"triggerCharacters":["(",","]},"textDocumentSync":2,"typeDefinitionProvider":true,"workspace":{"fileOperations":{"willRename":{"filters":[{"pattern":{"glob":"**/*.{py,pyi}","matches":"file"},"scheme":"file"}]}},"workspaceFolders":{"changeNotifications":true,"supported":true}},"workspaceSymbolProvider":true},"serverInfo":{"name":"pyrefly-lsp","version":"pyrefly-lsp-test-version"}}}
client--->server {"method":"initialized","params":{}}
Reading messages
DEBUG Running with 3 threads (5 MiB stack size)
client<---server {"id":1,"method":"workspace/configuration","params":{"items":[{"section":"python"}]}}
client--->server {"id":1}
client--->server {"method":"textDocument/didOpen","params":{"textDocument":{"languageId":"python","text":"# Copyright (c) Meta Platforms, Inc. and affiliates.\n#\n# This source code is licensed under the MIT license found in the\n# LICENSE file in the root directory of this source tree.\n\nprint(\"Did you catch the change?\")\n","uri":"file:///tmp/pyrefly_lsp_testiT0Ha4/text_document.py","version":1}}}
client--->server {"id":2,"method":"textDocument/diagnostic","params":{"textDocument":{"uri":"file:///tmp/pyrefly_lsp_testiT0Ha4/text_document.py"}}}
 WARN While finding Python interpreter: Python environment (version, platform, or site-package-path) has value unset, but no Python interpreter could be found to query for values. Falling back to Pyrefly defaults for missing values.
DEBUG Running epoch 1 of run 0
 WARN While finding Python interpreter: Python environment (version, platform, or site-package-path) has value unset, but no Python interpreter could be found to query for values. Falling back to Pyrefly defaults for missing values.
Handling non-canceled request textDocument/diagnostic (2)
client<---server {"method":"textDocument/publishDiagnostics","params":{"diagnostics":[],"uri":"file:///tmp/pyrefly_lsp_testiT0Ha4/text_document.py","version":1}}
client--->server {"method":"textDocument/didChange","params":{"contentChanges":[{"text":"# test"}],"textDocument":{"languageId":"python","uri":"file:///tmp/pyrefly_lsp_testiT0Ha4/text_document.py","version":2}}}
client--->server {"id":3,"method":"textDocument/diagnostic","params":{"textDocument":{"uri":"file:///tmp/pyrefly_lsp_testiT0Ha4/text_document.py"}}}
DEBUG Running epoch 1 of run 1
DEBUG Committing transaction
client<---server {"method":"textDocument/publishDiagnostics","params":{"diagnostics":[],"uri":"file:///tmp/pyrefly_lsp_testiT0Ha4/text_document.py","version":1}}

thread 'test::lsp::lsp_interaction::diagnostic::test_version_support_publish_diagnostics' panicked at pyrefly/lib/test/lsp/lsp_interaction/object_model.rs:531:17:
assertion failed: `(left == right)`: Response mismatch

Diff < left / right > :
<{"method":"textDocument/publishDiagnostics","params":{"diagnostics":[],"uri":"file:///tmp/pyrefly_lsp_testiT0Ha4/text_document.py","version":2}}
>{"method":"textDocument/publishDiagnostics","params":{"diagnostics":[],"uri":"file:///tmp/pyrefly_lsp_testiT0Ha4/text_document.py","version":1}}


note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
Connection closed.
DEBUG Running epoch 1 of run 2
test test::lsp::lsp_interaction::diagnostic::test_version_support_publish_diagnostics ... FAILED

failures:

failures:
    test::lsp::lsp_interaction::diagnostic::test_version_support_publish_diagnostics

test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 2554 filtered out; finished in 0.21s

error: test failed, to rerun pass `-p pyrefly --lib`

hmm that test looks like the behavior you want. like the other comments say, you're looking for the publishDiagnostic notification, not the textDocument/diagnostic response. the first one happens because of the type error then the next one happens because of the didChange. you can remove the document diagnostic requests and make a similar method to expect_publish_diagnostics_error_count but with a version attached. this'll let you test the behavior correctly!

dearfl · 2025-10-30T17:11:02Z

could you paste the test failure? it should show the messages back/forth.

running 1 test
client--->server {"id":1,"method":"initialize","params":{"capabilities":{"textDocument":{"publishDiagnostics":{"codeDescriptionSupport":true,"dataSupport":true,"relatedInformation":true,"tagSupport":{"valueSet":[1,2]},"versionSupport":true}},"workspace":{"configuration":true}},"clientInfo":{"name":"debug"},"processId":64851,"rootPath":"/","trace":"verbose"}}
client<---server {"id":1,"result":{"capabilities":{"codeActionProvider":{"codeActionKinds":["quickfix"]},"completionProvider":{"triggerCharacters":["."]},"definitionProvider":true,"documentHighlightProvider":true,"documentSymbolProvider":true,"hoverProvider":true,"inlayHintProvider":true,"positionEncoding":"utf-16","signatureHelpProvider":{"triggerCharacters":["(",","]},"textDocumentSync":2,"typeDefinitionProvider":true,"workspace":{"fileOperations":{"willRename":{"filters":[{"pattern":{"glob":"**/*.{py,pyi}","matches":"file"},"scheme":"file"}]}},"workspaceFolders":{"changeNotifications":true,"supported":true}},"workspaceSymbolProvider":true},"serverInfo":{"name":"pyrefly-lsp","version":"pyrefly-lsp-test-version"}}}
client--->server {"method":"initialized","params":{}}
Reading messages
DEBUG Running with 3 threads (5 MiB stack size)
client<---server {"id":1,"method":"workspace/configuration","params":{"items":[{"section":"python"}]}}
client--->server {"id":1}
client--->server {"method":"textDocument/didOpen","params":{"textDocument":{"languageId":"python","text":"# Copyright (c) Meta Platforms, Inc. and affiliates.\n#\n# This source code is licensed under the MIT license found in the\n# LICENSE file in the root directory of this source tree.\n\nprint(\"Did you catch the change?\")\n","uri":"file:///tmp/pyrefly_lsp_testiT0Ha4/text_document.py","version":1}}}
client--->server {"id":2,"method":"textDocument/diagnostic","params":{"textDocument":{"uri":"file:///tmp/pyrefly_lsp_testiT0Ha4/text_document.py"}}}
 WARN While finding Python interpreter: Python environment (version, platform, or site-package-path) has value unset, but no Python interpreter could be found to query for values. Falling back to Pyrefly defaults for missing values.
DEBUG Running epoch 1 of run 0
 WARN While finding Python interpreter: Python environment (version, platform, or site-package-path) has value unset, but no Python interpreter could be found to query for values. Falling back to Pyrefly defaults for missing values.
Handling non-canceled request textDocument/diagnostic (2)
client<---server {"method":"textDocument/publishDiagnostics","params":{"diagnostics":[],"uri":"file:///tmp/pyrefly_lsp_testiT0Ha4/text_document.py","version":1}}
client--->server {"method":"textDocument/didChange","params":{"contentChanges":[{"text":"# test"}],"textDocument":{"languageId":"python","uri":"file:///tmp/pyrefly_lsp_testiT0Ha4/text_document.py","version":2}}}
client--->server {"id":3,"method":"textDocument/diagnostic","params":{"textDocument":{"uri":"file:///tmp/pyrefly_lsp_testiT0Ha4/text_document.py"}}}
DEBUG Running epoch 1 of run 1
DEBUG Committing transaction
client<---server {"method":"textDocument/publishDiagnostics","params":{"diagnostics":[],"uri":"file:///tmp/pyrefly_lsp_testiT0Ha4/text_document.py","version":1}}

thread 'test::lsp::lsp_interaction::diagnostic::test_version_support_publish_diagnostics' panicked at pyrefly/lib/test/lsp/lsp_interaction/object_model.rs:531:17:
assertion failed: `(left == right)`: Response mismatch

Diff < left / right > :
<{"method":"textDocument/publishDiagnostics","params":{"diagnostics":[],"uri":"file:///tmp/pyrefly_lsp_testiT0Ha4/text_document.py","version":2}}
>{"method":"textDocument/publishDiagnostics","params":{"diagnostics":[],"uri":"file:///tmp/pyrefly_lsp_testiT0Ha4/text_document.py","version":1}}


note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
Connection closed.
DEBUG Running epoch 1 of run 2
test test::lsp::lsp_interaction::diagnostic::test_version_support_publish_diagnostics ... FAILED

failures:

failures:
    test::lsp::lsp_interaction::diagnostic::test_version_support_publish_diagnostics

test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 2554 filtered out; finished in 0.21s

error: test failed, to rerun pass `-p pyrefly --lib`

hmm that test looks like the behavior you want. like the other comments say, you're looking for the publishDiagnostic notification, not the textDocument/diagnostic response. the first one happens because of the type error then the next one happens because of the didChange. you can remove the document diagnostic requests and make a similar method to expect_publish_diagnostics_error_count but with a version attached. this'll let you test the behavior correctly!

Now the test always fails with Timeout error, idk why.

kinto0 · 2025-11-20T22:33:42Z

bumping some old PRs: if you still want a review on this, please re-request review

pyrefly/lib/lsp/non_wasm/server.rs

pyrefly/lib/test/lsp/lsp_interaction/diagnostic.rs

kinto0 · 2025-12-04T01:07:43Z

pyrefly/lib/test/lsp/lsp_interaction/diagnostic.rs

+    interaction.initialize(InitializeSettings {
+        configuration: Some(None),
+        capabilities: Some(serde_json::json!({
+            "textDocument": {
+                "publishDiagnostics": {
+                    "versionSupport": true,
+                },
+            },
+        })),
+        ..Default::default()
+    });


any reason not to keep these default?

Suggested change

interaction.initialize(InitializeSettings {

configuration: Some(None),

capabilities: Some(serde_json::json!({

"textDocument": {

"publishDiagnostics": {

"versionSupport": true,

},

},

})),

..Default::default()

});

interaction.initialize(InitializeSettings::Default)

any reason not to keep these default?

Sorry I'm not familiar with this, is version support enabled by default?

kinto0 · 2025-12-04T01:09:07Z

pyrefly/lib/test/lsp/lsp_interaction/diagnostic.rs

+        move |msg: &Message| {
+            let Message::Notification(Notification { method, params }) = msg else {
+                return ValidationResult::Skip;
+            };
+            let Some(uri_val) = params.get("uri") else {
+                return ValidationResult::Skip;
+            };
+            let Some(expected_uri) = uri_val.as_str() else {
+                return ValidationResult::Skip;
+            };
+            if expected_uri == actual_uri && method == "textDocument/publishDiagnostics" {
+                if let Some(actual_version) = params.get("version") {
+                    if let Some(actual_version) = actual_version.as_i64() {
+                        assert!(
+                            actual_version <= expected_version,
+                            "expected version: {}, actual version: {}",
+                            expected_version,
+                            actual_version
+                        );
+                        return match actual_version.cmp(&expected_version) {
+                            std::cmp::Ordering::Less => ValidationResult::Skip,
+                            std::cmp::Ordering::Equal => ValidationResult::Pass,
+                            std::cmp::Ordering::Greater => ValidationResult::Fail,
+                        };
+                    }
+                }
+            }
+            ValidationResult::Skip
+        }
+    };


nit: can this be a match sothe skips are combined?

nit: can this be a match sothe skips are combined?

it either this or nested if let I think.

kinto0 · 2025-12-04T01:09:40Z

pyrefly/lib/test/lsp/lsp_interaction/diagnostic.rs

+                        );
+                        return match actual_version.cmp(&expected_version) {
+                            std::cmp::Ordering::Less => ValidationResult::Skip,
+                            std::cmp::Ordering::Equal => ValidationResult::Pass,


is this expected?

is this expected?

I think we should skip lower version, right? I'm not sure I understand this correctly though.

kinto0 · 2025-12-04T01:09:55Z

pyrefly/lib/test/lsp/lsp_interaction/diagnostic.rs

+        ..Default::default()
+    });
+
+    let gen_validator = |expected_version: i64| {


what does gen mean here? could we name this better?

what does gen mean here? could we name this better?

generate, is create_version_validator a better name?

pyrefly/lib/test/lsp/lsp_interaction/diagnostic.rs

Co-authored-by: Kyle Into <kyleatyahoo@gmail.com>

meta-cla bot added the cla signed label Oct 25, 2025

github-actions bot added the needs-triage label Oct 27, 2025

rchen152 requested a review from kinto0 October 27, 2025 22:46

rchen152 removed the needs-triage label Oct 27, 2025

kinto0 requested changes Oct 27, 2025

View reviewed changes

dearfl force-pushed the version-support-publish-diagnostics branch from 7028101 to 77eb1c0 Compare October 28, 2025 07:49

dearfl force-pushed the version-support-publish-diagnostics branch 2 times, most recently from 50c3750 to 9313fb2 Compare October 28, 2025 08:26

kinto0 requested changes Oct 28, 2025

View reviewed changes

dearfl force-pushed the version-support-publish-diagnostics branch from f982a09 to 32c4548 Compare October 30, 2025 17:00

dearfl force-pushed the version-support-publish-diagnostics branch from 32c4548 to a201c98 Compare November 4, 2025 06:00

yangdanny97 assigned kinto0 Nov 7, 2025

dearfl force-pushed the version-support-publish-diagnostics branch from a201c98 to 3624c06 Compare November 12, 2025 13:35

dearfl force-pushed the version-support-publish-diagnostics branch from 3624c06 to 2e27f24 Compare November 21, 2025 00:13

dearfl requested a review from kinto0 November 21, 2025 00:13

dearfl force-pushed the version-support-publish-diagnostics branch from 2e27f24 to 17c375e Compare November 30, 2025 14:26

kinto0 requested changes Dec 4, 2025

View reviewed changes

dearfl and others added 8 commits December 5, 2025 23:09

add version support to textDocument/publishDiagnostics

57670c9

use last version +1 on close

13ead8b

add test for publish diagnostics version support

f2183e4

tries to fix tests

f51b43c

improve test naming

fdd17e3

Co-authored-by: Kyle Into <kyleatyahoo@gmail.com>

simplify path operations

3abc829

Co-authored-by: Kyle Into <kyleatyahoo@gmail.com>

copy version info hashmap instead of locking it

458ca00

better naming

d49f787

dearfl force-pushed the version-support-publish-diagnostics branch from f1bfc8f to d49f787 Compare December 6, 2025 02:51

dearfl requested a review from kinto0 December 6, 2025 02:52

add version support to textDocument/publishDiagnostics #1382

Are you sure you want to change the base?

add version support to textDocument/publishDiagnostics #1382

Conversation

dearfl commented Oct 25, 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!

dearfl Oct 28, 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!

Choose a reason for hiding this comment

Uh oh!

dearfl Oct 28, 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!

dearfl commented Oct 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kinto0 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

dearfl commented Oct 28, 2025

Uh oh!

kinto0 commented Oct 29, 2025

Uh oh!

dearfl commented Oct 30, 2025

Uh oh!

kinto0 commented Nov 20, 2025

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

dearfl Oct 28, 2025 •

edited

Loading

dearfl Oct 28, 2025 •

edited

Loading

dearfl commented Oct 28, 2025 •

edited

Loading