Remove `PatternSynonym` constructor from `Parent`.
ClosedPublic

Authored by mpickering on May 6 2016, 11:22 AM.

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.
There are a very large number of changes, so older changes are hidden. Show Older Changes

Thanks for the comments Ben. There is one part I don't think is right which I have left a comment on now for @adamgundry to look at when he gets the chance.

compiler/rename/RnNames.hs
1151

I am not sure this is actually right but I suspected it didn't matter. I couldn't trace where these FieldLabels ended up, maybe @adamgundry can comment?

The basic problem is that I need to return a FieldLabel but the lookup*_overloaded functions return FieldOcc Name which is slightly different.

I spoke with Simon about this and he helped simplify a lot of the stuff happening in RnEnv and to do with FieldLabel which I will get around to implementing shortly.

adamgundry added inline comments.May 16 2016, 12:19 PM
compiler/rename/RnNames.hs
1151

Sorry for the delay in getting back to you. Hopefully @simonpj has already helped clarify things, but just in case it helps, here are some quick thoughts...

I believe the first and third components are correct, but not the True. This should cache whether the selector was defined in a module with DuplicateRecordFields enabled (and should be equivalent to testing whether the OccName of the selectorFieldOcc is the same as the rdrNameFieldOcc). One way to get this information would be to return it from lookupSubBndrOcc_overloaded, as it is evident from the GRE (it is isJust (par_lbl (gre_par gre))). I don't know whether it actually matters, but if it doesn't that suggests FieldLabel is the wrong type to be returning from this function.

Perhaps all this can be simplified, though!

mpickering updated this revision to Diff 7690.May 21 2016, 5:48 PM
mpickering edited edge metadata.
  • Basic rip out
  • working
  • Add test files
  • tabs
mpickering updated this revision to Diff 7691.May 21 2016, 5:51 PM
mpickering edited edge metadata.
  • test

OK -- I've updated this based on Simon's comments. I think I've now spent over 20 hours on this refactoring so I'm not a bit burnt out from fiddling with this part of GHC for now.

The important changes are lookupExportChild and tcExports. lookupExportChild looks up names in export lists, disambiguating by the parent if
the lookup is ambiguous. I spent a lot of time tweaking the error messages in tcExports as well, hopefully people like them. They are probably better than the crappy ones from before anyway.

bgamari accepted this revision.May 24 2016, 4:45 AM
bgamari edited edge metadata.

@mpickering, a few last comments. If you have time feel free to take care of them otherwise I can take care of them when I merge this. Regardless I'll wait for a few more reviews to come in.

compiler/rename/RnNames.hs
1121

Perhaps just split this signature across two lines to stay in 80 columns.

compiler/typecheck/TcRnDriver.hs
2295

Perhaps you could take care of this?

This revision is now accepted and ready to land.May 24 2016, 4:45 AM
mpickering updated this revision to Diff 7709.May 24 2016, 5:22 AM
mpickering edited edge metadata.
  • Formatting

The new errors look much better. @mpickering thank you very much for persevering with this!

mpickering updated this revision to Diff 7711.May 24 2016, 5:54 AM
mpickering edited edge metadata.
  • Formatting and comments
  • comment
mpickering updated this revision to Diff 7713.May 24 2016, 6:09 AM
mpickering edited edge metadata.
  • rebase
bgamari requested changes to this revision.Jun 3 2016, 2:38 PM
bgamari edited edge metadata.

Unfortunately it seems that this regresses rather subtly during validation,

"inplace/bin/ghc-stage1" -hisuf hi -osuf  o -hcsuf hc -static  -O0 -H64m -Wall -fllvm-fill-undef-with-garbage    -Werror    -this-unit-id Cabal-1.25.0.0 -hide-all-packages -i -ilibraries/Cabal/Cabal/. -ilibraries/Cabal/Cabal/dist-install/build -ilibraries/Cabal/Cabal/dist-install/build/autogen -Ilibraries/Cabal/Cabal/dist-install/build -Ilibraries/Cabal/Cabal/dist-install/build/autogen -Ilibraries/Cabal/Cabal/.    -optP-include -optPlibraries/Cabal/Cabal/dist-install/build/autogen/cabal_macros.h -package-id array-0.5.1.1 -package-id base-4.9.0.0 -package-id binary-0.8.3.0 -package-id bytestring-0.10.8.1 -package-id containers-0.5.7.1 -package-id deepseq-1.4.2.0 -package-id directory-1.2.6.2 -package-id filepath-1.4.1.0 -package-id pretty-1.1.3.3 -package-id process-1.4.2.0 -package-id time-1.6.0.1 -package-id unix-2.7.2.0 -Wall -fno-ignore-asserts -fwarn-tabs -Wcompat -Wnoncanonical-monad-instances -Wnoncanonical-monadfail-instances -XHaskell98 -O -dcore-lint  -no-user-package-db -rtsopts -Wno-deprecated-flags     -Wnoncanonical-monad-instances  -odir libraries/Cabal/Cabal/dist-install/build -hidir libraries/Cabal/Cabal/dist-install/build -stubdir libraries/Cabal/Cabal/dist-install/build   -dynamic-too -c libraries/Cabal/Cabal/./Distribution/Compat/MonadFail.hs -o libraries/Cabal/Cabal/dist-install/build/Distribution/Compat/MonadFail.o -dyno libraries/Cabal/Cabal/dist-install/build/Distribution/Compat/MonadFail.dyn_o

libraries/Cabal/Cabal/Distribution/Compat/MonadFail.hs:7:1: warning: [-Wunused-imports]
    The import of ‘fail’ from module ‘Control.Monad.Fail’ is redundant

In this case fail is clearly used,

module Distribution.Compat.MonadFail ( MonadFail(fail) ) where

import Control.Monad.Fail (MonadFail(fail))
This revision now requires changes to proceed.Jun 3 2016, 2:38 PM

Ack! This easy to fix fortunately but I won't get to it for a week at least.

mpickering updated this revision to Diff 7942.Jun 11 2016, 4:34 PM
mpickering edited edge metadata.
  • Basic rip out
  • working
  • Add test files
  • tabs
  • test
  • Formatting
  • Formatting and comments
  • comment
  • Add test
  • Record usages

@bgamari I validated this locally now. Can I merge it?

Is there anything else holding back this patch?

simonpj edited edge metadata.Jun 16 2016, 11:09 AM

Mainly me I think. We had a very useful Skype conversation last time. Can we repeat the exercise? Monday?

mpickering planned changes to this revision.Jun 16 2016, 11:18 AM

Mainly me I think. We had a very useful Skype conversation last time. Can we repeat the exercise? Monday?

I am away for most of the next month but once I get back (towards the end of July) I'll post here again and we can arrange to speak.

mpickering updated this revision to Diff 8141.Jul 5 2016, 3:22 AM
mpickering edited edge metadata.
  • rebase

@simonpj

I am back now for this week. Do you want to schedule a time?

What is the current status of this, @mpickering?

Matthew and I had a call yesterday where we agreed a much simpler plan. He's going to work on it.

bgamari requested changes to this revision.Jul 20 2016, 4:50 AM
bgamari edited edge metadata.

Sounds good. Bumping out of the review queue for now in that case.

This revision now requires changes to proceed.Jul 20 2016, 4:50 AM
mpickering updated this revision to Diff 8413.Aug 14 2016, 7:32 AM
mpickering edited edge metadata.
  • Third iteration.

@simonpj This is ready for another review now. I've spent far too long doing this so I would prefer not to have to do another rewrite! We can talk about it next week on skype if you would like again. Highlights are:

  • A new module TcRnExports which contains all the code which typechecks and renames exports
  • Much better error messages
  • The main logic is in lookupChildrenExport and lookupExportChild. There is a new data type which records the result of the lookup ChildLookupResult which means we can handle different errors more explicitly than directly calling the error functions.

Part of the issue is that the logic baked into this part of GHC is very complicated. Now having written three different versions of this, it seems that this complexity is unavoidable...

compiler/rename/RnEnv.hs
7

remove

531

remove

mpickering updated this revision to Diff 8414.Aug 14 2016, 7:41 AM
mpickering edited edge metadata.
  • Remove whitespace

@simonpj I know you're very busy but do you know when you will be able to review this? There is no immediate rush on my end but I would like to close this out at some point, it has been rumbling on much longer than I originally anticipated!

bgamari requested changes to this revision.Sep 5 2016, 2:32 PM
bgamari edited edge metadata.

Bumping out of review queue while final things are sorted.

This revision now requires changes to proceed.Sep 5 2016, 2:32 PM

Whether to disambiguate the pattern synonym P or not depending on the type.

compiler/typecheck/TcRnExports.hs
533

Delete singleton case

581

Only export the right one.

617

Change name and update comment.

621

Change ns to something else.

628

Add comment about the two different types.

666

Push error message into branch.

670

Move to the start of the function and update note with bullet points.

mpickering added inline comments.Sep 12 2016, 10:05 AM
compiler/typecheck/TcRnExports.hs
419

Give some examples here.

447

To do with the namespaces explicitly inside the children list.

mpickering updated this revision to Diff 8747.Sep 21 2016, 9:04 AM
mpickering edited edge metadata.

Simon and I talked last week again about this which helped clean up the code some more.

There are still some problems with the error messages in corner cases
but I don't want to work on them any more. The error messages are much
better than what used to be there in any case.

mpickering planned changes to this revision.Sep 22 2016, 3:43 PM

Validate failed.

Good work! Let me know if you need help.

Simon

mpickering updated this revision to Diff 8760.Sep 23 2016, 5:00 PM
mpickering edited edge metadata.
  • Fix failing test

It validates!

This revision was automatically updated to reflect the committed changes.