New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Avoid parse error occurred when the field name generated by TH matches any of Haskell keywords #1476
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for writing this up! One minor performance quibble.
At a future point, it would be nice to customize this behavior.
Since anyone writing type
as a field name currently will receive GHC error, I think we can safely call this a bugfix. Mind doing a patch version bump and adding a changelog notice?
persistent/Database/Persist/TH.hs
Outdated
avoidKeyword name = if name `elem` keywords then name ++ "_" else name | ||
|
||
keywords :: [Text] | ||
keywords = | ||
["case","class","data","default","deriving","do","else" | ||
,"if","import","in","infix","infixl","infixr","instance","let","module" | ||
,"newtype","of","then","type","where","_" | ||
,"foreign" | ||
] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lots of list lookups like this is pretty inefficient - would prefer to see a Set
lookup
avoidKeyword name = if name `elem` keywords then name ++ "_" else name | |
keywords :: [Text] | |
keywords = | |
["case","class","data","default","deriving","do","else" | |
,"if","import","in","infix","infixl","infixr","instance","let","module" | |
,"newtype","of","then","type","where","_" | |
,"foreign" | |
] | |
avoidKeyword name = if name `Set.member` haskellKeywords then name ++ "_" else name | |
haskellKeywords :: Set Text | |
haskellKeywords = Set.fromList | |
["case","class","data","default","deriving","do","else" | |
,"if","import","in","infix","infixl","infixr","instance","let","module" | |
,"newtype","of","then","type","where","_" | |
,"foreign" | |
] |
Floating it to the top-level ensures we aren't constructing the Set
for each call to mkRecordName
, though I would guess GHC is capable of performing that optimization on it's own
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your review, I changed the suggested part and added a notice to bump up a patch version in ChangeLog.
Co-authored-by: Matt Parsons <parsonsmatt@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
awesome, thanks! I'll release this today.
To avoid the problem that GHC fails compiling due to parse error when the field name generated by Template Haskell matches any of Haskell keywords, this PR fixes
mkRecordName
to suffix_
if the field name matches any of keywords.Before submitting your PR, check that you've:
@since
declarations to the Haddockstylish-haskell
on any changed files..editorconfig
file for details)After submitting your PR:
(unreleased)
on the Changelog