uid_of_path can use the decl_id if shapes fail

[goldfirere]

Fixes #1699. Written with direction from @lpw25.

[goldfirere]

I don't know how best to test this within the merlin testsuite; advice welcome.

[voodoos]

Thanks for having a look.

For testing you should be able to reproduce your script in a single-file cram test by cd-ing and calling the compiler manually.
I wonder whether it could be simplified a bit since the hack.ml file only references the _intf modules?
You could also write a .merlin file to make the command line call to Merlin simpler.

(Also, it seems that two other tests are affected by your changes: in-implicit-trans-dep.t and ignore-kept-locs.t. )

[goldfirere]

Those new test case failures look (slightly) troublesome. Would you have a chance to look @voodoos? I don't know my way around this code very well and provided the fix in this patch only with @lpw25's guidance.

[voodoos]

I will have a look!

[voodoos]

I started looking at this and there is two different kinds of fall backing:

• If, looking for a definition, we fail to get an uid we fallback to looking the for the uid of the declaration. This fallback can also fail, as is the case in the broken test because it also relies on loading an external cmt.
• We can also fallback to a loc that is found by looking for the item in the environment (which rely only on the cmi file) and should also correspond to the declatation.

I'm wondering whether these two fallbacks lead to the same loc in which case we could only rely only on the second one which doesn't require the cmt files.

I will try to rework that mechanism and make it clearer!

[lpw25]

These two fallbacks will lead to the same location but the uid one has more information which enables it to distinguish between different files with the same name. The aim of this PR was to make sure we used the uid rather than the loc. The test shows that sometimes the uid one can fail due to missing files, so I guess we should first try the uid and then fallback again to the loc.

[voodoos]

Yes, if finding the definition's uid failed because of a missing cmt, looking for information about the declaration's uid will often fail similarly. I agree that we should fallback on the "lookup loc" if that happens. I will work on reproducing the issue in a test and fixing it cleanly.

[lpw25]

Sorry, to be clear, we want to fall back to the declaration's uid when the definitions uid could not be found, and only if that uid lookup failed should we use the loc.

[voodoos]

Yes, that's what I meant 🙂

[voodoos]

I pushed a commit that should fix the issue with all tests passing.
(I still need to add a test illustrating the initial issue)

@goldfirere can you check that it still fix your issue ?

[voodoos]

@goldfirere I added a test and force pushed to illustrate the impact of these changes.

There is still an issue though: when both files have exactly the same content, they have the same digest, we should probably salt the digest...

[lpw25]

FWIW the remaining issue a long-standing one that thankfully doesn't occur very often