Fix issue 503

[jchavarri]

Fixes #503.

In the first commit, notice the difference in the ending location of the Pexp_fun with the labelled bar.

In 5.1:

ppxlib/test/501_migrations/one_migration.t

Line 1726 in 4661a93

((pos_fname file.ml) (pos_lnum 1) (pos_bol 0) (pos_cnum 30)))

In 5.2:

ppxlib/test/502_migrations/one_migration.t

Line 719 in 4661a93

((pos_fname file.ml) (pos_lnum 1) (pos_bol 0) (pos_cnum 18)))

The second commit updates Migrate_502_501 so that the ranges of locations of Pexp_fun stay consistent with the behavior in 5.1.

[NathanReb]

Thanks for taking the time to report and fix this bug!

If I understand it correctly you should apply the same change to the Pexp_newtype node below as well!

The tests for the 501<->502 migrations are pretty unreadable in their current state unfortunately and I'd rather avoid adding more of those. I suggest you instead take an approach closer to how we test the ocaml.ppx.context migration (see the driver.ml, dune and run.t files). The idea is that you'd move both your tests to their own folder and write a custom no-op (as in, no actual transformaion) driver that run specific migrations and could here, display the location of specific bits of the AST, such as the Pexp_fun with the bar argument on the standard output.

I'm happy to help with the refactoring of those tests if needed so please let me know!

[jchavarri]

Hi @NathanReb thanks for the quick review :)

I applied the following changes:

• In b8e1170, I reverted the fix and added a small exe that checks the locations integrity for the given AST nodes
• In 6cc652c, I re-applied the fix and can see how the tests errors are gone

I also removed the previous location tests.

Please lmk if something like this is what you had in mind.

[NathanReb]

This is exactly what I had in mind, thanks for taking the time to refactor it and add the changelog entry!