Opened 8 months ago

Closed 6 months ago

Last modified 6 months ago

#16104 closed bug (fixed)

Plugin name lookup behavior change from GHC 8.4 series

Reported by: lerkok Owned by:
Priority: normal Milestone: 8.6.4
Component: Compiler Version: 8.6.3
Keywords: Cc:
Operating System: Unknown/Multiple Architecture: Unknown/Multiple
Type of failure: None/Unknown Test Case:
Blocked By: Blocking:
Related Tickets: Differential Rev(s):
Wiki Page:

Description

I'm trying to port a core plugin to GHC 8.6.3, which was last working fine with GHC 8.4 series. Unfortunately, I'm running into issues. Wondering if pluging programming requirements have changed, or is this a regression in GHC itself. I boiled it down to the following example and would like some guidance on how to make this work:

I have the following in file TestPlugin.hs:

{-# LANGUAGE TemplateHaskell #-}

module TestPlugin (plugin) where

import GhcPlugins
import Data.Bits

plugin :: Plugin
plugin = defaultPlugin {installCoreToDos = install}
  where install _ todos = return (test : todos)

        test = CoreDoPluginPass "Test" check

        check :: ModGuts -> CoreM ModGuts
        check m = do mbN <- thNameToGhcName 'complement
                     case mbN of
                       Just _  -> liftIO $ putStrLn "Found complement!"
                       Nothing -> error "Failed to locate complement"

                     return m

And I have a very simple Test.hs file:

{-# OPTIONS_GHC -fplugin TestPlugin #-}

main :: IO ()
main = return ()

With GHC-8.4.2, I have:

$ ghc-8.4.2 --make -package ghc -c TestPlugin.hs
[1 of 1] Compiling TestPlugin       ( TestPlugin.hs, TestPlugin.o )

$ ghc-8.4.2 -package ghc -c Test.hs
Found complement!

But with GHC 8.6.3, I get:

$ ghc-8.6.3 --make -package ghc -c TestPlugin.hs
[1 of 1] Compiling TestPlugin       ( TestPlugin.hs, TestPlugin.o )

$ ghc-8.6.3 -package ghc -c Test.hs
ghc: panic! (the 'impossible' happened)
  (GHC version 8.6.3 for x86_64-apple-darwin):
	Failed to locate complement

The problem goes away if I change Test.hs to:

{-# OPTIONS_GHC -fplugin TestPlugin #-}

import Data.Bits  -- Should not be required in the client code!

main :: IO ()
main = return ()

That is, if I explicitly import Data.Bits. But this is quite undesirable, since Test.hs is client code and the users of the plugin have no reason to import all bunch of modules the plugin might need for its own purposes. (In practice, this would require clients to import a whole bunch of irrelevant modules; quite unworkable and not maintainable.)

Should I be coding my plugin differently? Or is this a GHC regression?

Change History (12)

comment:1 Changed 8 months ago by mpickering

One work around might be to define the plugin in a package, install it in the package database and then use it. This standalone support for plugins has caused a number of issues.

comment:2 Changed 8 months ago by lerkok

Thanks Matt. Unfortunately that doesn't work either:

Here's the original plugin registered:

$ ghc-pkg list sbvPlugin
/usr/local/lib/ghc-8.6.3/package.conf.d
    (no packages)
/Users/LeventErkok/.ghc/x86_64-darwin-8.6.3/package.conf.d
    sbvPlugin-0.11

Example test file:

$ cat T11.hs
{-# OPTIONS_GHC -fplugin=Data.SBV.Plugin #-}

module T11 where

import Data.SBV.Plugin

h :: Integer -> Integer
h x = x - 1

g :: Integer -> Integer
g x = if x < 12 then x+1 else h x

{-# ANN f theorem #-}
f :: Integer -> Bool
f x = g x < g (x+1)

And I still get:

$ ghci T11.hs
GHCi, version 8.6.3: http://www.haskell.org/ghc/  :? for help
[1 of 1] Compiling T11              ( T11.hs, interpreted )
*** Exception: [SBV] Impossible happened, while trying to locate GHC name for: Data.Bits.complement
CallStack (from HasCallStack):
  error, called at ./Data/SBV/Plugin/Env.hs:380:30 in sbvPlugin-0.11-9tpp46BgzL09G1cUaUExU2:Data.SBV.Plugin.Env

Exactly the same issue. It's the same problem whether I load it in ghci or compile with ghc.

To fix it, I have to add the following two imports to my client file:

import Data.Bits
import Data.SBV

which is really unreasonable for the end user to do.

Do we at least know if this breakage from 8.4 was intentional, due to some internal design change perhaps?

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

comment:3 Changed 8 months ago by mpickering

No I don't know why this is happening.

In my plugins I would use the lookupOrig function so that I can be precise about what module a definition is in.

comment:4 Changed 8 months ago by michaelpj

I think we're also observing this.

I think this is not the intended behaviour. Looking at the documentation of thNameToGhcName it says: "Qualified or unqualified TH names will be dynamically bound to names in the module being compiled, if possible." My interpretation of this was: if you can import the name qualified in the module being compiled when the core plugin triggers, then the name lookup should succeed. This seems to no longer be true.

I can also confirm that adding (unused) imports for the modules whose names are being accessed makes the lookup work again.

In our case the plugin *is* registered via a separate package and we're still seeing this.

comment:5 Changed 7 months ago by simonpj

In GHC 8.4 we have

thNameToGhcName th_name = do
    hsc_env <- getHscEnv
    liftIO $ initTcForLookup hsc_env (lookupThName_maybe th_name)

lookupThName ends up calling lookupGlobalOccRn_maybe; and (since I think we have an Orig at this point) thence lookupExactOrOrig, and thence IfaceEnv.lookupOrig. The latter

  • Looks up in the original-name cache
  • If the lookup fails, it makes a fresh external Name, updates the orig-name cache, and returns the Name.

But in 8.6 we have

thNameToGhcName th_name
  =  do { names <- mapMaybeM lookup (thRdrNameGuesses th_name)
          -- Pick the first that works
          -- E.g. reify (mkName "A") will pick the class A in preference
          -- to the data constructor A
        ; return (listToMaybe names) }
  where
    lookup rdr_name
      | Just n <- isExact_maybe rdr_name   -- This happens in derived code
      = return $ if isExternalName n then Just n else Nothing
      | Just (rdr_mod, rdr_occ) <- isOrig_maybe rdr_name
      = do { cache <- getOrigNameCache
           ; return $ lookupOrigNameCache cache rdr_mod rdr_occ }
      | otherwise = return Nothing

See: it looks up in the orig-name cache, but doesn't extend it on failure.

That's what's going wrong. We just need to do what lookupOrig does.

In HEAD some more refactoring has happened, which makes it easier. We want thNameToGhcName to call something very like IfaceEnv.lookupOrigIO.

comment:6 Changed 7 months ago by Ben Gamari <ben@…>

In 312c957f/ghc:

testsuite: Add test for #16104

comment:7 Changed 7 months ago by bgamari

Milestone: 8.8.1
Status: newpatch

comment:8 Changed 7 months ago by bgamari

Milestone: 8.8.18.6.4

Actually, we can even get this in to 8.6.4.

comment:9 Changed 7 months ago by michaelpj

Getting this into 8.6.4 would be fantastic!

comment:10 Changed 6 months ago by bgamari

Resolution: fixed
Status: patchclosed

comment:11 Changed 6 months ago by Ben Gamari <ben@…>

In 372b5d1/ghc:

testsuite: Add test for #16104

comment:12 Changed 6 months ago by Ben Gamari <ben@…>

In 0d9f105b/ghc:

GhcPlugins: Fix lookup of TH names

Previously `thNameToGhcName` was calling `lookupOrigNameCache` directly, which
failed to handle the case that the name wasn't already in the name cache. This
happens, for instance, when the name was in scope in a plugin being used during
compilation but not in scope in the module being compiled. In this case we the
interface file containing the name won't be loaded and `lookupOrigNameCache`
fails. This was the cause of #16104.

The solution is simple: use the nicely packaged `lookupOrigIO` instead.
Note: See TracTickets for help on using tickets.