Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve the infer-interface code action and add an update-signature code action #1289

Merged
merged 7 commits into from
May 20, 2024

Conversation

awilliambauer
Copy link
Contributor

This PR improves the insert-inferred-interface code-action by restricting the set of signatures inserted to only the names not already present in the interface file. Previously, this code action was only really useful on a completely empty .mli file, since all signaures from the corresponding .ml file were inserted, including duplicates of signatures already present in the .mli.

Repeated applications of this code-action will now more appropriately be no-ops.

This PR also adds an update-signatures code action that updates the signatures for selected elements of the interface based on types inferred from the corresponding implementation.

The code action works by checking for an implementation file corresponding to the current interface and querying Merlin for type analysis on both. It then extracts items from the interface that overlap with the range the user has selected and attempts to identify matching items from the inferred interface of the implementation. For any matching items, it asks Merlin to print signatures for the old and new types, and if those strings differ, it produces a text edit for the interface to make it match the implementation.

Testing

I added a new expect-test that confirms the inserted signatures exclude ones already present in the interface. The existing expect-test that assumed an empty .mli is unaffected.

New expect-tests verify the intended behavior of update-signatures in the following situations:

  • new arguments added to a function
  • arguments dropped from a function
  • type of a function argument changed
  • multiple functions selected; some changed some not
  • within a module exposed in the mli: a function added and cases of a variant changed

This can be slow (~2s) on very large .mli files (~5k lines), but I've confirmed that the pre-existing version of insert-inferred-interface took a similarly long time.

_
Signed-off-by: Rudi Grinberg <me@rgrinberg.com>
@coveralls
Copy link

coveralls commented May 20, 2024

Pull Request Test Coverage Report for Build 4268

Details

  • 112 of 131 (85.5%) changed or added relevant lines in 4 files are covered.
  • 10 unchanged lines in 1 file lost coverage.
  • Overall coverage increased (+0.3%) to 21.791%

Changes Missing Coverage Covered Lines Changed/Added Lines %
ocaml-lsp-server/src/code_actions/action_update_signature.ml 14 15 93.33%
ocaml-lsp-server/src/inference.ml 91 109 83.49%
Files with Coverage Reduction New Missed Lines %
ocaml-lsp-server/src/inference.ml 10 65.31%
Totals Coverage Status
Change from base Build 4265: 0.3%
Covered Lines: 5345
Relevant Lines: 24529

💛 - Coveralls

_
Signed-off-by: Rudi Grinberg <me@rgrinberg.com>
@rgrinberg
Copy link
Member

LGTM after a couple of changes:

  1. removed ppx_let in favor of the syntax that comes with OCaml. It's not as good as ppx_let, but for the simple use cases here it's enough and it's nice to not require users to install ppx for lsp

  2. remove all the base references to Import. Eventually, we should just write include Base in Import and replace Stdune completely, but that's a bit too much for this PR.

@rgrinberg rgrinberg merged commit 6a7dff1 into ocaml:master May 20, 2024
9 checks passed
| Typedtree.Tsig_include _
| Typedtree.Tsig_class _
| Typedtree.Tsig_class_type _
| Typedtree.Tsig_attribute _ -> None
Copy link
Collaborator

@voodoos voodoos May 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we want to relax ocaml-lsp's dependency on the Typedtree on the long term we should really try to put such utilities in merlin-lib instead...

Also, I am curious about the reason why classes' ids are not needed for the update-signature action ?

@awilliambauer awilliambauer deleted the improved-infer-interface branch May 21, 2024 14:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants