Fix handling of unbound constructor names in TH #14627
ClosedPublic

Authored by mgsloan on Mon, Jul 2, 5:26 PM.

Details

Summary

Also adds a comment to UnboundVarE clarifying that it also is used for unbound
constructor identifiers, since that isn't very clear from the name.

Test Plan

testsuite/tests/th/T14627.hs

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.
mgsloan created this revision.Mon, Jul 2, 5:26 PM

One thing to note is that to get this expected output, https://phabricator.haskell.org/D4914 needs to be merged first.

Somebody once thought vName was enough there. Probably good to leave a comment saying why we need vcName. Otherwise, LGTM. Thanks!

mgsloan updated this revision to Diff 17178.Tue, Jul 3, 11:22 PM
  • Add a comment clarifying use of vcName

@goldfire Thanks for reviewing!

Somebody once thought vName was enough there. Probably good to leave a comment saying why we need vcName. Otherwise, LGTM. Thanks!

Good point, I've added a comment.

I think that probably vName seemed good enough previously since the constructor has Var in it. UnboundIdent or UnboundIdentifier would have potentially been clearer.

I have been doing some unrelated work using TH recently, and this unbound var stuff is quite a deviation from how TH used to work.

Now, if you do [e| contramap (+1) thing |], and you forgot to import Data.Functor.Contravariant, this will happily typecheck. The downstream user would then need to import it to get the name to resolve. To me it seems better to require [e| $(varE (mkName "contramap")) (+1) thing |] if the author desires this behavior.

Not sure what to do about this, because it could also be quite convenient to reference unbound variables in AST quotes. Maybe turn it into a special variety of warning that is implied by -Wall?

I have been doing some unrelated work using TH recently, and this unbound var stuff is quite a deviation from how TH used to work.

Now, if you do [e| contramap (+1) thing |], and you forgot to import Data.Functor.Contravariant, this will happily typecheck. The downstream user would then need to import it to get the name to resolve. To me it seems better to require [e| $(varE (mkName "contramap")) (+1) thing |] if the author desires this behavior.

Not sure what to do about this, because it could also be quite convenient to reference unbound variables in AST quotes. Maybe turn it into a special variety of warning that is implied by -Wall?

This is not directly related to this ticket, but I don't like the idea of a warning here. Warnings are for code that's likely wrong and, critically, can be fixed. If the user really wants an unbound variable in a quote (which is a reasonable desire) then there is no fix, short of using mkName in an interior splice, which is awfully roundabout. Though it's a painful deferment of what could be a static error into a dynamic one, you could always check the AST for UnboundVarE constructors... perhaps a better design is out there, though.

goldfire accepted this revision.Fri, Jul 6, 6:05 PM

Thanks!

This revision is now accepted and ready to land.Fri, Jul 6, 6:05 PM
mgsloan added a comment.EditedFri, Jul 6, 6:55 PM

Thanks for the feedback and diff approval!

(unrelated to this diff) TH expression names in AST quotes now have a lot of implicit complexity:

  1. Global identifiers are exact names
  2. Local identifiers use implicit lift
  3. Unbound identifiers use UnboundVarE and so are like mkName.

Seems very fiddly to me, but I don't feel very strongly about it now that I know about this caveat. I have put down a note in my TODO tracker to update the TH docs about this, because right now it only mentions the first two cases.

The main reason I think this is a bit dangerous is because it can be very easy to miss the difference between an exact name and an unbound name, if the tests happen to have the name in scope.

That said, I'm definitely more in favor of not spending much effort on mechanisms that TH code is correct. Most things can be caught as errors in the generated code.

Seems very fiddly to me, but I don't feel very strongly about it now that I know about this caveat. I have put down a note in my TODO tracker to update the TH docs about this, because right now it only mentions the first two cases.

Yes -- do update the docs around this. Thanks!

This revision was automatically updated to reflect the committed changes.