Opened 7 years ago

Last modified 10 months ago

#7398 new bug

RULES don't apply to a newtype constructor

Reported by: shachaf Owned by:
Priority: normal Milestone: 8.10.1
Component: Compiler Version: 7.6.1
Keywords: Cc: bgamari@…, ghc.haskell.org@…, dfeuer
Operating System: Unknown/Multiple Architecture: Unknown/Multiple
Type of failure: Runtime performance bug Test Case:
Blocked By: Blocking:
Related Tickets: #6082, #10418, #13290 Differential Rev(s):
Wiki Page:

Description

For some reason, RULES that involve a newtype constructor never seem to fire. The following program demonstrates the problem:

module Main where

newtype Foo a = Foo { unFoo :: a }
  deriving Show

foo :: a -> Foo a
foo = Foo

{-# RULES "rule Foo"  forall v.  Foo v = error "Foo" #-}
{-# RULES "rule foo"  forall v.  foo v = error "foo" #-}

main :: IO ()
main = do
    print (Foo ())
    print (foo ())

"rule foo" fires, but "rule Foo" doesn't. The program prints

Foo {unFoo = ()}
D: foo

Note that this doesn't seem to affect selectors, only constructors.

Change History (18)

comment:1 Changed 7 years ago by bgamari

Cc: bgamari@… added

comment:2 Changed 7 years ago by igloo

difficulty: Unknown
Milestone: 7.8.1

Thanks for the report.

comment:3 Changed 5 years ago by thoughtpolice

Milestone: 7.8.37.10.1

Moving to 7.10.1

comment:4 Changed 5 years ago by thomie

GHC 7.8.3 now shows two warnings:

$ ghc-7.8.3 -fforce-recomp -O2 D.hs
[1 of 1] Compiling Main             ( D.hs, D.o )

D.hs:9:11: Warning:
    Rule "rule Foo" may never fire
      because ‘Main.Foo’ might inline first
    Probable fix: add an INLINE[n] or NOINLINE[n] pragma on ‘Main.Foo’

D.hs:10:11: Warning:
    Rule "rule foo" may never fire because ‘foo’ might inline first
    Probable fix: add an INLINE[n] or NOINLINE[n] pragma on ‘foo’
Linking D ...

GHC HEAD shows a different warning:

$ ghc-7.9.20141121 -O2 -fforce-recomp D.hs
[1 of 1] Compiling Main             ( D.hs, D.o )

D.hs:9:11: Warning:
    RULE left-hand side too complicated to desugar
      Optimised lhs: v
                     `cast` (Sym (Main.NTCo:Foo[0] <a>_R) :: a ~R# Foo a)
      Orig lhs: Main.Foo @ a v

D.hs:10:11: Warning:
    Rule "rule foo" may never fire because ‘foo’ might inline first
    Probable fix: add an INLINE[n] or NOINLINE[n] pragma on ‘foo’

If the warning RULE left-hand side too complicated to desugar is correct, we should add a test for it (there currently aren't any).

comment:5 Changed 5 years ago by thomie

The warnings were added in commit 79062a8a0e9168b7f925c05699c30333563a9303:

Author: Simon Peyton Jones <>
Date:   Mon Jul 23 09:17:09 2012 +0100

    Make the desugarer warn about RULES that may not fire
    
    This warning was suggested by Trac #6082, where we had
    a library RULE that failed to fire because its function
    was inlined too soon.

comment:6 Changed 5 years ago by thoughtpolice

Milestone: 7.10.17.12.1

Moving to 7.12.1 milestone; if you feel this is an error and should be addressed sooner, please move it back to the 7.10.1 milestone.

comment:7 Changed 5 years ago by liyang

Cc: ghc.haskell.org@… added

comment:8 Changed 4 years ago by duncan

comment:9 Changed 4 years ago by thomie

Type of failure: Incorrect result at runtimeRuntime performance bug

Since RULES are usually (always?) defined for performance reasons, I'm grouping this with all other runtime performance bugs.

comment:10 Changed 4 years ago by thoughtpolice

Milestone: 7.12.18.0.1

Milestone renamed

comment:11 Changed 4 years ago by thomie

Milestone: 8.0.1

comment:12 Changed 3 years ago by rwbarton

#13290 proposes disallowing rules for constructors altogether.

comment:13 Changed 21 months ago by dfeuer

Milestone: 8.6.1

I'm adding a milestone because I suspect we may have enough machinery to make rules involving newtype constructors work (when they're not the outermost names in the rules). Consider this code:

import Data.Functor.Identity
import Data.Coerce

hello :: (a -> b) -> a -> b
hello f x = f x
{-# INLINE [1] hello #-}

{-# RULES
"not good"  forall (f :: Identity a -> a) x. hello f (Identity x) = x
"also bad"  forall f (x :: a). hello f (Identity x :: Identity a) = x


"yes good"  forall (f :: Identity a -> a) x. hello f (coerce x) = x
"also good" forall f (x :: a) . hello f (coerce x :: Identity a) = x
"just fine" forall f (x :: a). hello f (coerce @a @(Identity a) x) = x

 #-}

test :: (Identity a -> a) -> a -> a
test f x = hello f (Identity x)
{-# NOINLINE test #-}

The rule labeled "not good" doesn't fire, but the one labeled "yes good" fires. The only difference is the spelling of coerce! This seems to suggest that we can apply some of the machinery for coerce rules to sometimes make good things happen. In particular, it seems likely that after type checking the rule, we can simply replace each newtype constructor/accessor with an appropriately typed invocation of coerce, and make the rules engine work.

Last edited 21 months ago by dfeuer (previous) (diff)

comment:14 Changed 21 months ago by dfeuer

Cc: dfeuer added

comment:15 Changed 21 months ago by simonpj

Currently, wen you have

{-# RULES "foo" forall x. f (g x) = h x #-}

we have to delay inlining g until the rule has had a decent chance to fire. We currently do this manually, usually thus

{-# NOINLINE[1] f,g #-}

The obvious thing for newtypes (and indeed other data constructors, if they have wrappers that unpack their arguments) is to delay inlining them. Something like

newtype T a = MkT (Maybe a)
{-# NOINLINE[1] MkT #-}

We don't support that right now, but we could.

The alternative is to try to make the rule work after inlining MkT, by being clever about casts. That might be possible.

But it doesn't work for data constructors like

data S = MkS {-# UNPACK #-} !Int

where the wrapper evaluates and unboxes the argument. After inlining, the original (f (MkS x)) turns into f (case x ov I# y -> MkS y), which is a lot harder to match.

So I think my suggestion, for now, is that we might want to allow users to put a NOINLINE pragma on data constructors.

comment:16 Changed 21 months ago by nomeata

The alternative is to try to make the rule work after inlining MkT, by being clever about casts. That might be possible.

We somewhat do this already: We have

{-# RULES "map/coerce" [1] map coerce = coerce #-}

which fires in all these cases:

foo :: [Int] -> [Int]
foo = map id
fooAge :: [Int] -> [Age]
fooAge = map Age
fooCoerce :: [Int] -> [Age]
fooCoerce = map coerce
fooUnsafeCoerce :: [Int] -> [Age]
fooUnsafeCoerce = map unsafeCoerce

(this is testsuite/tests/simplCore/should_run/T2110.hs).

comment:17 Changed 19 months ago by bgamari

Milestone: 8.6.18.8.1

It doesn't look like this will happen for 8.6.

comment:18 Changed 10 months ago by osa1

Milestone: 8.8.18.10.1

Bumping milestones of low-priority tickets.

Note: See TracTickets for help on using tickets.