Replace haskell-src-exts with ghc-lib-parser - #39
Conversation
brandonchinn178
left a comment
brandonchinn178
left a comment
There was a problem hiding this comment.
Cursory skim looks fine to me; AI is usually pretty good at mechanical transformations like convertExpr
| convertExpr (HsProjection _ flds) = | ||
| Right (TH.ProjectionE (fmap (\(L _ (DotFieldOcc _ (L _ fld))) -> fieldLabelToString fld) flds)) | ||
| #endif | ||
| convertExpr _ = Left "Unsupported Haskell expression form in hpgsql's SQL quasi-quoter" |
There was a problem hiding this comment.
Nit: I'd recommend explicitly enumerating instead of wildcard so that when new expressions are added, you'll get alerted to it and decide if you want to support it or not
There was a problem hiding this comment.
I hope bug reports will come with the expression that failed to be parsed, but I followed your suggestion and found that the error messages we can show are much nicer when we list every language feature. Plus it really helped me see what is still unsupported, and led me to support a few more cases (and add tests as I go).
ea6c015 is a pretty good summary of what we still don't support, and I'm planning on adding support for at least let and do.
What else in that list do you think is worth supporting?
There was a problem hiding this comment.
Yes, that's another good reason to explicitly list out branches - it forces you to make a decision whether you want to support something or not
I'd think all the TH constructs could be represented with ghc-lib constructs. Is it just a matter of doing the work or not? If so, I think what you have is fine for now, and you can always implement incrementally over time if people ask
There was a problem hiding this comment.
It's just a matter of doing the work and maintaining it, I guess. But it would be nice to keep it relatively small - it's not that little in CPP macros and LOC, and it's not hard to float out expressions from the quasiquoter for users when necessary if they're "exotic".
"Exotic" is subjective, of course, but you and I agree on OverloadedRecordDot being nice to have, and I'm sure there's a lot more users would agree on since I expect most #{} and ^{} to be small Haskell expressions.
There was a problem hiding this comment.
Yep, makes sense to me. Maybe in the error message, add some detail around "define outside quasiquoter, raise an issue at github if support should be added"?
There was a problem hiding this comment.
Good idea, I reworded the error message with 23a67bf
| @@ -0,0 +1,322 @@ | |||
| {-# LANGUAGE CPP #-} | |||
| {-# LANGUAGE PackageImports #-} | |||
| {- FOURMOLU_DISABLE -} | |||
There was a problem hiding this comment.
Why use fourmolu if you just disable entire files 😅
There was a problem hiding this comment.
Fourmolu is choking on some of the files with CPP macros :(
hpgsql/src/Hpgsql/GhcParseExp.hs:173:9-10
The GHC parser (in Haddock mode) failed:
[GHC-58481] parse error on input `<-'
I read their github, and it seems support for CPP macros is quite limited.
I was able to narrow the range of lines of code with fourmolu disabled in FromThExtension.hs, but in this file failed after a few attempts, so left the entire file unformatted because it didn't feel worth the trouble.
But I'm not very knowledgeable of fourmolu. Do you know if there's a better way of doing this?
There was a problem hiding this comment.
It's fine for the extensions file, since it's basically one giant pattern match. But this file is a bit more involved. I would recommend just as much as possible, break out CPP into isolated functions and only disable fourmolu for that function as a whole, instead of the entire file.
There was a problem hiding this comment.
I'm not sure I'm too concerned about making this file amenable to fourmolu. At least not at the moment, and not for getting this PR to a mergeable state.
Because it's just formatting, not linting, so we're not risking any correctness problems, and because the amount of effort I might have to put in to achieve that doesn't feel worth it.
I tried pattern synonyms a little to remove some CPP macros from here, but I think they require COMPLETE pragmas to avoid producing warnings unless I replicate all the constructors (which change for each ghc-lib-parser version..), so I'm not seeing a simple way to get this file to be formattable, I'm afraid.
| convertPat (AsPat _ (L _ rdr) _ (L _ p)) = TH.AsP (rdrToName rdr) <$> convertPat p | ||
| #endif | ||
| convertPat (BangPat _ (L _ p)) = TH.BangP <$> convertPat p | ||
| convertPat _ = Left "Unsupported pattern form in hpgsql's SQL quasi-quoter. Please file a bug report at https://github.com/mzabani/hpgsql/issues if you want this." |
There was a problem hiding this comment.
The only wildcard matches now are on lists, but every constructor has been explicitly laid out.
This is just Haskell syntax for data constructors checking the first character. I understand it now.
These are literal numbers, so they're numbers as per the source code, which of course are well described by rationals.
This shows how the quasi-quoter, when unable to parse a Haskell
expression, treats it like a fragment of SQL.
This was scary at first, but is actually fine, as valid SQL will never
have #{ or ^{ inside not a string or an identifier.
It's not great that this happens, but I'm not sure there's anything we
can do other than detecting invalid but reasonably-Haskell-looking
expressions an err on them? Best not to do anything
… good At least on GHC 9.10 they have source line information
|
@brandonchinn178 thank you for your review thus far. I have reduced the scope a little bit and decided not to cover May I take a bit more of your time and ask you to take a look at this once more? |
2 participants
This should fix #36