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

Disable redundant type annotation #1037

Merged
merged 7 commits into from Mar 6, 2023
Merged

Conversation

3Rafal
Copy link
Collaborator

@3Rafal 3Rafal commented Mar 2, 2023

This fixes #1036

The constrained pattern from example code

let my_fun (x : int) = x

is represented as below typedtree:

  structure_item (scratch.ml[1,0+0]..scratch.ml[1,0+24])
    Tstr_value Nonrec
    [
      <def>
        pattern (scratch.ml[1,0+4]..scratch.ml[1,0+10])
          Tpat_var \"my_fun/273\"
        expression (scratch.ml[1,0+11]..scratch.ml[1,0+24]) ghost
          Texp_function
          Nolabel
          [
            <case>
              pattern (scratch.ml[1,0+12]..scratch.ml[1,0+13])
                extra
                  Tpat_extra_constraint
                  core_type (scratch.ml[1,0+16]..scratch.ml[1,0+19])
                    Ttyp_constr \"int/1!\"
                    []
                Tpat_alias \"x/275\"
                pattern (scratch.ml[1,0+12]..scratch.ml[1,0+13])
                  Tpat_any
              expression (scratch.ml[1,0+23]..scratch.ml[1,0+24])
                Texp_ident \"x/275\"
          ]
    ]
]

Which means that we have to explicitly check for nested patterns of any and alias.

@3Rafal 3Rafal changed the title WIP: Disable redundant type annotation Disable redundant type annotation Mar 2, 2023
@3Rafal 3Rafal marked this pull request as draft March 2, 2023 16:16
@rgrinberg
Copy link
Member

LGTM. A test would be nice as well.

@rgrinberg rgrinberg added this to the 1.15.1 milestone Mar 2, 2023
@3Rafal 3Rafal marked this pull request as ready for review March 3, 2023 11:25
@3Rafal
Copy link
Collaborator Author

3Rafal commented Mar 3, 2023

@rgrinberg, yes. I made this as a draft because there was no tests and changelog description. Just added them and now I think it's good. :)

@3Rafal 3Rafal requested a review from rgrinberg March 3, 2023 12:03
@@ -478,6 +478,76 @@ let f (x : t) = x
`);
});

it("does not type-annotate already annotated argument", async () => {
Copy link
Member

Choose a reason for hiding this comment

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

Could you try porting this test to OCaml in e2e-new?

The typescript suite is our legacy test suite. We haven't yet ported it to all to OCaml, but we'd like to stop adding new tests at least.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@rgrinberg , sure! I have ported them. After this PR gets merged I will port other type-annotate as well, so that they're all in one place. I already have the filtering functionality, so it should be a simple refactor.

@3Rafal 3Rafal requested a review from rgrinberg March 6, 2023 08:48
@rgrinberg rgrinberg merged commit a2fe6ff into ocaml:master Mar 6, 2023
4 of 5 checks passed
rgrinberg added a commit to rgrinberg/opam-repository that referenced this pull request Jun 18, 2023
CHANGES:

## Fixes

- Disable code lens by default. The support can be re-enabled by explicitly
  setting it in the configuration. (ocaml/ocaml-lsp#1134)

- Fix initilization of `ocamlformat-rpc` in some edge cases when ocamlformat is
  initialized concurrently (ocaml/ocaml-lsp#1132)

- Kill unnecessary `$ dune ocaml-merlin` with SIGTERM rather than SIGKILL
  (ocaml/ocaml-lsp#1124)

- Refactor comment parsing to use `odoc-parser` and `cmarkit` instead of
  `octavius` and `omd` (ocaml/ocaml-lsp#1088)

  This allows users who migrated to omd 2.X to install ocaml-lsp-server in the
  same opam switch.

  We also slightly improved markdown generation support and fixed a couple in
  the generation of inline heading and module types.

- Allow opening documents that were already open. This is a workaround for
  neovim's lsp client (ocaml/ocaml-lsp#1067)

- Disable type annotation for functions (ocaml/ocaml-lsp#1054)

- Respect codeActionLiteralSupport capability (ocaml/ocaml-lsp#1046)

- Fix a document syncing issue when utf-16 is the position encoding (ocaml/ocaml-lsp#1004)

- Disable "Type-annotate" action for code that is already annotated.
  ([ocaml/ocaml-lsp#1037](ocaml/ocaml-lsp#1037)), fixes
  [ocaml/ocaml-lsp#1036](ocaml/ocaml-lsp#1036)

- Fix semantic highlighting of long identifiers when using preprocessors
  ([ocaml/ocaml-lsp#1049](ocaml/ocaml-lsp#1049), fixes
  [ocaml/ocaml-lsp#1034](ocaml/ocaml-lsp#1034))

- Fix the type of DocumentSelector in cram document registration (ocaml/ocaml-lsp#1068)

- Accept the `--clientProcessId` command line argument. (ocaml/ocaml-lsp#1074)

- Accept `--port` as a synonym for `--socket`. (ocaml/ocaml-lsp#1075)

- Fix connecting to dune rpc on Windows. (ocaml/ocaml-lsp#1080)

## Features

- Add "Remove type annotation" code action. (ocaml/ocaml-lsp#1039)

- Support settings through `didChangeConfiguration` notification (ocaml/ocaml-lsp#1103)

- Add "Extract local" and "Extract function" code actions. (ocaml/ocaml-lsp#870)

- Depend directly on `merlin-lib` 4.9 (ocaml/ocaml-lsp#1070)
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.

Redundant "Type-annotate" action
2 participants