Plinth: improve error reporting for unsupported Haskell features#7659
Plinth: improve error reporting for unsupported Haskell features#7659
Conversation
zliu41
force-pushed
the
zliu41/unsupported
branch
from
eff28eb to
12c380c
Compare
Unisay
left a comment
Unisay
left a comment
There was a problem hiding this comment.
First of all, I am happy that we're able to do this now.
I'd be even more happy to see some unit tests to prove this functionality works as intended and demonstrate how the error message looks like.
Unisay
left a comment
Unisay
left a comment
There was a problem hiding this comment.
A few suggestions — mostly style nits plus removing the unnecessary MonadIO additions (confirmed cabal build all passes without them).
| plugin = | ||
| GHC.defaultPlugin | ||
| { GHC.typeCheckResultAction = injectAnchors | ||
| { GHC.typeCheckResultAction = const . const $ injectAnchors >=> injectUnsupportedMarkers |
There was a problem hiding this comment.
Nit: named ignored arguments are more self-documenting than point-free const . const:
| { GHC.typeCheckResultAction = const . const $ injectAnchors >=> injectUnsupportedMarkers | |
| { GHC.typeCheckResultAction = \_cliArgs _modSummary -> injectAnchors >=> injectUnsupportedMarkers |
There was a problem hiding this comment.
This is a matter of style preference - const feels cleaner to me, at least in this particular instance.
There was a problem hiding this comment.
Agree, this is indeed a style / taste matter, my preference is driven by the desire to name explicitly the thing that is being discarded or ignored, as in:
"The CLI Args and Mod Summary are available in the context but are unused"
vs.
"There are two arguments that are unused"
There was a problem hiding this comment.
Ok, updated per your suggestion.
| plugin = | ||
| GHC.defaultPlugin | ||
| { GHC.pluginRecompile = GHC.flagRecompile | ||
| { GHC.typeCheckResultAction = const . const $ injectUnsupportedMarkers |
There was a problem hiding this comment.
Same nit here:
| { GHC.typeCheckResultAction = const . const $ injectUnsupportedMarkers | |
| { GHC.typeCheckResultAction = \_cliArgs _modSummary -> injectUnsupportedMarkers |
| , MonadReader r | ||
| , MonadQuote | ||
| , MonadWriter w | ||
| , MonadIO |
There was a problem hiding this comment.
MonadIO isn't needed here — cabal build all passes without it. The IO usage is in injectUnsupportedMarkers which runs in TcM, not in the compilation monad.
| , MonadIO |
There was a problem hiding this comment.
Good catch! I initially added some IO actions in compileExpr, but later removed them.
| , MonadState CompileState m | ||
| , MonadDefs LexName uni fun Ann m | ||
| , MonadWriter CoverageIndex m | ||
| , MonadIO m |
There was a problem hiding this comment.
Same as above — MonadIO isn't needed in the Compiling constraint. The marker extraction in Expr.hs is pure pattern matching.
| , MonadIO m |
zliu41
left a comment
zliu41
left a comment
There was a problem hiding this comment.
Right, I need to add some tests.
| , MonadReader r | ||
| , MonadQuote | ||
| , MonadWriter w | ||
| , MonadIO |
There was a problem hiding this comment.
Good catch! I initially added some IO actions in compileExpr, but later removed them.
| plugin = | ||
| GHC.defaultPlugin | ||
| { GHC.typeCheckResultAction = injectAnchors | ||
| { GHC.typeCheckResultAction = const . const $ injectAnchors >=> injectUnsupportedMarkers |
There was a problem hiding this comment.
This is a matter of style preference - const feels cleaner to me, at least in this particular instance.
zliu41
force-pushed
the
zliu41/unsupported
branch
from
e2147fd to
6c8f70c
Compare
2 participants
Added a
typeCheckResultActionthat detects unsupported Haskell features, and wraps offending exprs with a specific marker function. It currently detectsbase'sEq/Ord, andIOactions.This requires compiling Plinth using
plinthcorplcinstead ofcompile, sincecompileuses TH, and TH cannot install frontend plugins.Old error (it's much longer than this, and only a portion is shown):
New error: