Type enclosing query #1304
Type enclosing query #1304
Conversation
xvw
force-pushed
the
type-enclosing-query
branch
from
7806021
to
716be32
Compare
|
I don't remember: is there any reason why this is based on the 5.2 branch instead of master ? |
Branch rebased on |
xvw
force-pushed
the
type-enclosing-query
branch
from
716be32
to
109801e
Compare
Pull Request Test Coverage Report for Build 4367Details
💛 - Coveralls |
Pull Request Test Coverage Report for Build 4368Details
💛 - Coveralls |
Pull Request Test Coverage Report for Build 4369Details
💛 - Coveralls |
Pull Request Test Coverage Report for Build 4370Details
💛 - Coveralls |
xvw
force-pushed
the
type-enclosing-query
branch
3 times, most recently
from
f6e6a53
to
425f6e0
Compare
Pull Request Test Coverage Report for Build 4371Details
💛 - Coveralls |
Pull Request Test Coverage Report for Build 4372Details
💛 - Coveralls |
Pull Request Test Coverage Report for Build 4373Details
💛 - Coveralls |
Pull Request Test Coverage Report for Build 4380Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
| | Some position -> | ||
| let lend = range.Range.end_ in | ||
| if lend.line = position.Position.line then | ||
| lend.character > position.character |
There was a problem hiding this comment.
| lend.character > position.character | |
| lend.character >= position.character |
I think ?
There was a problem hiding this comment.
Not really necessary because it is already guarded by the first condition:
let overlap_with_range_end range = function
| None -> true
| Some position ->
let lend = range.Range.end_ in
if lend.line = position.Position.line (* this one *) then
lend.character > position.character
else lend.line > position.lineThere was a problem hiding this comment.
Are you sure about that ? I don't see it... they could be on the same line and the same character ?
There was a problem hiding this comment.
oh you're right. I was think that your where speaking about line!
| } | ||
| ], | ||
| "type": "sig type t = A of int val x : int end" | ||
| } |}] |
There was a problem hiding this comment.
We should also put a focus on the range_end cases that rely on tricky position comparisons and index shifting :-)
xvw
force-pushed
the
type-enclosing-query
branch
from
b630a27
to
5516d73
Compare
Pull Request Test Coverage Report for Build 4382Details
💛 - Coveralls |
Pull Request Test Coverage Report for Build 4383Details
💛 - Coveralls |
xvw
force-pushed
the
type-enclosing-query
branch
from
eef38b4
to
90f67de
Compare
There was a problem hiding this comment.
It's starting to look good !
There is still some weirdness in the tests however...
(Also, but that's personal taste: I think utility functions are sometimes a bit too far from their call point. Especially when they are only used once, sometime you could just inline them or move them in a deeper scope. But that's really nitpicking, don't feel obliged to change anything.)
| "position": Position, | ||
| "index": uinteger, | ||
| "verbosity?": uinteger, | ||
| "rangeEnd?": Position |
There was a problem hiding this comment.
I think we should describe the roles of the parameters here, especially rangeend (we could simply reuse the comments in the design doc if they are up-to-date)
There was a problem hiding this comment.
Instead of this scheme, I'd recommend the following:
- Either take a
rangeparameter - Or a
positionparameter
That way you can stick to standard field names and users can use the convenient Range.t type to work with your request.
There was a problem hiding this comment.
You are right. Fixed in 89efc55
| let get_logical_position tdp = | ||
| let p = tdp.Lsp.Types.TextDocumentPositionParams.position in | ||
| Position.logical p |
There was a problem hiding this comment.
Nitpick: not sure about the need for this function. It is far from it's unique callsite and does nothing more than calling a function that already bear a quite similar name.
| | Some position -> | ||
| let lend = range.Range.end_ in | ||
| if lend.line = position.Position.line then | ||
| lend.character > position.character |
There was a problem hiding this comment.
Are you sure about that ? I don't see it... they could be on the same line and the same character ?
| let overlap_with_range_end range position = | ||
| let lend = range.Range.end_ in | ||
| if lend.line = position.Position.line then lend.character > position.character | ||
| else lend.line > position.line |
There was a problem hiding this comment.
Not sure about that, but maybe we could simply use Position.compare_inclusion to check that the range_end pos is inside the enclosing range. This has the main advantage of not duplicating this kind of error-prone reasoning...
| let source = {|let x = string_of_int 2002|} in | ||
| let request client = | ||
| let open Fiber.O in | ||
| let position = Position.create ~line:0 ~character:22 in |
There was a problem hiding this comment.
I am trying to read the tests: these are positions 0-based right ? For the line it looks like it is the case, and for the column (character) number too ?
| "enclosings": [ | ||
| { | ||
| "end": { "character": 26, "line": 0 }, | ||
| "start": { "character": 8, "line": 0 } | ||
| } | ||
| ], | ||
| "type": "int" |
There was a problem hiding this comment.
This is wrong isn't it ?
Looks like the enclosing of string_of_int 2002 which should have type string...
We expect something like this:
"index": 0,
"enclosings": [
{
"end": { "character": 26, "line": 0 },
"start": { "character": 22, "line": 0 }
},
{
"end": { "character": 26, "line": 0 },
"start": { "character": 8, "line": 0 }
}
],
"type": "int"
}
And if the required index was 1:
"index": 1,
"enclosings": [
{
"end": { "character": 26, "line": 0 },
"start": { "character": 22, "line": 0 }
},
{
"end": { "character": 26, "line": 0 },
"start": { "character": 8, "line": 0 }
}
],
"type": "string"
}
| | Ordering.(Gt, Eq) | ||
| | Ordering.(Eq, Eq) | ||
| | Ordering.(Gt, Lt) -> Some i | ||
| | _ -> aux (succ i) xs) |
| Query_protocol.Type_enclosing (None, position, Some index) | ||
|
|
||
| let get_first_enclosing_index range_end enclosings = | ||
| let rec aux i = function |
There was a problem hiding this comment.
I think this is just List.findi?
| let print_type_enclosing result = | ||
| result |> Yojson.Safe.pretty_to_string ~std:false |> print_endline | ||
|
|
||
| let test ?(verbosity = 0) ?(index = 0) ?range_end ~line ~character source = |
There was a problem hiding this comment.
I'd say just make these required
| in | ||
| Client.request client req | ||
|
|
||
| let print_type_enclosing result = |
There was a problem hiding this comment.
You can move this to e2e-new/test.ml since its general purpose
There was a problem hiding this comment.
Is that really general purpose? FMPOV it is really related to testing type enclosing queries.
There was a problem hiding this comment.
It appears in e2e-new/completion.ml, e2e-new/hover_extended.ml, e2e-new/merlin_call_compatible.ml. So I'd say it's general purpose. You gave it a very specific name here, but it could just be called pretty_print_json
| let text_document = TextDocumentIdentifier.create ~uri in | ||
| let params = | ||
| `Assoc | ||
| ([ ("textDocument", TextDocumentIdentifier.yojson_of_t text_document) |
There was a problem hiding this comment.
How about just exporting the type from req_type_enclosing? Writing out the json here will just make it harder to sync the test with the actual code.
| let dispatch_type_enclosing text_document_position index range_end pipeline = | ||
| let position = | ||
| Position.logical | ||
| text_document_position.Lsp.Types.TextDocumentPositionParams.position |
There was a problem hiding this comment.
You can annotation text_document_position instead.
| | Some (typ, enclosings) -> | ||
| (typ, List.map ~f:Lsp.Types.Range.yojson_of_t enclosings) | ||
| in | ||
| `Assoc |
There was a problem hiding this comment.
This needs a defined response type.
| Some { index; range_end; verbosity; text_document_position } | ||
| | _ -> None | ||
|
|
||
| let of_yojson_exn = function |
There was a problem hiding this comment.
Given that this function decodes params:
- Its name should be
params_of_yojson - Its signature should be
Yojson.t -> params
We must match the conventions of the code generator
|
Sorry for the long winded review. Regarding all the issues with json, I realize that these are issues that are essentially present in all of our custom requests. Indeed, we've added a lot of custom code doing things that are standard and all of our standard requests have a quirky behavior. I see that at least somebody tried to unify things a little bit by introducing
match In_request.of_jsonrpc req with
| Error message ->
let code = Jsonrpc.Response.Error.Code.InvalidParams in
let error = Jsonrpc.Response.Error.make ~code ~message () in
Fiber.return
(Jsonrpc_fiber.Reply.now (Jsonrpc.Response.error req.id error), state)We should strive for the same for custom requests. In any case, it's not fair to hold your code to a higher standard to all previous code. So for now you don't need to address to make everything uniform, but I would like at least point 3 addressed with request/response module and their own |
|
Thank you very much for your detailed review!
From my point of view, it's not worth merging code that we explicitly know can be improved, so I'm going to try, as best I can, to take into account all the proposed points. Then I'll try to do a pass on all the custom requests that have already been implemented. Thank you very much, I'll take the liberty of pinging you when this PR is ready (Monday). |
|
Sure, I'm certainly against improving things across the board. Feel free to regularize all custom requests. I'll give some background why the situation is like this in the first place. In the beginning, I was very much against custom requests and even more so against investing a lot of work into them. I thought they would require a lot duplicate work on the client side and the LSP protocol would evolve to make them all obsolete anyhow. The LSP protocol didn't really evolve much and the use cases for custom requests steadily grew. The end result is that we ended up with a lot of ad-hoc extensions. |
Pull Request Test Coverage Report for Build 4395Details
💛 - Coveralls |
Pull Request Test Coverage Report for Build 4397Details
💛 - Coveralls |
|
|
||
| let%expect_test {| | ||
| First, let's locate on [A.z], we expect the type [t], but we | ||
| will increase the verbosity in order to get the fuill expansion of |
There was a problem hiding this comment.
| will increase the verbosity in order to get the fuill expansion of | |
| will increase the verbosity in order to get the full expansion of |
|
The CI failures appear to be unrelated ? Maybe we should bump the setup-ocaml action like in ocaml/merlin#1791 ? |
Co-authored-by: Ulysse <5031221+voodoos@users.noreply.github.com>
CHANGES:
## Features
- Introduce a configuration option to control dune diagnostics. The option is
called `duneDiganostics` and it may be set to `{ enable: false }` to disable
diagnostics. (ocaml/ocaml-lsp#1221)
- Support folding of `ifthenelse` expressions (ocaml/ocaml-lsp#1031)
- Improve hover behavior (ocaml/ocaml-lsp#1245)
Hovers are no longer displaye on useless parsetree nodes such as keywords,
comments, etc.
Multiline hovers are now filtered away.
Display expanded ppx's in the hover window.
- Improve document symbols (ocaml/ocaml-lsp#1247)
Use the parse tree instead of the typed tree. This means that document
symbols will work even if the source code doesn't type check.
Include symbols at arbitrary depth.
Differentiate functions / types / variants / etc.
This now includes PPXs like `let%expect_test` or `let%bench` in the outline.
- Introduce a `destruct-line` code action. This is an improved version of the
old `destruct` code action. (ocaml/ocaml-lsp#1283)
- Improve signature inference to only include types for elements that were
absent from the signature. Previously, all signature items would always be
inserted. (ocaml/ocaml-lsp#1289)
- Add an `update-signature` code action to update the types of elements that
were already present in the signature (ocaml/ocaml-lsp#1289)
- Add custom
[`ocamllsp/merlinCallCompatible`](https://github.com/ocaml/ocaml-lsp/blob/e165f6a3962c356adc7364b9ca71788e93489dd0/ocaml-lsp-server/docs/ocamllsp/merlinCallCompatible-spec.md)
request (ocaml/ocaml-lsp#1265)
- Add custom [`ocamllsp/typeEnclosing`](https://github.com/ocaml/ocaml-lsp/blob/109801e56f2060caf4487427bede28b824f4f1fe/ocaml-lsp-server/docs/ocamllsp/typeEnclosing-spec.md) request (ocaml/ocaml-lsp#1304)
## Fixes
- Detect document kind by looking at merlin's `suffixes` config.
This enables more lsp features for non-.ml/.mli files. Though it still
depends on merlin's support. (ocaml/ocaml-lsp#1237)
- Correctly accept the `--clientProcessId` flag. (ocaml/ocaml-lsp#1242)
- Disable automatic completion and signature help inside comments (ocaml/ocaml-lsp#1246)
- Includes a new optional/configurable option to toggle syntax documentation. If
toggled on, allows display of syntax documentation on hover tooltips. Can be
controlled via environment variables and by GUI for VS code. (ocaml/ocaml-lsp#1218)
- For completions on labels that the LSP gets from merlin, take into account
whether the prefix being completed starts with `~` or `?`. Change the label
completions that start with `?` to start with `~` when the prefix being
completed starts with `~`. (ocaml/ocaml-lsp#1277)
- Fix document syncing (ocaml/ocaml-lsp#1278, ocaml/ocaml-lsp#1280, fixes ocaml/ocaml-lsp#1207)
- Stop generating inlay hints on generated code (ocaml/ocaml-lsp#1290)
- Fix parenthesizing of function types in `SignatureHelp` (ocaml/ocaml-lsp#1296)
- Fix syntax documentation rendering (ocaml/ocaml-lsp#1318)
This PR is based on 1265 and add a
type enclosingcustom request.