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

Various fixes and improvements to module linting #95

Merged
merged 11 commits into from
Jan 24, 2023

Conversation

Maelan
Copy link
Contributor

@Maelan Maelan commented Nov 1, 2022

This PR consists of incremental fixes and simplifications and is best reviewed per commit, with help from the (long) commit messages.

Although some of the bugs fixed were made obvious by the new type linter, they existed since the beginning of the Git history, years ago. The most notable bugfixes are:

  • the linting of module type of (was not working in signatures);
  • the linting of functor parameters (X: T) (was not working in signatures);
  • the linting of the functor construct (was not working except in one situation).

Test suite for module type of (comments indicate behavior before this PR):

(* approximately ok:
 *   - "module type of" linted as ocamlKeyword
 *   - "F" linted as ocamlModule
 *   - "(X)" linted meaninglessly but by chance it doesn’t look SO bad
 *)
module N : module type of F(X)

module M : sig
  (* not ok:
   *   - "module" linted as ocamlMPRestr3
   *   - "type" understood as starting a type definition
   *   - "of", "X" linted meaninglessly
   *)
  module N : module type of X
  (* not ok, failure made obvious by the new type linter:
   *   - "F(X)" understood as the beginning of "F(X).t" with dot missing
   *     => error linting, spectacular failure
   *)
  module N : module type of F(X)
  val x : int
end

Test suite for functor parameters:

module M : sig
  module F () (X : Ord) : sig end (* not ok *)
end
= struct
  module F () (X : Ord) : sig end = struct end (* ok *)
end

module F () (X : Ord) : sig end = struct end (* ok *)

Test suite for functor:

module M : sig
  module type F
    = functor () (X : Ord) -> sig end (* not ok *)
  module F
    : functor () (X : Ord) -> sig end (* ok *)
end
= struct
  module type F
    = functor () (X : Ord) -> sig end (* not ok *)
  module F
    : functor () (X : Ord) -> sig end (* not ok *)
    = functor () (X : Ord) -> struct end (* not ok *)
end

module type F
  = functor () (X : Ord) -> sig end (* not ok *)
module F
  : functor () (X : Ord) -> sig end (* not ok *)
  = functor () (X : Ord) -> struct end (* not ok *)

This PR closes

Note: the module linting stuff has more bugs that I did not care fixing, because I am not in mood for rewriting it from scratch. I cannot figure out the purpose of several bits, neither if some bits are astute hacks, obsolete stuff or just mistakes. For instance, issue #6 remains unsolved; I saw which part of the code causes it, it seems to be done on purpose?

- do not require space before `functor`
- do not match `functorx` as `functor x`

For precedence reasons, `ocamlMPRestr2` must then be declared after
`ocamlMPRestr3`.
- do not require space before contained `sig`
- do not match `sigx` as `sig x`

For precedence reasons, `ocamlMPRestr1` must then be declared after
`ocamlMPRestr3`.
for precedence reasons, ocamlSig must be declared after ocamlMPRestr3
also, minor fix: do not require a character before `struct` when nested
in parentheses; this fixes the following example:

    open F(
    struct end
    )
The `module` keyword is now treated uniformly. Previously, it was
matched differently in signatures (matchgroup `ocamlModSpec`) than
elsewhere (matchgroup `ocamlModule`). Both cases have almost identical
syntax:

    (* in signatures: *)
    module M (X1 : T1) … (Xn: Tn) : MODULE TYPE
    (* in structures: *)
    module M (X1 : T1) … (Xn: Tn) [: MODULE TYPE] = MODULE DEF

The case distinction was not taking profit of the small difference in
syntax, and it was creating complexity and mismatch between both cases.
Each case had shortcomings.

In signatures, functor parameters like this where not highlighted
corrrectly:

    (* in signatures: *)
    module M (X1 : T1) : …

Also, the only case where the `functor` keyword was highlighted
correctly was in a `module` type annotation *in a signature*. It was not
highlighted correctly in `module` type annotations *in structures*, nor
in `module` definitions, nor in `module type` definitions.

    (* in signatures: *)
    module type T = functor (X1 : T1) -> …
    module M : functor (X1 : T1) -> …  (* the only case that was working *)
    (* in structures: *)
    module type T = functor (X1 : T1) -> …
    module M : functor (X1 : T1) -> … = …
    module M = functor (X1 : T1) -> …

These bugs are now fixed. The `ocamlModule` matchgroup subsumes the
features of both former cases (`module` in signatures / in structures).
The `functor` keyword is no more "contained", so that it now matches in
all of the 5 situations above.
@rgrinberg rgrinberg requested a review from copy November 2, 2022 01:21
@copy
Copy link
Collaborator

copy commented Nov 3, 2022

Thanks!

I tested this against some files by diffing the output of :TOhtml and found some things look slightly worse than before:

module F : Bar = Foo
module Test : Make(Int).S = struct end

(Foo, Int and S are highlighted as constructors).

Besides that, this is a clear improvement, and I hope we can get this merged.

I cannot figure out the purpose of several bits, neither if some bits are astute hacks, obsolete stuff or just mistakes. For instance, issue #6 remains unsolved; I saw which part of the code causes it, it seems to be done on purpose?

It should be fine as long as the highlighting doesn't fail catastrophically (as in #91). After all, it looks like a strict improvement with this patch (at least until Simon discovers some new alien syntax :-))

@copy copy merged commit 4c907ef into ocaml:master Jan 24, 2023
@copy
Copy link
Collaborator

copy commented Jan 24, 2023

I'll merge this, as it's clearly better than before.

@Maelan
Copy link
Contributor Author

Maelan commented Jan 29, 2023

Great, thank you!

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.

2 participants