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

Add code action for inlining #847

Merged
merged 40 commits into from
Oct 10, 2022
Merged

Add code action for inlining #847

merged 40 commits into from
Oct 10, 2022

Conversation

jfeser
Copy link
Collaborator

@jfeser jfeser commented Aug 30, 2022

This PR adds an inlining code action for non-top-level let bindings. For example,

let test y =
  let x = y + 1 in
      ^
  x + 2

turns into

let test y =
  let x = y + 1 in
  (y + 1) + 2

after inlining.

Caveats:

  • The inlining context can shadow bindings in the body, which changes the meaning of the inlined code. I.e., this transformation isn't sound. We should at least be able to use Merlin to detect this and block the action, but I'm not sure how.
  • Right now I'm using Untypeast followed by Pprintast to turn the body of the let into a string. This means that comments are stripped and the inlined code contains expanded PPXes. This should be fixable by using the source code the body, but it requires a bit more care when inlining functions.
  • The original definition is not removed. It should be unused after inlining, so it'll be a target for the "remove unused" action.

@ulugbekna ulugbekna self-requested a review August 30, 2022 18:12
@ulugbekna
Copy link
Collaborator

This is great! Thanks! I will review it this week.

The inlining context can shadow bindings in the body, which changes the meaning of the inlined code. I.e., this transformation isn't sound. We should at least be able to use Merlin to detect this and block the action, but I'm not sure how.

Perhaps, @voodoos could advise as an expert here.

Otherwise, you could find quite a bit of help from people on ocaml discord (eg octachron is quite active and has expertise in the typechecker) or ocaml discourse, which also may attract many enthusiasts

First things that come to mind are to

  • either capture Path.ts of (free) variables in the let binding, and make sure that those variables keep referencing the same Path.ts when inlined. Variables also have Env.t that could be used to see where they're defined. I'll think about this more on Friday.
  • or see if any binding after the one being inlined shadows its free variables (how to compute the free variables though)

@jfeser
Copy link
Collaborator Author

jfeser commented Aug 30, 2022

Also, inlining is always unsound in OCaml, since it can duplicate side effects. But I think the issue with shadowing is more likely to introduce subtle bugs.

Copy link
Collaborator

@ulugbekna ulugbekna left a comment

Choose a reason for hiding this comment

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

Thanks! It was an interesting read; the feature seems to work well.

I will also think what we can do with shadowed variables and side-effects.

ocaml-lsp-server/src/code_actions/action_inline.ml Outdated Show resolved Hide resolved
ocaml-lsp-server/src/code_actions/action_inline.ml Outdated Show resolved Hide resolved
ocaml-lsp-server/src/code_actions/action_inline.ml Outdated Show resolved Hide resolved
ocaml-lsp-server/src/code_actions/action_inline.ml Outdated Show resolved Hide resolved
ocaml-lsp-server/src/code_actions/action_inline.ml Outdated Show resolved Hide resolved
ocaml-lsp-server/src/code_actions/action_inline.ml Outdated Show resolved Hide resolved
ocaml-lsp-server/src/code_actions/action_inline.ml Outdated Show resolved Hide resolved
ocaml-lsp-server/src/code_actions/action_inline.ml Outdated Show resolved Hide resolved
ocaml-lsp-server/src/code_actions/action_inline.ml Outdated Show resolved Hide resolved
@jfeser jfeser marked this pull request as ready for review September 8, 2022 19:55
@jfeser jfeser force-pushed the inline branch 2 times, most recently from d6884af to 481ea2a Compare September 8, 2022 20:11
@jfeser
Copy link
Collaborator Author

jfeser commented Sep 20, 2022

Comments should all be addressed.

@jfeser
Copy link
Collaborator Author

jfeser commented Sep 29, 2022

Inlining now does a best-effort round of beta reduction after inlining a function. It also works on top level let bindings.

@rgrinberg rgrinberg added this to the 1.14.0 milestone Oct 8, 2022
Copy link
Member

@rgrinberg rgrinberg left a comment

Choose a reason for hiding this comment

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

Looks good.

@ulugbekna are you still reviewing?

Let's ping @voodoos so that he has the feature and the issues with shadowing in the back of his mind. Perhaps merlin can help us make this more robust one day?

ocaml-lsp-server/src/code_actions/action_inline.ml Outdated Show resolved Hide resolved
ocaml-lsp-server/src/code_actions/action_inline.ml Outdated Show resolved Hide resolved
ocaml-lsp-server/src/code_actions/action_inline.ml Outdated Show resolved Hide resolved
ocaml-lsp-server/src/code_actions/action_inline.ml Outdated Show resolved Hide resolved
ocaml-lsp-server/src/code_actions/action_inline.ml Outdated Show resolved Hide resolved
ocaml-lsp-server/src/code_actions/action_inline.ml Outdated Show resolved Hide resolved
@ulugbekna
Copy link
Collaborator

@ulugbekna are you still reviewing?

I was off past two weeks. Will finish reviewing asap.

Copy link
Collaborator

@ulugbekna ulugbekna left a comment

Choose a reason for hiding this comment

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

Thanks again for this awesome work! I especially liked thoroughness and visual explicitness of tests. I left a couple of nits that definitely don't block this PR. Feel free to merge or let us know when you'd like to merge. I think it's best to squash before merging, btw.

ocaml-lsp-server/test/e2e-new/action_inline.ml Outdated Show resolved Hide resolved
ocaml-lsp-server/test/e2e-new/action_inline.ml Outdated Show resolved Hide resolved
ocaml-lsp-server/src/code_actions/action_inline.ml Outdated Show resolved Hide resolved
ocaml-lsp-server/src/code_actions/action_inline.ml Outdated Show resolved Hide resolved
ocaml-lsp-server/src/code_actions/action_inline.ml Outdated Show resolved Hide resolved
ocaml-lsp-server/src/code_actions/action_inline.ml Outdated Show resolved Hide resolved
@jfeser jfeser merged commit 8730a82 into ocaml:master Oct 10, 2022
@jfeser jfeser deleted the inline branch October 10, 2022 17:31
rgrinberg added a commit to rgrinberg/opam-repository that referenced this pull request Oct 15, 2022
CHANGES:

## Features

- Code action for inlining let bindings within a module or expression. (ocaml/ocaml-lsp#847)

- Tag "unused code" and "deprecated" warnings, allowing clients to better
  display them. (ocaml/ocaml-lsp#848)

- Refresh merlin configuration after every dune build in watch mode (ocaml/ocaml-lsp#853)

## Fixes

- Respect `showDocument` capabilities. Do not offer commands or code actions
  that rely on this request without client support. (ocaml/ocaml-lsp#836)

- Fix signatureHelp on .mll files: avoid "Document.dune" exceptions
Khady added a commit to Khady/ocaml-lsp that referenced this pull request Nov 4, 2022
* master:
  fix(go-to-def): report error in response (ocaml#899)
  Update readme (ocaml#895)
  chore(nix): update flakes and dune-release (ocaml#894)
  chore: remove ocamlformat-rpc (ocaml#892)
  chore(nix): pass opam-repository and nixpkgs (ocaml#893)
  chore: unvendor ocamlc-loc (ocaml#869)
  fix: merlin document safety (ocaml#890)
  chore: more precise CHANGES (ocaml#889)
  fix: diagnostics on non dune files (ocaml#887)
  refactor: remove Document.is_merlin (ocaml#888)
  fix: symbols in non merlin docs (ocaml#886)
  refactor: style tweaks in document_symbol (ocaml#885)
  fix: handle incorrect document types (ocaml#884)
  Ignore unknown config tags (ocaml#883)
  Make sure nodejs packages required are installed
  chore: prepare 1.14.0
  Don't let opam ask when not needed
  Allow copy-and-paste of contributing instructions
  Add code action for inlining (ocaml#847)
  Add note about protocol extensions to the readme
@ulugbekna
Copy link
Collaborator

I used this code action twice today to inline top-level functions, and it worked great! If not for this, inlining would be quite annoying for those fns. Thanks again! :-)

Let me know if you need any help with "extract" code action PR!

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

3 participants