Opened 4 years ago

Closed 4 years ago

Last modified 3 years ago

#10420 closed bug (fixed)

"Care with plugin imports" is wrong / orphan RULE visibility

Reported by: ezyang Owned by: ezyang
Priority: normal Milestone: 8.0.1
Component: Compiler (Type checker) Version: 7.11
Keywords: Cc:
Operating System: Unknown/Multiple Architecture: Unknown/Multiple
Type of failure: None/Unknown Test Case: plugins07
Blocked By: Blocking: #10294
Related Tickets: Differential Rev(s): Phab:D950
Wiki Page:

Description (last modified by ezyang)

A module loaded by a plugin poisons the module cache, so we never load an orphan RULE or instance even if we legitimately should do so. The way to test this is a bit convoluted, but it goes something like this:

plugins07.hs

module Main where

import Plugins07a
import RuleDefiningPlugin

{-# NOINLINE x #-}
x = "foo"

main = putStrLn (show x)

Plugins07a.hs

{-# OPTIONS_GHC -fplugin RuleDefiningPlugin #-}
module Plugins07a where

RuleDefiningPlugin.hs, in ANOTHER PACKAGE (otherwise, EPT rules don't apply)

module RuleDefiningPlugin where

import GhcPlugins

{-# RULES "unsound" forall x. show x = "SHOWED" #-}

plugin :: Plugin
plugin = defaultPlugin

I'll commit the full test.

Here's what happens:

  • Building Plugins07a results in loadPluginInterface on RuleDefiningPlugin. We load the ModIface but only add its types to the environment because of the "Care with plugin imports" special case.
  • Building plugins07.hs results in a normal source level import for RuleDefiningPlugin, but ModIface is already in the cache so we don't load anything. RULE is not loaded, disaster!

Admittedly, actually triggering this bug requires a convoluted chain of events. But really this problem arose because the "Care with plugin imports" fix is just completely nonsense. Here's what we should do:

  • We should apply the same fix from #2182 on orphan instances to orphan rules too. This way, we can safely load RULEs into the EPS without accidentally bringing them into scope when they shouldn't be.
  • Loading an interface should unconditionally suck in the instances and rules.

The result is more correct and simpler, so it seems worth fixing.

Change History (10)

comment:1 Changed 4 years ago by ezyang

Description: modified (diff)

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

In ab45de12cee5af5dcb68b2afce1826ab9bf71ba0/ghc:

Failing test for #10420 using plugins.

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

comment:3 Changed 4 years ago by ezyang

Test Case: plugins07

comment:4 Changed 4 years ago by ezyang

Blocking: 10294 added

comment:5 Changed 4 years ago by ezyang

Differential Rev(s): Phab:D950
Status: newpatch

comment:6 Changed 4 years ago by ezyang

Priority: lownormal

Marking higher since this can affect non-orphan instances too, see #10294 for an example.

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

In 0cb1f5cf26fae946ca745abc5e302e62a8f66feb/ghc:

Filter orphan rules based on imports, fixes #10294 and #10420.

Summary:
If we have an orphan rule in our database, don't apply it
unless the defining module is transitively imported by the
module we are processing.  We do this by defining a new RuleEnv
data type which includes both the RuleBase as well as the set
of visible orphan modules, and threading this through the
relevant environments (CoreReader, RuleCheckEnv and ScEnv).

This is analogous to the instances fix we applied in #2182
4c834fdddf4d44d12039da4d6a2c63a660975b95, but done for RULES.
An important knock-on effect is that we can remove some buggy
code in LoadInterface which tried to avoid loading interfaces
that were loaded by plugins (which sometimes caused instances
and rules to NEVER become visible).

One note about tests: I renamed the old plugins07 test to T10420
and replaced plugins07 with a test to ensure that a plugin
import did not cause new rules to be loaded in.

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

Test Plan: validate

Reviewers: simonpj, austin, goldfire

Subscribers: bgamari, thomie

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

GHC Trac Issues: #10420

comment:8 Changed 4 years ago by ezyang

Resolution: fixed
Status: patchclosed

comment:9 Changed 4 years ago by thomie

Milestone: 8.0.1

comment:10 Changed 3 years ago by Phyx-

This and other plugin tests are failing on Windows and I can't really figure out why. It Seems like a cabal bug.

Tamar@CI MINGW64 /c/TeamCity/buildAgent/work/28754042a1be6052/testsuite/tests/plugins/plugins07.run/rule-defining-plugin
$ make -C . package.plugins07 TOP=C:/TeamCity/buildAgent/work/28754042a1be6052/testsuite/
make: Entering directory '/c/TeamCity/buildAgent/work/28754042a1be6052/testsuite/tests/plugins/plugins07.run/rule-defining-plugin'
make -s --no-print-directory clean.plugins07
mkdir pkg.plugins07
"/c/TeamCity/buildAgent/work/28754042a1be6052/inplace/bin/ghc-stage2.exe" -outputdir pkg.plugins07 --make -v0 -o pkg.plugins07/setup Setup.hs
"/c/TeamCity/buildAgent/work/28754042a1be6052/inplace/bin/ghc-pkg.exe" init pkg.plugins07/local.package.conf
pkg.plugins07/setup configure --distdir pkg.plugins07/dist -v0 --enable-library-vanilla --disable-shared --prefix="/c/TeamCity/buildAgent/work/28754042a1be6052/testsuite/tests/plugins/plugins07.run/rule-defining-plugin/pkg.plugins07/install" --with-compiler="/c/TeamCity/buildAgent/work/28754042a1be6052/inplace/bin/ghc-stage2.exe" --with-hc-pkg="/c/TeamCity/buildAgent/work/28754042a1be6052/inplace/bin/ghc-pkg.exe" --package-db=pkg.plugins07/local.package.conf
pkg.plugins07/setup build     --distdir pkg.plugins07/dist -v0
pkg.plugins07/setup install   --distdir pkg.plugins07/dist -v3
/bin/sh: setup: command not found
make: *** [Makefile:18: package.plugins07] Error 127
make: Leaving directory '/c/TeamCity/buildAgent/work/28754042a1be6052/testsuite/tests/plugins/plugins07.run/rule-defining-plugin'

My guess is that cabal is expecting setup.exe to be in the same folder as the .cabal file. Which these tests don't do. If this is the case, how can they be working on linux? the install partially works until dies.

Note: See TracTickets for help on using tickets.