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

Relax functor lint rules in ppx_version #16047

Open
wants to merge 3 commits into
base: compatible
Choose a base branch
from

Conversation

martyall
Copy link
Member

@martyall martyall commented Sep 13, 2024

Quoting from the original README

One linter rule is that stable-versioned modules cannot be contained in the result of a functor application. The reason is that the types might depend on the functor arguments, so that distinct functor applications yield different types with different serializations. Functors that take no module arguments are allowed to return stable-versioned modules.

This rule makes the following code illegal:

module Functor (X : sig type x val foo :int end) = struct
  type x = X.x
  
  let f = g X.foo
  
  [%%versioned
  module Stable = struct
    module V1 = struct
      type t = string
      let to_latest =  Fn.id
    end
  end]
end

This turns out to be overly conservative and can cause problems when writing/refactoring harmless code. Take for example zkapp_command.ml where you have versioned modules nested inside the top level. If you wanted to change Call_forest to a functor, you can't. Even if the functor's argument doesn't appear at all in the versioned module.

This PR changes the linting rule to enforce that it is only illegal if you access a functor's argument directly in types defined wia %%versioned extension if you are outside of an expression. I figure that it's allowed in an expression because AFAIK we are only using this as a method to version types for coherent serialization

@martyall martyall changed the base branch from develop to compatible September 13, 2024 18:32
@martyall
Copy link
Member Author

!ci-build-me

@martyall martyall marked this pull request as ready for review September 13, 2024 18:36
@martyall martyall requested a review from a team as a code owner September 13, 2024 18:36
| Lident s ->
StringSet.add acc s
| Ldot (m, s) ->
lident m (StringSet.add acc s)
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be lident m acc? We don't care about the RHS of the path at all IMO

Copy link
Member

Choose a reason for hiding this comment

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

Of course, this allows aliasing á la

module Make (A : S) = struct
  module B = A
  module C = struct
    module%versioned Stable = struct
      module V1 = struct
        type t = B.t * B.t
      end
    end
  end
end

so this really circumvents the protection. We should move all the versioning out of the main code anyway, so I don't really care at this point.

Copy link
Member Author

@martyall martyall Sep 13, 2024

Choose a reason for hiding this comment

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

Of course, this allows aliasing á la ...

yeah I also thought of this 😞

We should move all the versioning out of the main code anyway, so I don't really care at this point.

I'm assuming here you mean if you want to defined versioned types, they need to be in a standalone module that contains only type definitions? This would be nice since this system doesn't always compose well with vanilla ocaml, maybe a lot of work.

In the meanwhile, what is your most preferred/least hated option:

  1. Something like this PR and remain vigilant
  2. Remove the in_functor check entirely and remain more vigilant
  3. Separate out type definitions piecemeal, i.e. do it if I'm already working in there and it becomes an obstruction

@martyall
Copy link
Member Author

!ci-build-me

@georgeee
Copy link
Member

!ci-nix-me

@georgeee
Copy link
Member

Nix Ci was successful: https://buildkite.com/o-1-labs-2/mina-nix-experimental/builds/34#0191ef23-0638-4f01-86a9-a9a1b11d10a4

This includes execution of ppx_version tests (not part of our regular CI).

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.

3 participants