add -print-variance and Asttypes.Bivariant by t6s · Pull Request #13820 · ocaml/ocaml

t6s · 2025-02-20T13:37:32Z

This PR adds a new command-line option -print-variance that lets the compiler print the variance of every type.
Types parameters like 'a in type 'a t = A are bivariant (assured to have no occurrence of the type parameter);
the bivariance is now printed as +- (like type +-'a t = A).
For the consistency between the printed result and the syntax, the parser is modified too.

Coauthored by @garrigue

Octachron · 2025-02-20T14:20:09Z

The support for

type +-'a t

could also be split in its own PR.
Who are the intended users of the new flag? If this is a debugging flag intended for compiler developers,
maybe -dprint-variance would be a better name?

t6s · 2025-02-20T14:33:28Z

This is a debugging flag, and -dprint-variance will certainly a better name.
Is it also okay then to print something that cannot be parsed?
Then the parser change can be totally omitted.

garrigue · 2025-02-20T14:38:27Z

The difference with usual debugging flags is that its output is intentionally made compatible with the parser. This is not the case for the -d... Flags.
So I would rather define it as a more accurate way to print type definitions, just too verbose to be the default.
We probably need to add some tests too.

Octachron · 2025-02-20T14:39:30Z

The -i option (and error messages) may already print non-parsable output -i in presence of signature avoidance, thus I think it would be fine for a debugging flag to add some more non-parsable output.

At the same time, I think that supporting +- on its own for the extra-explicitness might be worthwhile on its own.

t6s · 2025-02-20T15:04:00Z

Summing up the opinions, may I leave this PR as is for reviewing, if it does not take seriously longer than two PRs?

t6s · 2025-02-20T15:13:19Z

For tests, what about enabling -print-variance in typing-misc/injectivity.ml?

Octachron · 2025-02-20T15:16:27Z

Just splitting the commit in two is enough for me, there is no strong need to split the PR — if we agree on the name.

What about -i-variance or -i-show-variance to make it easy to discover the option next to -i?

For the tests, it might make sense to enable the option globally in expect.ml, since any changes of the inferred variances in the testsuite is probably worth a second look.

t6s · 2025-02-20T15:25:46Z

I did not know about -i (and that is for "inferred"); it looks reasonable.
I am going to do renaming (-i-variance) and split commits.

t6s · 2025-02-20T15:26:53Z

After splitting, I will try turning on the option in expect.ml.

t6s · 2025-02-20T15:57:54Z

Splitting done, but I am afraid that most changes went into the first commit.

t6s · 2025-02-20T15:59:31Z

      (fun (c,n,i) ->
        let i = if check_injectivity then i else false in
-        if c || n then (c,n,i) else (true,true,i))
+        if c <> n then (c,n,i) else (not c,not n,i))


This line seems to be the most tricky change.
Jacques and I could not find an immediate way to refactor this part.

To be more precise, c and n in required reflect + and - in the syntax, not the corresponding variance bits, so a conversion is needed here.
It would be nicer to do the conversion earlier, but other parts of the code depend on this meaning, so this should be a separate refactoring.

What about writing the conversion

(* c and n reflects respectively + and - in the syntax, and maps respectively to `not May_neg` and `not May_pos` in the {!Types.Variance.f} fields *) not n, not c, i

?

👍
Thank you so much for a careful reading and a nice cleaning.

garrigue · 2025-02-20T23:25:13Z

I'm not sure we want to turn this on on all expect tests at once, since this will require reviewing individually all the expect tests for changes.
For now, I suggest turning -i-variance on only in typing-misc/injectivity.ml and typing-misc/variance.ml.
It should be enough to test the feature, and this is were it matters most.

If we turn this on, I would suggest doing it in yet another PR.

Octachron · 2025-02-21T16:47:12Z

 and use_runtime = ref ""                (* -use-runtime ... *)
 and plugin = ref false                  (* -plugin ... *)
 and principal = ref false               (* -principal *)
+and print_variance = ref false          (* -print-variance *)


I would propose to move the definition just below print_types (and to update the comment).

Thank you for spotting. I will fix it.

Octachron · 2025-02-21T16:52:45Z

      { if $1 = "+!" then Covariant, Injective else
        if $1 = "-!" then Contravariant, Injective else
+        if $1 = "+-" then Bivariant, NoInjectivity else
+        if $1 = "+-!" then Bivariant, Injective else


Do we have a good reason and mnemonic to allow only +- and not -+?

I thought it would be better to minimize the change to the syntax, and did not think that the symmetry here could help a lot of users. Do you have other opinions?

I would tend to prefer allowing any permutation of ± and !, and to consider that ± can be written either +- or -+.
In other words, adding +-, -+, +-!, -+!, !+-, and !-+ (but not +!- nor -!+).

Okay, my preference was anyway not strong, and I am happy to follow your suggestions. I will add -+ and -+!.
(Btw, at the first attempt Jacques and I considered + -! +- !, etc. too, and soon realized that the list will become huge :)

t6s · 2025-02-22T11:54:45Z

I'm not sure we want to turn this on on all expect tests at once, since this will require reviewing individually all the expect tests for changes.

For now, I suggest turning -i-variance on only in typing-misc/injectivity.ml and typing-misc/variance.ml.

It should be enough to test the feature, and this is were it matters most.

If we turn this on, I would suggest doing it in yet another PR.

The change in the tests becomes indeed large. What do you think about this, @Octachron ?

Octachron · 2025-02-22T12:07:02Z

I have mostly finished to review the changes in the testsuite, so I am fine either way.

Octachron · 2025-02-22T12:15:24Z

      (fun (c, n, i) -> make (not n) (not c) (not abstract || i))
      required
-  else begin
+  | _ -> (* TODO: list every type_kind *)


Do you plan to implement this TODO in this PR or a later one?

Let me do it in this PR.

t6s · 2025-02-22T12:31:59Z

I have mostly finished to review the changes in the testsuite, so I am fine either way.

Thank you so much, and I am happy to know that variances have been correctly inferred in other tests.
I can as well understand Jacques' concern that the changes could interrupt other PRs.

What about adding -no-i-variance and set it in existing tests, while keeping expect.ml to default to -i-variance?

t6s · 2025-02-22T13:37:58Z

turn this on on all expect tests at once, since this will require reviewing individually all the expect tests for changes. For now, I suggest turning -i-variance on only in typing-misc/injectivity.ml and typing-misc/variance.ml. It should be enough to test the feature, and this is were it matters most.

If we turn this on, I would sugge

Sorry, I have been reading wrongly Jacques' comment. There is no concern about the testsuite after Florian's review. Let us not do "-no-i-variance".

t6s · 2025-02-22T14:05:06Z

rebased

Octachron · 2025-02-24T14:12:35Z

  val _absname : unit -> unit
  val _no_absname : unit -> unit
  val _alert : string -> unit
-  val _i_variance : unit -> unit


I am not sure why you moved the option, having -i-variance available in the toplevel seemed useful to me.

Oops, I misunderstood about the Common_options (that it were controlling only the usage output).
I will revert this change.

Octachron · 2025-02-24T14:31:17Z

+     ( Type_abstract _
+     | Type_record _
+     | Type_variant _
+     | Type_open ); type_manifest = _} ->


I would propose to split the case in two patterns to make it commutative with the previous case:

| { type_kind = _; type_manifest = Some _ } | { type_kind = Type_record _ | Type_variant _; type_manifest = _ } ->

Octachron · 2025-03-06T15:15:24Z

After rereading the testsuite, I fear that my opinion has soured a bit on the size of the testsuite change. Thus I would propose to not set -i-variance in the expect tool, but rather set it test by test, using the

(* TEST
 flags = "-I ${ocamlsrcdir}/utils";
 expect;
*)

Clflags.print_variance := true ;;

trick.

Looking back at the syntax, I was wondering if there were other possible options.

Some quite bad options:

= for denoting invariance: but type ='a t = 'a -> 'a is completely unreadable
? for bivariance, but ? is already used for option, and it could be interpreted as not-!: type +-?'a t = int
0 for bivariance, type 0'a t is hard to read in any font that does not distinguish strongly between O and 0. And a number has too many interpreations.

An ok-ish option suggested by @samsa1 :

~ for bivariance type ~'a t, with the disadvantage that ~ is associated with label, even if it is never used with this meaning in a type context.

Thus, even after looking at other options, I still think that +-/-+ is the most discoverable syntax.

Octachron · 2025-03-06T15:18:12Z

+     type !'a t       : may_pos + may_neg + inj
+     type +!'a t      : may_pos + inj
+     type -!'a t      : may_neg + inj
+     type +-!'a t      : inj


The alignment on : is broken here.

t6s · 2025-03-13T11:49:23Z

I have removed three commits that was enabling -i-vaiance for all tests,
adding instead flags = -i-variance in typing-misc/{variance, injectivity}.ml.

I did not know and just have learned the trick to import Utils and set Clflags directly in tests.
I simply set flags in the above two files since they are about variances.
Are there any other tests that should be run with -i-variance ?

Octachron · 2025-03-13T12:04:15Z

I think that the typing-misc/constraints.ml test could be also useful since the interaction of constraints with variance is non trivial.

t6s · 2025-03-13T14:52:56Z

Added -i-variance in constraints.ml.
Seeing the resulting output, I wonder if I could add variance info also to error messages.

Octachron

I agree that the addition of a surface language for bivariance is natural, and the -i-variance option can be useful both for debugging and learning about variance.

@t6s, do you wish to clean-up the history (I would suggest to keep at least an -i-variance commit, a +- commit and a test commit)?

t6s · 2025-03-14T15:26:46Z

Thank you for reviewing @Octachron . I will soon push squashed 3 commits following your suggestion.

yallop · 2025-08-22T09:37:50Z

It could be useful to have -i-variance affect the output of -dsource, too.

t6s · 2025-08-25T02:05:12Z

Could be. But a problem is that -dsource prints the result of parsing before typechecking.
A suggestion is to introduce -dtypedsource that pretty prints the corresponding typed tree
using Untypeast.untype_structure.

t6s · 2025-08-25T02:16:19Z

I have quickly checked that Untypeast can do that as intended.
(and found that -i-variance is not doing its job for parameters of classes; this needs to be fixed)

t6s mentioned this pull request Feb 20, 2025

Add a new type kind Type_external #13712

Merged

t6s force-pushed the print_variance branch from dcb94ab to ef275c3 Compare February 20, 2025 15:50

t6s commented Feb 20, 2025

View reviewed changes

Octachron reviewed Feb 21, 2025

View reviewed changes

Octachron reviewed Feb 22, 2025

View reviewed changes

t6s force-pushed the print_variance branch 2 times, most recently from 182fcf8 to f7be37f Compare February 22, 2025 14:04

Octachron reviewed Feb 24, 2025

View reviewed changes

t6s force-pushed the print_variance branch from f7be37f to 39f9997 Compare February 24, 2025 14:51

Octachron added the parsetree-change Track changes to the parsetree for that affects ppxs label Mar 3, 2025

dra27 assigned Octachron Mar 5, 2025

Octachron reviewed Mar 6, 2025

View reviewed changes

t6s force-pushed the print_variance branch from 2564a9f to b3196fb Compare March 13, 2025 11:44

t6s force-pushed the print_variance branch from b3196fb to ca56553 Compare March 13, 2025 11:59

Octachron approved these changes Mar 14, 2025

View reviewed changes

t6s added 3 commits March 15, 2025 00:24

add -i-variance

497f7fb

add the syntax for bivariance

f8a52fe

enable -i-variance in tests for variance and constraints

314018b

t6s force-pushed the print_variance branch from c3ecee0 to 314018b Compare March 14, 2025 15:27

Octachron merged commit 156ddad into ocaml:trunk Mar 18, 2025

t6s added a commit to COCTI/ocaml that referenced this pull request Jul 3, 2025

explain the syntax change for variance (by ocaml#13820)

25ee595

t6s mentioned this pull request Jul 3, 2025

explain the syntax change for variance (by #13820) #14126

Merged

stedolan mentioned this pull request Aug 21, 2025

Bad variance check with private aliases #14200

Closed

t6s added a commit to COCTI/ocaml that referenced this pull request Sep 9, 2025

explain the syntax change for variance (by ocaml#13820)

49f9c37

garrigue pushed a commit that referenced this pull request Sep 11, 2025

explain the syntax change for variance (by #13820) (#14126)

5364370

t6s mentioned this pull request Oct 23, 2025

enable -i-variance for classes and extension constructors, and describe -i-variance in manpages #14315

Merged

314eter mentioned this pull request Apr 18, 2026

OCaml 5.4 and 5.5 fixes tree-sitter/tree-sitter-ocaml#135

Merged

Uh oh!

Conversation

t6s commented Feb 20, 2025

Uh oh!

Octachron commented Feb 20, 2025

Uh oh!

t6s commented Feb 20, 2025

Uh oh!

garrigue commented Feb 20, 2025

Uh oh!

Octachron commented Feb 20, 2025

Uh oh!

t6s commented Feb 20, 2025

Uh oh!

t6s commented Feb 20, 2025

Uh oh!

Octachron commented Feb 20, 2025

Uh oh!

t6s commented Feb 20, 2025

Uh oh!

t6s commented Feb 20, 2025

Uh oh!

t6s commented Feb 20, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

garrigue Feb 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

garrigue commented Feb 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Octachron Feb 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

t6s commented Feb 22, 2025

Uh oh!

Octachron commented Feb 22, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

t6s commented Feb 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

t6s commented Feb 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

t6s commented Feb 22, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Octachron Feb 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Octachron commented Mar 6, 2025

Uh oh!

Choose a reason for hiding this comment

garrigue Feb 20, 2025 •

edited

Loading

garrigue commented Feb 20, 2025 •

edited

Loading

Octachron Feb 22, 2025 •

edited

Loading

t6s commented Feb 22, 2025 •

edited

Loading

t6s commented Feb 22, 2025 •

edited

Loading

Octachron Feb 24, 2025 •

edited

Loading

Octachron commented Mar 13, 2025 •

edited

Loading