search

ocaml-ppx / ppxlib

Base library and tools for ppx rewriters
MIT License
246 stars 98 forks source link

Fix 5.1 migrations for generative functor applications #433

Closed pitag-ha closed 7 months ago

pitag-ha commented 1 year ago

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:

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.

AllanBlanchard commented 1 year ago

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:
pitag-ha commented 1 year ago

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.

Octachron commented 1 year ago

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.

panglesd commented 1 year ago

@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.

panglesd commented 1 year ago

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!

AllanBlanchard commented 1 year ago

With this version, we do not have warnings anymore, so I cannot tell if the location is wrong ^^"

panglesd commented 1 year ago

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.

AllanBlanchard commented 1 year ago

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.

AllanBlanchard commented 7 months ago

This bug is fixed isn't it? From 0.31.0 the warning does not happen anymore.

NathanReb commented 7 months ago

Yes it was fixed in 0.31.0, see the first changelog entry here.