Closed ndmitchell closed 2 years ago
i confirm the sample program
infixr ++++
errors with ghc-9.2.1
❯ ghc -c -ddump-parsed-ast ~/rr.hs
==================== Parser AST ====================
ghc-9.2.1: panic! (the 'impossible' happened)
(GHC version 9.2.1:
Parser should only have RealSrcSpan
Please report this as a GHC bug: https://www.haskell.org/ghc/reportabug
the program,
infixr 9 ++++
however succeeds (producing an AST with a FixitySig
).
i'd say this is the span creeps in:
prec :: { Located (SourceText,Int) }
: {- empty -} { noLoc (NoSourceText,9) }
...
noLoc
gives rise to an UnhelpfulSpan UnhelpfulNoLocationInfo
which is the "other case" (not RealSrcSpan _
case) of SrcSpan
.
i observe that invoking the compiler such that it doesn't print the ast doesn't error e.g.
ghc -c -fforce-recomp ~/rr.hs
so, the parse itself seems successful.
So it seems we must be getting an AST tree back that has failures if we were to print it, and we then print it. It would be good to know where we are causing it to be printed, but this seems like a GHC bug? Can we report upstream? I wonder if a 9.2.2 is on the horizon or we need to find a workaround?
i'm afraid we need to drill down and find where this originates. i've verified the parse succeeds, it's in the hints but which one and where exactly?
If ddump-parsed-ast fails that suggests that some variant of pretty printing is bust? And given we can reproduce with GHC alone can we not report that?
if memory serves that function would be showAstData
. it is suggestive i agree and i can imagine us ending up reporting that it's broke but that doesn't bring us closer to understanding what's going wrong in hlint and how we might work around it.
If ddump-parsed-ast fails that suggests that some variant of pretty printing is bust? And given we can reproduce with GHC alone can we not report that?
thing is, i checked - hlint doesn't appear to make use of this function and substituting for unsafePrettyPrint
which it does use in a bunch of places doesn't sort the problem either so i'm really interested in where this is coming from. it might be printing (and we know we have found at least one GHC bug related to that) but it might not.
i'm very confident now that this is much deeper than just printing. in short, the presence of that UnhelpfulSpan
somehow breaks uniplate. this program is enough to demonstrate the error:
applyHintsReal :: [Setting] -> Hint -> [ModuleEx] -> [Idea]
applyHintsReal _ _ ms =
let cs = universeBi $ ghcModule (head ms) :: [LEpaComment]
in Debug.Trace.trace ("comments: " ++ show (length cs)) undefined
raised upstream issue 20846 for the showAstData
panic.
@alanz could use a little help here :)
Why showAstData
causes the panic? Could be flawed but I guess this is what's happening.
The rule in play is
| infix prec ops
{% checkPrecP $2 $3 >>
acsA (\cs -> sLL $1 $> $ SigD noExtField
(FixSig (EpAnn (glR $1) [mj AnnInfix $1,mj AnnVal $2] cs) (FixitySig noExtField (fromOL $ unLoc $3)
(Fixity (fst $ unLoc $2) (snd $ unLoc $2) (unLoc $1))))) }
where
prec :: { Located (SourceText,Int) }
: {- empty -} { noLoc (NoSourceText,9) }
| INTEGER
{ sL1 $1 (getINTEGERs $1,fromInteger (il_value (getINTEGER $1))) }
and noLoc
gives rise to an UnhelpfulSpan
.
You can see that in the construction of the FixSig
,mj AnnVal $2
buries a call to rs
in an EpaSpan
.
-- |Construct an AddEpAnn from the annotation keyword and the location
-- of the keyword itself
mj :: AnnKeywordId -> Located e -> AddEpAnn
mj a l = AddEpAnn a (EpaSpan $ rs $ gl l)
Later in showAstData
with NoBlankEpaAnnotations
a call to epaAnchor
causes the rs
to be evaluated
epaAnchor :: EpaLocation -> SDoc
epaAnchor (EpaSpan r) = parens $ text "EpaSpan" <+> realSrcSpan r
...
and the panic manifests.
I assume universeBi
triggers evaluation of r
in some analogous way.
@ndmitchell there is an obvious workaround. we can patch the following version of rs
into ghc-lib-parser
(via ghc-lib-gen) so that it no longer panics.
rs :: SrcSpan -> RealSrcSpan
rs (RealSrcSpan l _) = l
rs _ = badRealSrcSpan
badRealSrcSpan :: RealSrcSpan
badRealSrcSpan = mkRealSrcSpan bad bad
where
bad = mkRealSrcLoc (fsLit "ghc-lib-parser-nospan") 0 0
i've tested this end-to-end and it works.
so, if we were to go this way, we'd: (1) publish a new 9.2.1 ghc-lib-parser (and send the ghc-lib-gen patch https://github.com/digital-asset/ghc-lib/pull/342 for landing) (2) update hlint to
the last point is achieved by building hlint ascabal new-build --constraint "hlint +ghc-lib" --constraint "ghc-lib-parser-ex -auto -no-ghc-lib"
(the equivalent instack.yaml
is
flags:
hlint:
ghc-lib: true
ghc-lib-parser-ex:
auto: false
no-ghc-lib: false
) we can get this out the door pretty quickly. let me know if you want to execute on this.
OK, your analysis makes sense.
As to whether we should make a custom ghc-lib that works differently to GHC 9.2, my hope would be know. Let's wait and give the GHC devs a chance to weigh in.
For sure, we can't release HLint on 9.2.1 until GHC fixes this issue.
I will be posting a MR for the GHC bug shortly.
In the old days when the AST was riddled with undefineds, we added some rules to the SYB traversals we were using to tiptoe past them, by recognising the container and skipping child traversals if so. Perhaps something similar could be a workaround here?
See https://gitlab.haskell.org/ghc/ghc/-/merge_requests/7226 for the GHC Merge Request
i confirm that on GHC master with with 09b6cb45505c2c32ddaffcdb930fb3f7873b2cfc
landed, the issue is resolved.
Anyone know if a GHC 9.2.2 is likely? And if so, under what time frame?
And I guess will then have to make the choice as to whether we say HLint is incompatible with 9.2.1 (which seems a shame), or backport this to ghc-lib (which increases compile times).
What about the option of modifying the biplate to skip the undefined?
biplate
is based on Data
, so if Data
walks the undefined field, biplate
will too. The way biplate
works is to visit all possible types in advance, to build a more efficient traversal table, so it seems pretty difficult to avoid just this one undefined field.
Anyone know if a GHC 9.2.2 is likely? And if so, under what time frame?
Ben Gamari reports
"It should be out by mid-January. There are quite a few backports [1] that we need to push through and the focus recently has been on finalizing 9.0.2 but I'll be turning my attention to 9.2.2 after I the holiday.
Sounds cool - mid-January seems soon enough so no real reason to rush.
@ndmitchell GHC 9.2.2 has been out for a while. Is this the reason why a new hlint hackage release didn't happen yet? Perhaps it's ready now?
@LeventErkok various reasons mean I can't do it this week, but plan on doing so in a weeks time.
@ndmitchell Eagerly waiting!
Released!
Awesome! Thank you.
Given the sample file:
HLint head (GHC 9.2) fails with:
I think the reason is that the
rs
function in GHC demands everything produced by the parser has a real span. But if I pretty print the AST that code produces I see:So something has implicitly shoved some text in, and tried to get it out at some point, and its all gone wrong. I'm not really sure exactly where it goes wrong, but that looks like a GHC bug? A bit lost, but maybe @shayne-fletcher has thoughts?