Add a new command to extract expression into a fresh let binding #1948
Conversation
Tim-ats-d
changed the title
|
Just realized that I should probably add the corresponding documentation in the |
Tim-ats-d
force-pushed
the
refactor-extraction
branch
from
e1e25d9 to
a7a3729
Compare
|
There is one test that seems fail. Is that an issue or you need to repromote it? |
| let open Ast_helper in | ||
| let pattern = Pat.mk (Ppat_var { txt = name; loc = Location.none }) in | ||
| let body = Parsetree_utils.filter_expr_attr body in | ||
| Str.value Nonrecursive [ Vb.mk pattern body ] |
There was a problem hiding this comment.
Looking to Nonrecursive.
I am a little but curious about case like that:
let f =
let x = 1 in
let y = 2 in
let z = x + y in
z + 1Let's imagine that we extract let z = x + y in or let z = x + y we can imagine that it will produce the following code:
let z x y = x + y
let f =
let x = 1 in
let y = 2 in
(z x y) + 1No? In other terms, I think that if the selected range is a let-binding, we change the binding as a toplevel value (and we preserve the name if it makes senses, ie: if the binding is free, using Fresh_name, or using a fresh binding)
So we can imagine that if we extract a recursive definition, we preserve it.
No, it wasn't an issue, strictly speaking. It was an example of extraction that better not work, given that the extracted expression is much smaller than the size of the selected region the result is a bit confusing. I think I'll replace this test with another one. |
Tim-ats-d
force-pushed
the
refactor-extraction
branch
from
094b858 to
d790c41
Compare
Tim-ats-d
force-pushed
the
refactor-extraction
branch
from
d790c41 to
d2d6e08
Compare
I replaced it by other more relevant tests from ocaml-lsp (38055ff#diff-1b5f000f9708e3dfe32d04e4ca43f63eb3d55ce173e7c8cd63bbca41e61639d1) |
…d call and add a corresponding test.
Tim-ats-d
force-pushed
the
refactor-extraction
branch
from
cd3c4a0 to
195ed6d
Compare
Tim-ats-d
force-pushed
the
refactor-extraction
branch
from
4aeb105 to
33906c7
Compare
There was a problem hiding this comment.
Thanks @Tim-ats-d !
It looks good overall, I mostly have some naming issues that made the re-reading of the code harder at some places.
I did not review thoroughly the correctness of the code generation.
|
Also, the Ci breakage should be fixed :-) |
|
I don't know if this PR can be merged: there are still a few bugs in the extraction of values belonging to local modules that could be improved but on the other hand, it works well for most basic use cases. The extraction of |
At the very least, these "bugs" that you have identified should be reproduced in the testsuite with a FIXME stating the expected result. Maybe in separate test files to make them more easy to find ? |
|
And after |
|
I have added the |
| FIXME: `x` is used instead of `d_x` in the extracted function body! | ||
| Should be: (((a + b) + ((c * x) * y)) + z) + d_x | ||
|
|
||
| $ $MERLIN single refactoring-extract-region -start 10:2 -end 10:31 < foo.ml | ||
| { | ||
| "class": "return", | ||
| "value": { | ||
| "start": { | ||
| "line": 3, | ||
| "col": 0 | ||
| }, | ||
| "end": { | ||
| "line": 10, | ||
| "col": 31 | ||
| }, | ||
| "content": "let fun_name1 x y a b c d_x = (((a + b) + ((c * x) * y)) + z) + x |
There was a problem hiding this comment.
I have mixed feelings about this. First, is this a minimal example ? Do you need a, b and c to trigger the issue ?
The more minimal the example is, when reproducing a precise error, the easier it will be to reason about for someone who would like to try and fix it later.
Also it feels like the fix would not be too hard... would it ?
|
@Tim-ats-d what do you think of 9bb9388 ? This still feels a bit sketchy, but at least more cases are working now. |
It's perfectly fine, thanks. I think that will do the trick for now. |
3 participants
This PR adds a new command
refactoring-extract-regionto merlin protocol which allows extracting an expression into a let binding. See the interface comments for supported casesImplementation
The implementation of the substitution is a bit hacky and relies on string manipulation, especially when the generated binding is a
and. I couldn't deal with typedtree locations for the generated code, so I had to make a few manual location calculations that I tried to make as explicit as possible. Please tell me if there's a better way to achieve it.It still lacks a bit of testing, I'll be adding more in the next few days.
cc @xvw @voodoos