Opened 5 years ago

Closed 3 years ago

#7484 closed bug (fixed)

Template Haskell allows building invalid record fields/names

Reported by: iustin Owned by:
Priority: normal Milestone: 7.10.1
Component: Template Haskell Version: 7.6.1
Keywords: Cc:
Operating System: Unknown/Multiple Architecture: Unknown/Multiple
Type of failure: None/Unknown Test Case: th/T7484
Blocked By: Blocking:
Related Tickets: #8750 Differential Rev(s): Phab:D431
Wiki Page:

Description

This is not really a bug, more like a unintuitive behaviour.

Due to a bug in my definitions, I was passing a name like "opTestDelay " (note extra space) to a TH splice builder, which ended up with:

data OpCode
  = OpTestDelay {opDelayDuration  :: Double,
                 opDelayOnMaster :: Bool,
                 opDelayOnNodes :: [Ganeti.Types.NonEmptyString],
                 opDelayRepeat :: Ganeti.Types.NonNegative Int}

Note the double space around the first record field. This results in the actual accessor functions having the space in the name, which makes them unusable from normal code.

This seems to be allowed as well in other TH constructs:

λ> runQ $ return (ValD (VarP (mkName "a ") ) (NormalB (LitE (IntegerL 5))) [])
ValD (VarP a ) (NormalB (LitE (IntegerL 5))) []

I think that names should not be allowed to contain invalid identifiers (that would make them non-usable in normal Haskell code), but I'm not sure - maybe TH is designed to allow you to shoot yourself in the foot indeed. Anyway, opening this bug just in case.

Tested and behaves the same both on 6.12 and 7.6.

Change History (12)

comment:1 Changed 5 years ago by simonpj

difficulty: Unknown

Yes I see that. What would you like? Should mkName fail (by calling error) when given an illegal name?

I wonder if some people might use an illegal name specificaly to avoid the danger of accidental capture? (Though you can always use newName for that.)

Perhaps it would suffice to reject spaces in names, becuase that is perhaps particularly confusing.

Simon

comment:2 in reply to:  1 Changed 5 years ago by iustin

Replying to simonpj:

Yes I see that. What would you like? Should mkName fail (by calling error) when given an illegal name?

Yes, I think that makes sense (and is appropriate).

I wonder if some people might use an illegal name specificaly to avoid the danger of accidental capture? (Though you can always use newName for that.)

That would be a very "ugly" way of solving the problem. Since newName exists and works well, I don't see a problem against moving to that (if anyone relies on such behaviour).

Perhaps it would suffice to reject spaces in names, becuase that is perhaps particularly confusing.

Indeed. I don't know how difficult is to decide whether a name is "correct" versus simply checking for spaces; ideally names should be well-formed, but if spaces are much easier to detect, doing just space-checks is already an improvement.

Thanks! Iustin

comment:3 Changed 5 years ago by igloo

The problem with doing the check in mkName is that it would still be possible to make a variable that starts with a capital letter, or a constructor that starts with a lower case letter.

comment:4 Changed 5 years ago by igloo

Milestone: 7.8.1

comment:5 Changed 5 years ago by igloo

See also #7667

comment:6 Changed 4 years ago by thoughtpolice

Milestone: 7.8.37.10.1

Moving to 7.10.1

comment:7 Changed 3 years ago by goldfire

Differential Rev(s): Phab:D431
Status: newpatch

comment:8 Changed 3 years ago by jstolarek

comment:9 Changed 3 years ago by Richard Eisenberg <eir@…>

comment:10 Changed 3 years ago by Richard Eisenberg <eir@…>

In da2fca9e2be8c61c91c034d8c2302d8b1d1e7b41/ghc:

Fix #7484, checking for good binder names in Convert.

This commit also refactors a bunch of lexeme-oriented code into
a new module Lexeme, and includes a submodule update for haddock.

comment:11 Changed 3 years ago by Richard Eisenberg <eir@…>

In 1b22d9f288cbf819be90ec42b254fb1b67dded2d/ghc:

Release notes for #1476, #7484.

comment:12 Changed 3 years ago by goldfire

Resolution: fixed
Status: patchclosed
Test Case: th/T7484
Note: See TracTickets for help on using tickets.