selection_range: use merlin enclosing query instead of shape

[voodoos]

@awilliambauer @pitag-ha I looked at the server answer to the selectionRange query after our discussion earlier today, and I noticed that they were, in fact, duplicated ranges:

[Trace - 17:40:29] Received response 'textDocument/selectionRange - (20)' in 6ms.
Result: [
    {
        "parent": {
            "parent": {
                "parent": {
                    "parent": {
                        "parent": {
                            "parent": {
                                "parent": {
                                    "parent": {
                                        "parent": {
                                            "parent": {
                                                "range": {
                                                    "end": {
                                                        "character": 38,
                                                        "line": 968
                                                    },
                                                    "start": {
                                                        "character": 0,
                                                        "line": 0
                                                    }
                                                }
                                            },
                                            "range": {
                                                "end": {
                                                    "character": 26,
                                                    "line": 409
                                                },
                                                "start": {
                                                    "character": 0,
                                                    "line": 379
                                                }
                                            }
                                        },
                                        "range": {
                                            "end": {
                                                "character": 26,
                                                "line": 409
                                            },
                                            "start": {
                                                "character": 0,
                                                "line": 379
                                            }
                                        }
                                    },
                                    "range": {
                                        "end": {
                                            "character": 26,
                                            "line": 409
                                        },
                                        "start": {
                                            "character": 2,
                                            "line": 380
                                        }
                                    }
                                },
...
            "range": {
                "end": {
                    "character": 26,
                    "line": 409
                },
                "start": {
                    "character": 4,
                    "line": 409
                }
            }
        },
        "range": {
            "end": {
                "character": 26,
                "line": 409
            },
            "start": {
                "character": 20,
                "line": 409
            }
        }
    }
]

With that patch these are removed:

[Trace - 17:34:27] Received response 'textDocument/selectionRange - (149)' in 12ms.
Result: [
    {
        "parent": {
            "parent": {
                "parent": {
                    "parent": {
                        "parent": {
                            "parent": {
                                "parent": {
                                    "parent": {
                                        "parent": {
                                            "range": {
                                                "end": {
                                                    "character": 38,
                                                    "line": 968
                                                },
                                                "start": {
                                                    "character": 0,
                                                    "line": 0
                                                }
                                            }
                                        },
                                        "range": {
                                            "end": {
                                                "character": 26,
                                                "line": 409
                                            },
                                            "start": {
                                                "character": 0,
                                                "line": 379
                                            }
                                        }
                                    },
                                    "range": {
                                        "end": {
                                            "character": 26,
                                            "line": 409
                                        },
                                        "start": {
                                            "character": 2,
                                            "line": 380
                                        }
                                    }
                                },
...
            "range": {
                "end": {
                    "character": 26,
                    "line": 409
                },
                "start": {
                    "character": 4,
                    "line": 409
                }
            }
        },
        "range": {
            "end": {
                "character": 26,
                "line": 409
            },
            "start": {
                "character": 20,
                "line": 409
            }
        }
    }
]

Sadly vscode still feels the need to add extraneous steps, notably when there is some indentation:

Screen.Recording.2024-09-03.at.17.36.03.mov

That's quite unfortunate... but this PR at least improves the servers' answer and does remove a few no-op steps compared to the previous behaviour.

[coveralls]

Pull Request Test Coverage Report for Build 4575

Warning: 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.

• For more information on this, see Tracking coverage changes with pull request builds.
• To avoid this issue with future PRs, see these Recommended CI Configurations.
• For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

• 0 of 9 (0.0%) changed or added relevant lines in 1 file are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+0.007%) to 21.927%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
ocaml-lsp-server/src/ocaml_lsp_server.ml	0	9	0.0%

Totals
Change from base Build 4571:	0.007%
Covered Lines:	5554
Relevant Lines:	25330

---

💛 - Coveralls

[rgrinberg]

Does this work for documents that aren't well typed?

[voodoos]

Does this work for documents that aren't well typed?

I would expect it to behave as good (or as bad) as the current version. Looking at the Merlin implementation they do fairly similar things but shape returns too much information for our need.

[voodoos]

I just had this déjà-vu feeling.

I openned a very similar PR 2 years ago...

#860

[rgrinberg]

Yup. I just read it over and I still don't really understand how much this changes. I suppose you're confident that this is a strict improvement? A few tests would make that a lot more convincing.

To recall my main desire for this feature: the criteria for what is a "selection" should be a purely syntactic one. It would make this command very useful for moving logical chunks of code around that don't need to be typed (comments, records field definitions, match statement clauses).

[voodoos]

It is true that the logic is quite different, but so far I was not able to witness any difference... I will try to write more interesting tests.

The de-duplication is done explicitly in Merlin for the Enclosing command. If we want we could simply do the same thing for the Shape command.

Still, it looks like the output of the Shape command is not fitted for the selectionRange query since most of the result from Merlin is filtered out to "try to find the nearest range". The enclosing command returns only the typedtree branch containing the cursor, which seems to correspond better to what is needed.

I will write more test, but I would argue that it is a strict improvement: the code is simpler (on both sides, Merlin and OCaml-LSP), and the results do not contain duplicates anymore.

I (still) agree with the fact that the Typedtree is probably not the best way to answer these queries, but that's out of scope for fix PR which aim is to remove duplicates.

[rgrinberg]

No argument against this being an improvement. You would know more than me. I would like to see some more tests to make sure we are understanding if there's a change of behavior or a regression. After hover and completion, this is my most used request in LSP. I imagine other users are also relying on this request heavily. We would all welcome improvements in this functionality.

[voodoos]

@rgrinberg I added a few more tests and so far these changes only remove duplicate entries from the results. I also tried the feature a bit around the codebase and I am quite confident that there is no bad change in behavior. (I am not a regular user however, so I might miss some differences.) If you have some suggestion for some more precise tests cases I will be happy to add them, but right now I think this is ready to be merged.

[rgrinberg]

Some tests demonstrating the behavior with:

1. modules containing a syntax error
2. modules that are unable to type check

Would be appropriate I think