Implement sequential name lookup properly
ClosedPublic

Authored by mpickering on Apr 30 2017, 3:34 PM.

Details

Summary

Previously we would run all the monadic actions and then
combine their results. This caused problems if later actions
raised errors but earlier lookups suceeded. We only want to run later
lookups if the earlier ones fail.

Fixes Trac #13622

Diff Detail

Repository
rGHC Glasgow Haskell Compiler
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
mpickering created this revision.Apr 30 2017, 3:34 PM
bgamari added inline comments.May 1 2017, 11:42 AM
compiler/typecheck/TcRnExports.hs
595

Previously we considered `DisambigOcc _ mappend DisambigOcc _``` to be ambiguous. Now it seems we don't. This is presumably what fixed the bug, not the try logic above. Perhaps we should mention this in the commit message?

mpickering added inline comments.May 1 2017, 12:05 PM
compiler/typecheck/TcRnExports.hs
595

Actually no but this does fix another bug which is tested in the D3507. I thought I would also include it in this patch so that it is easy to merge into the 8.2 branch.

https://phabricator.haskell.org/D3507#aa527679

mpickering added inline comments.May 1 2017, 12:10 PM
compiler/typecheck/TcRnExports.hs
595

To be clear. This fixes the problem that if 2 UniqueOccurences combine into an AmbiguousOccurence then that doesn't throw away a DisambiguatedOccurence.

bgamari added inline comments.May 1 2017, 12:31 PM
compiler/typecheck/TcRnExports.hs
595

Okay, although it's not entirely clear to me why this change is correct in that case. Aren't two disambiguated occurrences ambiguous? If not, why? Perhaps this deserves a note?

This looks like a good example of when not to use Monoid, but at least some description of what the meaning of the monoid operation is would be a good thing.

This looks like a good example of when not to use Monoid, but at least some description of what the meaning of the monoid operation is would be a good thing.

How do you suggest refactoring the code? After thinking, it seems reasonable in this case.

mpickering updated this revision to Diff 12351.May 2 2017, 5:02 AM
  • Add comment
mpickering added a comment.EditedMay 2 2017, 5:03 AM

I added a comment about DisambiguatedOccurrence but it seems quite straightforward to me and the one intended use-site immediately precedes it in the code. Is there anything else you would like changing in this patch?

simonpj accepted this revision.May 2 2017, 6:29 AM
simonpj added a subscriber: simonpj.

Minor suggestions

compiler/typecheck/TcRnExports.hs
464

Can you explain why we want the first result?

467

Push lkup into here, so we get

res <- lookupExportChild parent (setRdrNameSpace bareName v)

Less higher order and complicated

472

Use NameNotFound here

600

Add an ASSERT( gre_name g == gre_name g' ) to support this comment

This revision is now accepted and ready to land.May 2 2017, 6:29 AM

I got stuck adding the ASSERT as it was causing inscrutable parse errors.

This revision was automatically updated to reflect the committed changes.