ocaml-ppx
Fix 5.1 migrations for generative functor applications
Our migrations for the Parsetree change for generative functor application are wrong: As one of the tests shows, they turn
F ()
into
F (struct end)
That bug is user-facing because the latter triggers a warning on 5.1.
The situation now is: When using ppxlib on 5.1, any generative functor application will trigger a waning: F (struct end) does so because it should (that's what the compiler change was about) and F () does so because our migrations are wrong.
I don't think fixing this is extremely urgent:
- It's a warning, not an error (hat's also why the health-checks haven't noticed this bug).
- Generative functors isn't a super widely used feature.
Still, the warning will be both very confusing and very hard to backtrack to a ppxlib bug for the few people who encounter it. So, as soon as one of us has a little bit of time, we should fix it and cut a ppxlib.0.30.1 patch release.
We have this bug in Frama-C. This is not a big problem since we can disable the warning for files where it happens. However, it took us quite some time to understand the reason why the warning appeared, in particular, the location of the error was:
File "_none_", line 1:
Thanks for the comment, @AllanBlanchard! It's important for us to know that this problem is hitting people in the real world.
in particular, the location of the error was: (...)
File "_none_", line 1:
That's bad! And good to know. We'll need to include a location check in the tests.
Unfortunately, we have very very little time for Ppxlib at the moment in general. On top of that, most of us are off during August, including me. We'll get back to this as soon as possible.
As far as I see, the correct change would be to change the upward 5.0 to 5.1 migration to
F(struct end) ⇒ F(struct end[@warning "-73"]) (which support was added to handle precisely this case). I can send a patch in this direction this week.
@tmattio @pitag-ha have discussed that and already opened a PR: https://github.com/ocaml-ppx/ppxlib/pull/432 I'll see if @tmattio is still willing to work on this.
I ended up doing it, directly in #432. (You are welcome to review, since you were willing to work on this, but no pressure!)
@AllanBlanchard if you pin ppxlib to #432, the location of the error should now make sense, hopefully. Please report if not!
With this version, we do not have warnings anymore, so I cannot tell if the location is wrong ^^"
Indeed! Thanks for trying.
The migration now "respects" the presence, or absence, of a warning.
So with module F(struct end) there will be a warning, with a location modified by the migration, but hopefully reasonable this time.
With module F() you won't get any warning.
For modules where we had (struct end) in the code, it was already correct AFAIK. But anyway, we indeed have the right warning when we write this. So I believe that everything is OK.
This bug is fixed isn't it? From 0.31.0 the warning does not happen anymore.
