Use Yojson and lsp types for analysis JSON output#8436
Conversation
Replace the vendored JSON helpers with Yojson and the OCaml lsp types across analysis, reanalyze, and tools JSON handling. This updates LSP response construction for completions, hovers, diagnostics, code actions, symbols, signature help, and related CLI commands, refreshes analysis snapshots, and adjusts extract-codeblocks decoding for the new serialized output.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #8436 +/- ##
==========================================
+ Coverage 60.10% 60.63% +0.53%
==========================================
Files 373 373
Lines 54231 54057 -174
==========================================
+ Hits 32593 32778 +185
+ Misses 21638 21279 -359
🚀 New features to boost your workflow:
|
Serialize analysis command results with Yojson output helpers instead of string extraction, fixing document symbols so they return JSON arrays instead of crashing. Also use typed LSP enum constructors for completion and inlay hint kinds, clean up JSON config parsing, and update analysis test snapshots.
Replace the custom URI record with Lsp.Uri.t and delegate path/URI conversion to the LSP library. This fixes interface-aware analysis results so definitions, references, and renames consistently target .resi files when an interface is present, and updates the analysis snapshots accordingly.
| match | ||
| Tools.ExtractCodeblocks.extractCodeblocksFromFile ~transformAssertEqual | ||
| ~entryPointFile:path | ||
| with | ||
| | Ok _ as r -> | ||
| print_endline (Analysis.Protocol.stringifyResult r); | ||
| exit 0 | ||
| | Error _ as r -> | ||
| print_endline (Analysis.Protocol.stringifyResult r); | ||
| exit 1) |
There was a problem hiding this comment.
If is an error print to stderr
rescript
@rescript/darwin-arm64
@rescript/darwin-x64
@rescript/linux-arm64
@rescript/linux-x64
@rescript/runtime
@rescript/win32-x64
commit: |
…GenericJsxCompletion.res.txt
Only include optional doc extraction fields like deprecated, detail, payload, and moduletypeid when they have values. Update tool test snapshots to match the leaner JSON output.
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1d9d660927
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| |> List.map (fun uri -> | ||
| let path = Uri.toPath uri in | ||
| let dir = Filename.dirname path in | ||
| let path = Uri.toString uri in |
There was a problem hiding this comment.
When renaming a top-level module, this code starts from Uri.toString uri, which is a file://... URI in normal execution, then applies Filename.dirname/Filename.extension and finally Uri.fromPath to the result. That treats a URI as a filesystem path and then wraps it as a URI again, which can generate incorrect newUri values and break rename-file operations outside test mode (stripPath = false). Build newPath from Uri.toPath uri and convert back to a URI only once.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
I don’t think this change is unnecessary in the current code.
referencesToToplevelModules is built from filesystem paths via Uri.fromPath in References.allReferencesForLocItem
There was a problem hiding this comment.
To me the comment from Codex seems to make sense - we want a path, not a file uri string.
Why did you change Uri.toPath to Uri.toString here?
There was a problem hiding this comment.
To avoid changes in the tests. When i use Uri.toPath the output is:
[
{
"kind": "rename",
- "newUri": "file:///RenameWithInterfacePrime.resi",
+ "newUri": "file:///./src/RenameWithInterfacePrime.resi",
"oldUri": "file:///RenameWithInterface.resi"
},
{
"kind": "rename",
- "newUri": "file:///RenameWithInterfacePrime.res",
+ "newUri": "file:///./src/RenameWithInterfacePrime.res",
"oldUri": "file:///RenameWithInterface.res"
},
{But in fact, Filename doesn't handle the file:// schema well.
Done in 2f73b51
|
Great to finally be able to standardize on yojson and close #7456! 🎉 |
2 participants
Replace the vendored JSON helpers with Yojson and the OCaml lsp types across analysis, reanalyze, and tools JSON handling.
This updates LSP response construction for completions, hovers, diagnostics, code actions, symbols, signature help, and related CLI commands, refreshes analysis snapshots, and adjusts extract-codeblocks decoding for the new serialized output.
TODO
nullCross.res.txtDefinitionWithInterface.resi.txturifield should print withfile://schemaCherry-pick of #8425
Close #7456