Opened 4 years ago

Last modified 9 months ago

#10599 new bug

Template Haskell doesn't allow `newName "type"`

Reported by: meteficha Owned by: goldfire
Priority: normal Milestone: 8.10.1
Component: Template Haskell Version: 7.10.2-rc2
Keywords: Cc: jb55, k-bx
Operating System: Unknown/Multiple Architecture: Unknown/Multiple
Type of failure: None/Unknown Test Case:
Blocked By: Blocking:
Related Tickets: Differential Rev(s):
Wiki Page:

Description

Using type as a name is, of course, forbidden. OTOH, type_1 is allowed as a name.

However, using GHC 7.10.1 and ghc --make T.hs on files:

-- T.hs
{-# LANGUAGE TemplateHaskell #-}
module T where
import Q
test

-- Q.hs
module Q where

import Language.Haskell.TH

test :: Q [Dec]
test = do
  t <- newName "type"
  return
    [FunD t
      [Clause
         []
         (NormalB $ LitE $ CharL 't')
         []]]

Leads to the following error:

$ ghc --make -ddump-splices T.hs
[2 of 2] Compiling T                ( T.hs, T.o )

T.hs:6:1:
    Illegal variable name: ‘type’
    When splicing a TH declaration: ident_0 type_1 = type_1

The above example works fine for GHC 7.8.4, so it's a regression.

Reference: https://github.com/yesodweb/persistent/issues/412

Change History (23)

comment:1 Changed 4 years ago by GregWeber

I just tested on 7.10.2 (the docker image which uses the hvr ppa).

➜  th-bug  docker run --rm -v `pwd`:/proj -w /proj haskell:7.10.2 ghc T.hs
[1 of 2] Compiling Q                ( Q.hs, Q.o )
[2 of 2] Compiling T                ( T.hs, T.o )

T.hs:5:1:
    Illegal variable name: ‘type’
    When splicing a TH declaration: ident_0 type_1 = type_1
Last edited 4 years ago by GregWeber (previous) (diff)

comment:2 Changed 4 years ago by GregWeber

Priority: normalhigh
Version: 7.10.17.10.2-rc2

comment:3 Changed 4 years ago by goldfire

Milestone: 7.12.1
Owner: set to goldfire

Interesting. Part of the confusion here is that names generated by newName are different from other names you can use in Haskell. So when you say newName "type", the variable's name really is type, but because it comes from newName, GHC prints it specially, as it's different from any other variable named type. So what you're trying to do -- create a variable named type -- is objectionable. It's just that the pretty-printer makes it look more sensible.

On the other hand, there is no reason at all for TH to be picky about newName names. GHC got pickier about TH names between 7.8 and 7.10 because 7.8 allowed non-newName names that aren't allowed in Haskell, making variables that can't be referred to outside of TH. However, with newName, the whole point is that you can't refer to them outside of TH. So I think an improvement would be just to let newName be very liberal in what it accepts.

As I'm inclined to say that GHC is doing the Right Thing here (that is, rejecting a variable named type), it seems most sensible to wait until 7.12 to fix. But if this is ruining your day, speak up. I don't feel very strongly on this point, at all.

comment:4 Changed 4 years ago by ekmett

As a brief addendum:

You _could_ reference a mkName'd variable named type that was produced from within template-haskell from within the language before:

Foo.type parsed just fine as a qualified name.

(This has apparently happened in some code folks have out there using lens.)

Last edited 4 years ago by ekmett (previous) (diff)

comment:5 Changed 4 years ago by bgamari

It seems to me that we should be closing up the cases where type can be used as a name, not opening more of these holes. It would be quite confusing to the user if they end up with a situation where a they see an export named type, find that it can be imported without trouble if qualified, and then try to define another similarly named definition without of TH, only to find that they get a syntax error.

I'm not entirely sure what we want to do about the parsing issue, but it seems that TH's current behavior is what we would want here, no?

comment:6 Changed 4 years ago by GregWeber

So when you say newName "type", the variable's name really is type, but because it comes from newName, GHC prints it specially, as it's different from any other variable named type. So what you're trying to do -- create a variable named type -- is objectionable

This isn't the goal here. If it was, mkName would be used.

It seems to me that we should be closing up the cases where type can be used as a name, not opening more of these holes. It would be quite confusing to the user if they end up with a situation where a they see an export named type, find that it can be imported without trouble if qualified, and then try to define another similarly named definition without of TH, only to find that they get a syntax error.

Again, "type" is not being used for a name but instead to seed a new name

From a TH user perspective it seems that in 7.10 newName has become a leaky abstraction where I am forced to understand how GHC is internalizing these names in a way that is different than what is printed out.

As a library author, I can certainly work around this. But it isn't something I would think to test and I only found out about it when a user reported that their program stopped working on 7.10. So at this point there is no way for us to quantify the breakage it would cause. I would expect it to be fairly rare that a name gets into a TH function that is reserved as an identifier: for persistent this happens because it later adds a record field prefix to the name before generating a record field.

comment:7 in reply to:  5 ; Changed 4 years ago by goldfire

Replying to bgamari:

It would be quite confusing to the user if they end up with a situation where a they see an export named type,

Ah. Good point. I hadn't thought about exports. If newNames were used only as locals, we would be fine with a very liberal treatment for newName. But once we think about exporting, we do run into an issue.

Come to think of it, we really shouldn't allow exporting of names created with newName. As Greg has pointed out, there's no reason a user should assume that a newName "foo" creates a variable named foo. If we allow exporting, then newNames are leaky, indeed.

If we disallow exports, would that alleviate your concerns, Ben? At that point, we should really allow any (non-empty?) string as the argument for newName. I think Greg is spot on in suggesting that the argument to newName is merely a seed, not the name in question.

comment:8 Changed 4 years ago by ekmett

I likewise agree with Greg. I'd expect to be able to call newName with more or less anything.

comment:9 in reply to:  7 Changed 4 years ago by bgamari

Replying to goldfire:

If we disallow exports, would that alleviate your concerns, Ben? At that point, we should really allow any (non-empty?) string as the argument for newName. I think Greg is spot on in suggesting that the argument to newName is merely a seed, not the name in question.

This sounds fine to me.

comment:10 Changed 4 years ago by jb55

Cc: jb55 added

comment:11 Changed 4 years ago by thoughtpolice

Milestone: 7.12.18.0.1

Milestone renamed

comment:12 Changed 4 years ago by k-bx

Just to mention that it affected our codebase, because we use

$(makeLensesWith abbreviatedFields ''Geo)

And some fields happen to be named like "_fooType".

comment:13 Changed 4 years ago by goldfire

My first step toward fixing this is prohibiting export of newName names. But it strikes me that a good approach is to avoid adding system names to the GlobalRdrEnv. newName names are indeed system names, and I think, in general, users should never be able to refer to system names in code.

Is there a reason this is a bad approach?

comment:14 Changed 4 years ago by simonpj

That sounds plausible to me. Names you export should be civilised.

But how could they be exported anyway? You can't mention it in the export list, presumably.

comment:15 in reply to:  14 Changed 4 years ago by goldfire

Replying to simonpj:

But how could they be exported anyway? You can't mention it in the export list, presumably.

But you can. :(

{-# LANGUAGE TemplateHaskell #-}

module TestTH ( bar ) where

import Language.Haskell.TH

$( do bar <- newName "bar"
      return [ValD (VarP bar) (NormalB (ConE 'True)) []] )

They also get exported with an omitted export list.

comment:16 Changed 4 years ago by AlexanderThiemann

Just a side note: the same thing happens when you use instance instead of type. This is probably broken for all "reserved" identifiers?

comment:17 Changed 4 years ago by goldfire

Yes, that's right. I hope to fix this for 8.0, but time is short right now!

comment:18 Changed 4 years ago by bgamari

Milestone: 8.0.18.2.1

It seems pretty unlikely that this will happen for 8.0.1, although who knows what might happen.

comment:19 Changed 3 years ago by bgamari

Milestone: 8.2.18.4.1

It doesn't look like this will happen for 8.2 either.

comment:20 Changed 20 months ago by bgamari

Milestone: 8.4.18.6.1
Priority: highnormal

Nor will it happen for 8.4.

comment:21 Changed 15 months ago by bgamari

Milestone: 8.6.18.8.1

These won't be fixed for 8.6, bumping to 8.8.

comment:22 Changed 14 months ago by k-bx

Cc: k-bx added

comment:23 Changed 9 months ago by osa1

Milestone: 8.8.18.10.1

Bumping milestones of low-priority tickets.

Note: See TracTickets for help on using tickets.