Opened 11 months ago

Closed 8 months ago

#7918 closed bug (fixed)

SrcSpan's associated with expanded quasi-quotes are inconsistent

Reported by: edsko Owned by:
Priority: high Milestone: 7.8.1
Component: Compiler Version: 7.7
Keywords: Cc:
Operating System: Unknown/Multiple Architecture: Unknown/Multiple
Type of failure: None/Unknown Difficulty: Unknown
Test Case: quasiquotation/T7918 Blocked By:
Blocking: Related Tickets:

Description

Consider

{-# LANGUAGE TemplateHaskell #-}
module A where
import Language.Haskell.TH.Quote
qq = QuasiQuoter {
         quoteExp  = \str -> case str of
                                "a" -> [| True |]
                                "b" -> [| id True |]
                                "c" -> [| True || False |]
                                "d" -> [| False |]
       , quotePat  = undefined
       , quoteType = undefined
       , quoteDec  = undefined
       }


{-# LANGUAGE QuasiQuotes #-}
module B where
import A
ex1 = [qq|a|]
ex2 = [qq|b|]
ex3 = [qq|c|]
ex4 = [qq|d|]

In the expansion of [qq|a|] the source span for True is reported as 4:7-4:14 and 7:7-7:14 respectively -- i.e., the span of the entire quasi-quote. However, for the expansion of [qq|b|] and [qq|c|] the source span for id, True, False, and (||) are all reported as 5:11-5:14 / 6:11-6:14, i.e., starting at the "contents" of the quasi-quote.

Attachments (3)

T7918A.hs (461 bytes) - added by edsko 9 months ago.
T7918B.hs (119 bytes) - added by edsko 9 months ago.
T7918.hs (2.2 KB) - added by edsko 8 months ago.
Test case (fix silly misuse of SYB; fix unrelated to bug)

Download all attachments as: .zip

Change History (23)

comment:1 Changed 10 months ago by simonpj

  • Difficulty set to Unknown
  • Status changed from new to infoneeded

Can you check with 7.6 or HEAD? Looks ok with HEAD to me:

bash-3.1$ c:/code/HEAD/inplace/bin/ghc-stage2 -c T7918.hs -ddump-splices -fforce-recomp -ddump-rn -dppr-debug
Loading package ghc-prim ... linking ... done.
Loading package integer-gmp ... linking ... done.
Loading package base ... linking ... done.
Loading package array-0.4.0.2 ... linking ... done.
Loading package deepseq-1.3.0.2 ... linking ... done.
Loading package containers-0.5.0.0 ... linking ... done.
Loading package pretty-1.1.1.0 ... linking ... done.
Loading package template-haskell ... linking ... done.
T7918.hs:4:7:
    T7918.hs:4:7-13: Splicing expression
        {T7918.hs:4:11-13}
        "a"
      ======>
        {T7918.hs:4:11-13}
        GHC.Types.True{d}
T7918.hs:5:7:
    T7918.hs:5:7-13: Splicing expression
        {T7918.hs:5:11-13}
        "b"
      ======>
        {T7918.hs:5:11-13}
        GHC.Base.id{v} GHC.Types.True{d}
T7918.hs:6:7:
    T7918.hs:6:7-13: Splicing expression
        {T7918.hs:6:11-13}
        "c"
      ======>
        {T7918.hs:6:11-13}
        (GHC.Types.True{d} GHC.Classes.||{v} GHC.Types.False{d})
T7918.hs:7:7:
    T7918.hs:7:7-13: Splicing expression
        {T7918.hs:7:11-13}
        "d"
      ======>
        {T7918.hs:7:11-13}
        GHC.Types.False{d}

==================== Renamer ====================
nonrec {T7918.hs:7:1-13}
       main:T7918.ex4{v r0}
       main:T7918.ex4{v r0}
         = {T7918.hs:7:7-13}
           ghc-prim:GHC.Types.False{(w) d 68}
       <>
nonrec {T7918.hs:6:1-13}
       main:T7918.ex3{v r1}
       main:T7918.ex3{v r1}
         = {T7918.hs:6:7-13}
           (ghc-prim:GHC.Types.True{(w) d 6u}
            ghc-prim:GHC.Classes.||{v r1g} ghc-prim:GHC.Types.False{(w) d 68})
       <>
nonrec {T7918.hs:5:1-13}
       main:T7918.ex2{v r2}
       main:T7918.ex2{v r2}
         = {T7918.hs:5:7-13}
           base:GHC.Base.id{v r2q} ghc-prim:GHC.Types.True{(w) d 6u}
       <>
nonrec {T7918.hs:4:1-13}
       main:T7918.ex1{v r3}
       main:T7918.ex1{v r3}
         = {T7918.hs:4:7-13}
           ghc-prim:GHC.Types.True{(w) d 6u}
       <>

bash-3.1$ 

Consistently 7-13.

Changed 9 months ago by edsko

Changed 9 months ago by edsko

comment:2 Changed 9 months ago by edsko

As demonstrated by the attached example, the problem still persists in GHC HEAD. The output of the program is

Found HsVar GHC.Types.False	at T7918B.hs:7:7-13
Found HsVar GHC.Types.True	at T7918B.hs:6:11-13
Found HsVar GHC.Classes.||	at T7918B.hs:6:11-13
Found HsVar GHC.Types.False	at T7918B.hs:6:11-13
Found HsVar GHC.Types.True	at T7918B.hs:6:11-13
Found HsVar GHC.Classes.||	at T7918B.hs:6:11-13
Found HsVar GHC.Types.False	at T7918B.hs:6:11-13
Found HsVar GHC.Types.True	at T7918B.hs:6:11-13
Found HsVar GHC.Classes.||	at T7918B.hs:6:11-13
Found HsVar GHC.Types.False	at T7918B.hs:6:11-13
Found HsVar GHC.Types.True	at T7918B.hs:6:11-13
Found HsVar GHC.Classes.||	at T7918B.hs:6:11-13
Found HsVar GHC.Types.False	at T7918B.hs:6:11-13
Found HsVar GHC.Types.True	at T7918B.hs:5:11-13
Found HsVar GHC.Types.True	at T7918B.hs:5:11-13
Found HsVar GHC.Types.True	at T7918B.hs:4:7-13

Note the 7-13 rather than 11-13 in the first and last line. Note that this is the source span associated with the HsVar?, not with the larger expression (which is, indeed, consistent across the 4 quasi-quotes).

I will try and see if I can come up with a patch for this.

Changed 8 months ago by edsko

Test case (fix silly misuse of SYB; fix unrelated to bug)

comment:3 Changed 8 months ago by edsko

  • Status changed from infoneeded to new

comment:4 Changed 8 months ago by edsko

  • Owner set to edsko

comment:5 Changed 8 months ago by edsko

  • Version changed from 7.4.2 to 7.7

comment:6 Changed 8 months ago by simonpj

Thanks for taking this edsko. It can't be too hard!

Simon

Version 0, edited 8 months ago by simonpj (next)

comment:7 Changed 8 months ago by simonpj

  • Milestone set to 7.8.1
  • Priority changed from normal to high

comment:8 Changed 8 months ago by edsko

So here's what's happening: we have

runQuasiQuoteExpr :: HsQuasiQuote RdrName -> RnM (LHsExpr RdrName)

which executes a quasi-quote. Note that runQuasiQuoteExpr returns a located expression; it makes an attempt to set the location of the expanded quasi-quote to something sensible. However, runQuasiQuoteExpr gets called by rnExpr, which ignores the outermost location returned by runQuasiQuoteExpr:

rnExpr (HsQuasiQuoteE qq)
  = runQuasiQuoteExpr qq        `thenM` \ (L _ expr') ->
    rnExpr expr'

The result is that the outermost location is whatever location of the quasi-quote was, while all inner locations have been set by runQuasiQuoteExpr. That's why in the test case the case of a single True or False is different from a compound expression; in all cases the runQuasiQuoteExpr sets the location of all identifiers to the same thing, but in the case of a non-compound expression (a single True or False) this is then overwritten by rnExpr; for the compound expression it's the location of the compound expression (HsApp or HsPar) which gets overwritten instead.

Now, we can fix this by adding a special case to rnLExpr instead:

rnLExpr :: LHsExpr RdrName -> RnM (LHsExpr Name, FreeVars)
-- Don't ignore the new location set by runQuasiQuoteExpr (#7918)
#ifndef GHCI
rnLExpr (L _ e@(HsQuasiQuoteE _)) = 
  pprPanic "Cant do quasiquotation without GHCi" (ppr e)
#else
rnLExpr (L loc (HsQuasiQuoteE qq)) = setSrcSpan loc $
  runQuasiQuoteExpr qq `thenM` rnLExpr 
#endif  /* GHCI */
rnLExpr e = wrapLocFstM rnExpr e

I have done this and it works and it fixes the problem for the case of expressions. I've done it too for the case of types but there seems to be another location override in that case; still looking for that.

comment:9 Changed 8 months ago by edsko

  • Test Case set to quasiquotation/T7918

Second overwrite for types turned out to be caused by the handling for foralls. Changed this and added a note.

comment:10 Changed 8 months ago by simonpj

(I wrote this comment yesterday, but it has disappeared. Sigh.)

Edsko, putting a special case in rnLExpr is a bit undesirable because:

  • It makes rnLExpr more than a dispactch to rnExpr
  • It means that rnExpr will never see a HsQuasiQuoteE, so it is "missing a case"; there's an invariant that the HsQuasiQuoteE case never occurs
  • It's fragile, because there might be other calls to rnExpr that don't satisfy that invariant.

Here's an alternative. Replace

rnExpr (HsQuasiQuoteE qq)
  = runQuasiQuoteExpr qq        `thenM` \ (L _ expr') ->
    rnExpr expr'

with

rnExpr (HsQuasiQuoteE qq)
  = runQuasiQuoteExpr qq        `thenM` \ lexpr' ->
    rnExpr (HsPar lexpr')

Here I'm wrapping the returned expression in parens (which is probably a reasonable idea if we ever want to display the result of expanding quasiquotes), and that makes the types work out ok. The location on lexpr' will correctly be handled by the call to rnLExpr in the HsPar case of rnExpr.

Would that work? The same approach would work nicely for types too I think.

Simon

Last edited 8 months ago by simonpj (previous) (diff)

comment:11 Changed 8 months ago by edsko

So actually I agreed with you that the special case was undesirable, which is why I had (for types, not yet for expressions) got rid of rnHsType completely, and defined only rnLHsType. This way the exceptions you mention are taken care off statically, which is much better.

Your suggestion is an alternative; I thought about it but I thought I'd need to add a new constructor to HsExpr/HsType/Pat/..; didn't realize that I could use the constructor for parens. Do you prefer I use HsPar and co instead of removing rnHsExpr (rnHsType, ..)? It certainly would be a less intrusive change I guess.

Last edited 8 months ago by edsko (previous) (diff)

comment:12 Changed 8 months ago by edsko

Note however that introducing HsPar and co will change the output of the pretty-printer, not sure if that will break regression tests.

comment:13 Changed 8 months ago by edsko

Ok, tried this for both expressions and types and I think it works, and no regression tests break. Will try it for patterns too (and perhaps declarations -- not sure yet what the story is there) and commit.

comment:14 Changed 8 months ago by simonpj

Yes, I think I prefer the HsPar approach. Moreover, if we were to display the output of the renamer then this program

f x = g [qq| ...blah... |] x

would display, after renaming, as

f x = g (...expanded quote...) x

where the parens are actively helpful in parsing!

Let's do that. Thanks.

Simon

comment:15 Changed 8 months ago by Edsko de Vries <edsko@…>

comment:16 Changed 8 months ago by edsko

  • Resolution set to fixed
  • Status changed from new to closed

Ok, implemented as discussed above for expressions, types and patterns; for declarations the locations as returned by the quasi-quoter were respected. The test case verifies the locations to guard against future regressions.

comment:17 Changed 8 months ago by simonmar

  • Owner edsko deleted
  • Resolution fixed deleted
  • Status changed from closed to new

The test is failing for me:

=====> T7918(normal) 10 of 13 [0, 0, 0]
cd . && '/home/simon/code-all/work/ghc-validate/inplace/bin/ghc-stage2' -fforce-recomp -dcore-lint -dcmm-lint -dno-debug-output -no-user-package-db -rtsopts -fno-ghci-history -o T7918 T7918.hs  -package ghc  >T7918.comp.stderr 2>&1
cd . && ./T7918 "/home/simon/code-all/work/ghc-validate/inplace/lib"   </dev/null >T7918.run.stdout 2>T7918.run.stderr
Wrong exit code (expected 0 , actual 1 )
Stdout:

Stderr:
T7918: <command line>: can't load .so/.DLL for: /home/simon/code-all/work/ghc-validate/libraries/ghc-prim/dist-install/build/libHSghc-prim-0.3.1.0-ghc7.7.20130904.so (/home/simon/code-all/work/ghc-validate/libraries/ghc-prim/dist-install/build/libHSghc-prim-0.3.1.0-ghc7.7.20130904.so: undefined symbol: stg_forkOnzh)

*** unexpected failure for T7918(normal)

My guess is that the test itself is linked statically, but the GHC package is expecting to be linked dynamically, so it tries to load the shared libraries rather than the static ones.

comment:18 Changed 8 months ago by jstolarek

Test also fails for me on x86_64 Linux.

comment:19 Changed 8 months ago by edsko

  • Status changed from new to merge

I think this is now fixed; added config.ghc_th_way_flags to the test (copied from other TH tests). Not sure why that wasn't necessary on OSX or why the other tests in quasiquotation don't need but, it it does seem to fix the problem (inserts -dynamic). Closing the issue again; please reopen if it turns out not to be fixed.

comment:20 Changed 8 months ago by thoughtpolice

  • Resolution set to fixed
  • Status changed from merge to closed
Note: See TracTickets for help on using tickets.