Opened 3 years ago

Closed 3 years ago

Last modified 2 years ago

#10419 closed task (invalid)

Refactor LoadIface to distinguish getting a ModIface versus loading into EPT

Reported by: ezyang Owned by: ezyang
Priority: normal Milestone:
Component: Compiler (Type checker) Version: 7.11
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

At the moment, making a call to loadSysInterface (or the related interfaces) has two effects:

  1. You get a ModIface
  2. The declarations/instances/etc of the interface are type-checked and loaded into the EPT

In fact, users of these functions rarely want both of these; they either only want the ModIface, or only want to load up the declarations. Here is a survey I did, where I indicates a ModIface was wanted, D indicates we wanted to load the declarations, and ID means both were wanted;

RnSplice
    loadInterfaceForName:
 D      rnBracket: deprecation checking of identifiers in brackets ()

RnEnv
    loadInterfaceForName
I       warnIfDeprecated: deprecation checking of GlobalRdrElt (mi_warn_fun)
I       lookupFixityRn: fixity lookup (mi_fix_fun)
    loadSrcInterface_maybe
ID      lookupQualifiedNameGHCi: implicit import


RnNames
    loadSrcInterface
ID      rnImportDecl: source import! $$$$$
I       printMinimalImports: minimal import calculation (mi_exports)

DsMonad
    loadInterface
I       loadModule: load and return exported entities (like a source import but for Module)
                    special case for Data.Array.Parallel

DynamicLoading
    loadPluginInterface
 D      forceLoadModuleInterfaces: what.

Linker
    loadInterface
I       getLinkDeps: poke the dependencies (mi_boot, mi_deps)
    loadUserInterface
I       random ass thing to check if it's a sigof XXX (mi_sig_of)

MkIface
    loadInterface
I?      needInterface: utility
            checkModUsage: given the usage information from the old hi, determine if recompilation is required (hashes only)

FamInst
    loadSysInterface
 D      getFamInsts: load instance into EPS and return it

TcDeriv
    loadInterfaceForName
I       getDataConFixityFun: looks at mi_fix_fn

TcRnDriver
    loadSysInterface
I       tcRnSignature: signature renaming hack
 D      loadUnqualIfaces: for accurate available instance reporting
    loadModuleInterfaces
 D      tcRnImports: load orphans
    loadModuleInterface
        getModuleInterface: load a 'Module' (GHCi)
 D          hscCheckSafe' (HscMain, mi_trust, mi_trust_pkg, mi_deps)
ID          getPackageModuleInfo (contains interface)
    loadSrcInterface
 D      runTcInteractive: mi_deps/dep_orphs, pull in orphans from interactive context ic_imports

TcSplice
    loadInterfaceForModule
I       reifyModule: mi_usages only

It's well worth noting that for most Names, it is NOT NECESSARY, since when we pull on it with loadDecl, the interface will get loaded in. Consequently, I think some of the cases where we're loading interfaces are unnecessary, e.g. the case in rnBracket. The primary cases are dealing with orphan imports; there are multiple sites which redo this logic, and it should all be centralized in one place.

So here is the concrete proposal:

  • Add a new function for taking a ModIface and loading it into the EPT.
  • Change the rest of the existing loadXXXInterface functions to NOT load declarations. We'll actually typecheck the interface file when we pull on the Name in question during type checking.
  • Introduce a new function to LoadIface for loading orphans (i.e. what to do when a source level import occurs). Have lookupQualifiedNameGHCi, rnImportDecl, tcRnImports and runTcInteractive use this function.

Change History (4)

comment:1 Changed 3 years ago by simonpj

Hmm. Note that

  • If you want a declaration you have to load the ModIface, and we don't want to do that twice, so D implies I.
  • If you want the ModIface it's not much work to load all the declarations because we typecheck them lazily (via forkM in TcIface). So it's not clear to me that you are going to save much work.

So: what's your goal here? Just efficiency?

The proposal would mean that in the ExternalPackageState there might be members of the eps_PIT whose declarations have not been typechecked and loaded into the eps_PTE. Currently it's an invariant that they all are. That might be fine but it needs documenting. Here just for reference is the current defn of EPS:

data ExternalPackageState
  = EPS {
        eps_is_boot      :: !(ModuleNameEnv (ModuleName, IsBootInterface)),
        eps_PIT          :: !PackageIfaceTable,
        eps_PTE          :: !PackageTypeEnv,
        eps_inst_env     :: !PackageInstEnv,  
        eps_fam_inst_env :: !PackageFamInstEnv,
        eps_rule_base    :: !PackageRuleBase,  
        eps_vect_info    :: !PackageVectInfo,  
        eps_ann_env      :: !PackageAnnEnv,   
        eps_mod_fam_inst_env :: !(ModuleEnv FamInstEnv), 
        eps_stats :: !EpsStats                 
  }

Thinking about the invariant leads to some questions:

  • Consider a module whose ModIface M we have loaded, into the eps_PIT. Now suppose we want the declaration for M.foo. Do we:

    1. Find foo's IfaceDecl in M's ModIface and add that declaration alone to eps_PTE? But how do we find foo's IfaceDecl, remembering that one IfaceDecl may bind many Names.? You'll need to build a little index; but that is (in effect) what the eps_PTE already is!

    2. Typecheck all the declarations in M's ModIface and add all of them to the eps_PTE? If so, how do we record that we have done this so we don't repeat it? Maybe it's enough that future lookups will find M.foo in the PTE.
  • Currently we assume that if we've loaded an interface for a non-orphan module we've loaded its instances into eps_inst_env. But that won't be true any more. Or will it?
  • Similarly we'd have to think about eps_ann_env, eps_mod_fam_inst_env etc.

So currently I'm unconvinced.

Simon

comment:2 Changed 3 years ago by ezyang

Resolution: invalid
Status: newclosed

My original motivation was clarity surrounding plugin interface loading, but actually I think there's a better way to solve this problem (#10420), so I guess let's let this die.

Though, I think it may still be useful to add a new function to LoadIface expressly for orphan loading, to consolidate this logic.

comment:3 in reply to:  2 Changed 3 years ago by simonpj

Though, I think it may still be useful to add a new function to LoadIface expressly for orphan loading, to consolidate this logic.

Maybe so. Which logic did you have in mind, and where is it duplicated? Can you suggest a patch?

Simon

comment:4 Changed 2 years ago by Edward Z. Yang <ezyang@…>

In 640fe14255706ab9c6a1fa101d9b05dfabdc6556/ghc:

Remove unnecessary loadInterface for TH quoted name.

Summary:
The load was introduced a32d3e4da0aceb624c958f02cad7327e17ac94db
to fix a bug where deprecations assumed that the name in question
had already had their interface loaded.  The new deprecation
code no longer makes this assumption and just loads the interface,
so this eager load is not necessary.

Verified that TH_reifyType2 continues to work.

Signed-off-by: Edward Z. Yang <ezyang@cs.stanford.edu>

Test Plan: validate

Reviewers: simonpj, austin

Subscribers: bgamari, thomie

Differential Revision: https://phabricator.haskell.org/D891

GHC Trac Issues: #10419
Note: See TracTickets for help on using tickets.