5.1 migrations: Add tests for generative functors

[pitag-ha]

When we added the migrations for the Parsetree change for generative functor application, we didn't have a test mechanism yet to test the migrations. Now that we have one, I've added those tests.

It turns out that the migrations aren't correct: One of the tests shows that our migrations turn

F ()

into

F (struct end)

That bug is user-facing because the latter triggers a warning on 5.1. I've just opened in issue to keep track of that: #433

[tmattio]

Thanks for the descriptive issue, the pointers to the upstream PR and the test @pitag-ha, it was all very useful!

My understanding of what needs to be done is:

• From 501 to 500, F () -> F (struct end)
• From 500 to 501, F (struct end) -> F (struct end [@ocaml.warning "-73"])

Is it correct? If so, here's a patch to do this (I'm unsure about the effect of not having the locations in the attribute though):

diff --git a/astlib/migrate_500_501.ml b/astlib/migrate_500_501.ml
index bb26e4f3..572d399a 100644
--- a/astlib/migrate_500_501.ml
+++ b/astlib/migrate_500_501.ml
@@ -750,6 +750,40 @@ and copy_module_expr_desc :
   | Ast_500.Parsetree.Pmod_functor (x0, x1) ->
       Ast_501.Parsetree.Pmod_functor
         (copy_functor_parameter x0, copy_module_expr x1)
+  | Ast_500.Parsetree.Pmod_apply
+      ( x0,
+        ({ pmod_desc = Pmod_structure []; pmod_loc = _; pmod_attributes = _ } as
+        x1) ) ->
+      let x1 = copy_module_expr x1 in
+      let Ast_501.Parsetree.{ pmod_desc; pmod_attributes; pmod_loc } = x1 in
+      let pmod_attributes =
+        Ast_501.Parsetree.
+          {
+            attr_name = { txt = "ocaml.warning"; loc = Location.none };
+            attr_payload =
+              PStr
+                [
+                  {
+                    pstr_desc =
+                      Pstr_eval
+                        ( {
+                            pexp_desc =
+                              Pexp_constant
+                                (Pconst_string ("-73", Location.none, None));
+                            pexp_loc = Location.none;
+                            pexp_loc_stack = [];
+                            pexp_attributes = [];
+                          },
+                          [] );
+                    pstr_loc = Location.none;
+                  };
+                ];
+            attr_loc = Location.none;
+          }
+        :: pmod_attributes
+      in
+      Ast_501.Parsetree.Pmod_apply
+        (copy_module_expr x0, { pmod_desc; pmod_attributes; pmod_loc })
   | Ast_500.Parsetree.Pmod_apply (x0, x1) ->
       Ast_501.Parsetree.Pmod_apply (copy_module_expr x0, copy_module_expr x1)
   | Ast_500.Parsetree.Pmod_constraint (x0, x1) ->

[pitag-ha]

Thanks for working on this, @tmattio!

My understanding of what needs to be done is:

• From 501 to 500, F () -> F (struct end)
• From 500 to 501, F (struct end) -> F (struct end [@ocaml.warning "-73"])

We don't need to add the warning to the AST. The compiler will trigger the warning automatically on 501 on F (struct end) (if F is generative).

Ideally, we don't want the migrations to change the parsing behavior at all, i.e. we want

• From 501 to 500, F () -> F () and F (struct end) -> F (struct end)
• From 500 to 501, F () -> F () and F (struct end) -> F (struct end)

However, I'm not sure whether that's possible: Possibly, F () and F (struct end) are represented the same way in the 500 AST. If that's the case, let's call that representation in 500 500_repr. In that case, you're lucky and the fix here would be easy: We'd want:

• From 501 to 500, F () -> 500_repr and F (struct end) -> 500_repr
• From 500 to 501, 500_repr -> F ()

PD: If it's the latter, then ppxlib will silence the new 73 warning until we bump the AST. To avoid that, we could do

• From 501 to 500, F () -> 500_repr and F (struct end) -> 500_repr_with_attr
• From 500 to 501, 500_repr -> F () and 500_repr_with_attr -> F (struct end)

I'd personally keep things simple and not do that, though.

[pitag-ha]

Oh, I forgot to mention: To find out whether F () and F (struct end) are represented the same way in 500, you can use utop -dparsetree on a 5.0 switch.

[tmattio]

Ah yes sorry I skipped the result of the detective work, assuming you already had the right migration in mind, let me expand on my message.

F () and F (struct end) are represented exactly the same way on 500.

For the 501 -> 500 migration, there's no question:

• F () -> F (struct end)
• F (struct end) -> F (struct end)

For the 500 -> 501 migration, we have two options:

1. F (struct end) -> F (struct end [@warning "-73"])
2. F (struct end) -> F ()

Con of (1.) is that F () becomes F (struct end [@warning "-73"])
Con of (2.) is that F (struct end) becomes F ()

Either way, we end up with a different syntax than we had at the beginning, so I am tempted to explore the option you listed and add attributes when down-migrating so we can restore the initial syntax. Could you expand on that idea? What attributes would you add?

[pitag-ha]

Right, I misread what you wrote.

There's another important point to take into account when comparing option 1. and option 2.: Which of the two keeps the two round trips 501 -> 500 -> 501 and 500 -> 501 -> 500 as close to the identity as possible? Under that criteria, option 2. is better:

Option 1.:

• The 501 -> 500 -> 501 round-trip is neither the identity on F () nor on F (struct end).
• Not even the 500 -> 501 -> 501 round-trip is the identity on F(struct end) (unless you add more logic to the down-migration, it adds the disable-warning attr). This means that we'd have to come back to these migrations when we bump the AST.

Option 2.:

• The 501 -> 500 -> 501 round-trip is the identity on F (). It's not the identity on F (struct end).
• The 500 -> 501 -> 501 round-trip is the identity on F(struct end).

So I'm still clearly in favor of option 2 between those two.

[pitag-ha]

Either way, we end up with a different syntax than we had at the beginning, so I am tempted to explore the option you listed and add attributes when down-migrating so we can restore the initial syntax. Could you expand on that idea? What attributes would you add?

You could use an attribute to distinguish the two 501 nodes also in 500:
501 -> 500 could do:

• F () -> F (struct end)[@generative]
• F (struct end) -> F(strcut end)

Then, 500 -> 501 could do:

• F (struct end)[@generative] -> F ()
• F (strcut end) -> F (struct end)

The downside would be that it means a bit more work:

• Once we bump the AST, we'd probably want to get rid of the attr to make 500 -> 501 -> 500 the identity and avoid unnecessary overhead.
• A few details to take into account, e.g. if you use a custom attr such as @genertive, you'd need to add it to our reserved namespace.

The upside would be that the 501 - 500 - 501 roundtrip (i.e. the roundtrip that happens before we bump the AST) wouldn't silence the new 73 warning. I expect that we'll take considerably longer to bump the AST this time than usual because silencing this warning would literally be the only user-facing change of the 501 - 500 - 501 roundtrip (unless I've missed something). So I'd say that doing this bit of work to avoid this might be worth it for us to be more confident in not bumping the AST, but it's not necessary. I leave it up to you :)

[tmattio]

So I'd say that doing this bit of work to avoid this might be worth it for us to be more confident in not bumping the AST, but it's not necessary. I leave it up to you :)

I agree, I'll do it with the attribute and the way you suggested, thank you!

[panglesd]

Just some small comments about this...

• Simply having 500 -> 501 do F(struct end) ⇒ F(struct end[@warning "-73"]) means that whenever ppxlib is used, warning 73 is never going to be raised.
  Indeed, a 501 -> 500 -> 501 migration would turn any F(struct end) into F(struct end[@warning "-73"]).
  That solution implies the least work, I don't know if cancelling warning 73 is a big deal or not...
• Having @pitag-ha's proposition:

501 -> 500 could do:

• F () -> F (struct end)[@generative]
• F (struct end) -> F(struct end)

Then, 500 -> 501 could do:

• F (struct end)[@generative] -> F ()
• F (struct end) -> F (struct end)

would take care of roundtripping 501 -> 500 -> 501, but would not work well with generated code. If a PPX generates F(struct end) during its (500) rewriting, the PPX won't add the [@generative], and a warning will be raised (unsolvable by the PPX user).

• So, another solution would be to remember the 501 nodes that need a warning:

501 -> 500 could do:

• F () -> F (struct end)
• F (struct end) -> F(struct end)[@keep_warning]

Then, 500 -> 501 could do:

• F (struct end)[@keep_warning] -> F (struct end)
• F (struct end) -> F () (or equivalently F (struct end) -> F (struct end[@warning "-73"]))

[panglesd]

The migrations tests for generative functors now work, apart from locations, however all locations are reasonable: they won't confuse the user too much.

[pitag-ha]

Nice, thanks a lot for picking this up and finding and implementing a good solution!

I have two small questions about locations below, but everything looks good (I can't officially Approve as I'm the theoretical author of the PR, but I would if I could).

However, more importantly: I assume the none Location reported in #433 (comment) came from the none Location in the emptry_struct. Do you also think so?

Btw, a test with some attributes (both in syntax and generated by e.g. metaquot) would make the tests even more complete.

[pitag-ha]

Why Location.none instead of { x1.pmod_loc with loc_ghost = true } like above?

[panglesd]

Because you are right! It should be { x1.pmod_loc with loc_ghost = true } like above

[pitag-ha]

Shouldn't this be a ghost location?

[panglesd]

Yes, I guess it should be...
Currently it is not really used (the upward migration will not use it) but when the ppxlib AST is bumped to 5.1, it will be used.
I think I don't know sure enough how tooling treats ghost locations.

[panglesd]

Actually, making it ghost creates one more divergence in the reverse_migrations.t test. So I guess it makes sense to leave it not ghost.

[pitag-ha]

I think I don't know sure enough how tooling treats ghost locations.

➕

[pitag-ha]

Some day (:tm:) I'll have a concrete look at what Merlin does.

[pitag-ha]

Btw, this PR started off as "Add tests" and now is "Fix bug". Can you add the changelog entry?

[panglesd]

Do you also think so?

I think so!

Thanks for the review! I answered the comments, rebased and added a changelog entry.

[pitag-ha]

Thank you! It all looks good to me (can't officially approve because I'm the "author"). Do you want to merge?

[panglesd]

I can approve!