Refactor analysis to decouple I/O from core logic#8426
Conversation
Splits `Commands.ml` into a pure layer that returns OCaml values (option, list, typed records like Protocol.hover, Protocol.signatureHelp, Protocol.completionItem) and a new `analysis/src/Cli.ml` that does the stringify-and-print step. `analysis/bin/main.ml` now dispatches to `Cli.*`, while the LSP server consumes `Commands.*` directly. Makes the parsers accept source strings: `res_driver` gains `parse_interface_from_source` alongside the existing `parse_implementation_from_source`
| let emit e = | ||
| let sortedTokens = | ||
| e.tokens | ||
| |> List.sort (fun (l1, c1, _, _) (l2, c2, _, _) -> | ||
| if l1 = l2 then compare c1 c2 else compare l1 l2) | ||
| in | ||
| let buf = Buffer.create 1 in | ||
| sortedTokens |> List.iter (fun t -> e |> emitToken buf t); | ||
| let arrays = sortedTokens |> List.filter_map (fun t -> e |> emitToken t) in | ||
| Array.concat arrays |
There was a problem hiding this comment.
Instead returning a string "[0,1,2,3]" return a int array https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#semanticTokens
| let fields = | ||
| [("range", Some (Protocol.stringifyRange range))] | ||
| @ | ||
| match placeholderOpt with | ||
| | None -> [] | ||
| | Some s -> [("placeholder", Some (Protocol.wrapInQuotes s))] | ||
| in |
There was a problem hiding this comment.
I believe this was incorrect. Prepare rename should return Range | { range: Range, placeholder: string } | { defaultBehavior: boolean } | null (https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#textDocument_prepareRename)
placeholderOpt is an option, so if it's None, the placeholder field doesn't exist; however, { range: Range, placeholder: string } is expected.
rescript
@rescript/darwin-arm64
@rescript/darwin-x64
@rescript/linux-arm64
@rescript/linux-x64
@rescript/runtime
@rescript/win32-x64
commit: |
|
@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: 7ccb7ecb9d
ℹ️ 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".
| let max = String.length source in | ||
| let range = | ||
| Protocol. | ||
| {start = {line = 0; character = 0}; end_ = {line = max; character = max}} |
There was a problem hiding this comment.
Commands.format now returns a TextEdit whose end position is built from String.length source for both line and character, but those fields are line/column coordinates, not byte offsets. For multi-line inputs this produces an out-of-document range, and strict LSP clients can reject or misapply the formatting edit instead of replacing the whole file. Compute the end position from the actual last line and its column (or an explicitly valid full-document range) before returning the edit.
Useful? React with 👍 / 👎.
| Res_driver.print_implementation_from_source = | ||
| (fun ~width:_ ~source ~comments:_ _ -> dump_tokens source); |
There was a problem hiding this comment.
The new print_implementation_from_source hook forwards source to dump_tokens, but dump_tokens interprets its argument as a filesystem path and calls open_in on it. Any caller that uses the new *_from_source print API with in-memory source will fail by trying to open a file named after the source contents and then exiting. This path should scan the provided source string directly instead of routing through filename-based I/O.
Useful? React with 👍 / 👎.
The previous implementation used the source byte length as both line and character values for the end of the format range, which was incorrect. Replace it with a helper that computes the actual final line index and character offset by splitting on newlines. Signed-Off-By: Your Name <email>
2 participants
Splits
Commands.mlinto a pure layer that returns OCaml values (option, list, typed records like Protocol.hover,Protocol.signatureHelp, Protocol.completionItem) and a new
analysis/src/Cli.mlthat does the stringify-and-print step.analysis/bin/main.mlnow dispatches toCli.*, while the LSP server consumesCommands.*directly.Makes the parsers accept source strings:
res_drivergainsparse_interface_from_sourcealongside the existingparse_implementation_from_sourceCherry-pick of #8425