[GHC] #13311: Audit shady uses of tcSplitSigmaTy
#13311: Audit shady uses of tcSplitSigmaTy -------------------------------------+------------------------------------- Reporter: RyanGlScott | Owner: (none) Type: task | Status: new Priority: normal | Milestone: 8.4.1 Component: Compiler | Version: 8.1 (Type checker) | Keywords: | Operating System: Unknown/Multiple Architecture: | Type of failure: None/Unknown Unknown/Multiple | Test Case: | Blocked By: Blocking: | Related Tickets: Differential Rev(s): | Wiki Page: -------------------------------------+------------------------------------- When fixing #12918, I ended up needing to introduce a helper function called `tcSplitNestedSigmaTys`, which is quite similar to `tcSplitSigmaTy`, except that it attempts to look through as many nested `forall`s as possible. (See the documentation for `tcSplitNestedSigmaTys` [http://git.haskell.org/ghc.git/blob/611f998fd545b45167170d9e60b7d9147178f0a1... here]). In the process, I accidentally discovered a bug! First, consider this program: {{{#!hs {-# LANGUAGE RankNTypes #-} module Bug where class C a where op :: forall b c. (Monoid b, Monoid c, Eq a) => a -> b -> c }}} This should be (and is) rejected, since we're trying to constrain `a` with `Eq` in `op`, but we don't have `-XConstraintedClassMethods` enabled. But what about this equivalent program? {{{#!hs {-# LANGUAGE RankNTypes #-} module Bug where class C a where op :: forall b. Monoid b => forall c. Monoid c => Eq a => a -> b -> c }}} By the same logic, this should also be rejected. But in GHC 8.0.2, it wasn't! That's because we were checking for the presence of constrained class variables using `tcSplitSigmaTy`, which only gives you `Monoid b` as the constraint, totally ignoring the bogus constraint inside of `forall c. Monoid c => Eq a => a -> b -> c`. When I fixed #12918 (a seemingly unrelated task), I ended up replacing that very use of `tcSplitSigmaTy` with `tcSplitNestedSigmaTys`. Now it gives you `(Monoid b, Monoid c, Eq a)` as expected when checking the constraints, and the second program is now rightly rejected on GHC HEAD. But I fear that there may be many more bogus uses of `tcSplitSigmaTy`. I should go through and try flushing out bugs of a similar caliber, and see if subbing in `tcSplitNestedSigmaTys` fixes these bugs. -- Ticket URL: http://ghc.haskell.org/trac/ghc/ticket/13311 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler
#13311: Audit shady uses of tcSplitSigmaTy -------------------------------------+------------------------------------- Reporter: RyanGlScott | Owner: RyanGlScott Type: task | Status: new Priority: normal | Milestone: 8.4.1 Component: Compiler (Type | Version: 8.1 checker) | Resolution: | Keywords: Operating System: Unknown/Multiple | Architecture: | Unknown/Multiple Type of failure: None/Unknown | Test Case: Blocked By: | Blocking: Related Tickets: | Differential Rev(s): Wiki Page: | -------------------------------------+------------------------------------- Changes (by RyanGlScott): * owner: (none) => RyanGlScott -- Ticket URL: http://ghc.haskell.org/trac/ghc/ticket/13311#comment:1 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler
#13311: Audit shady uses of tcSplitSigmaTy -------------------------------------+------------------------------------- Reporter: RyanGlScott | Owner: RyanGlScott Type: task | Status: new Priority: normal | Milestone: 8.4.1 Component: Compiler (Type | Version: 8.1 checker) | Resolution: | Keywords: Operating System: Unknown/Multiple | Architecture: | Unknown/Multiple Type of failure: None/Unknown | Test Case: Blocked By: | Blocking: Related Tickets: | Differential Rev(s): Wiki Page: | -------------------------------------+------------------------------------- Comment (by simonpj): I'm not terribly bothered about failing to reject a program without `-XConstrainedClassMethods` once you start having `RankNTypes`. By all means look at it; but I'm not losing sleep over it. -- Ticket URL: http://ghc.haskell.org/trac/ghc/ticket/13311#comment:2 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler
#13311: Audit shady uses of tcSplitSigmaTy -------------------------------------+------------------------------------- Reporter: RyanGlScott | Owner: RyanGlScott Type: task | Status: new Priority: normal | Milestone: 8.4.1 Component: Compiler (Type | Version: 8.1 checker) | Resolution: | Keywords: Operating System: Unknown/Multiple | Architecture: | Unknown/Multiple Type of failure: None/Unknown | Test Case: Blocked By: | Blocking: Related Tickets: | Differential Rev(s): Wiki Page: | -------------------------------------+------------------------------------- Comment (by RyanGlScott): After a quick audit, I found two `tcSplitSigmaTy`-related bugs: * http://git.haskell.org/ghc.git/blob/8f20844d3435094583db92a30550ca319d2be863... When you type this into GHCi: {{{ λ> let a :: forall a b. (Num a, Num b) => (# a, b #); !a = (# 1, 2 #) }}} you get this error: {{{ You can't mix polymorphic and unlifted bindings !a = (# 1, 2 #) Probable fix: add a type signature }}} But if you type this instead: {{{ λ> let a :: forall a . (Num a) => forall b. (Num b) => (# a, b #); !a = (# 1, 2 #) }}} then GHCi panics! {{{ ghc: panic! (the 'impossible' happened) (GHC version 8.0.2 for x86_64-unknown-linux): dsLet: unlifted !a_a1Py = (# 1, 2 #) returnIO @ [()] (: @ () (unsafeCoerce# @ 'PtrRepLifted @ 'PtrRepLifted @ (forall a_a1P7. Num a_a1P7 => forall b_a1P8. Num b_a1P8 => (# a_a1P7, b_a1P8 #)) @ () a_a1P6) ([] @ ())) }}} * http://git.haskell.org/ghc.git/blob/8f20844d3435094583db92a30550ca319d2be863... Compare this error message: {{{ λ> let f :: forall a b. (Monoid a, Monoid b) => Maybe a -> Maybe b; f _ = mempty λ> do { f; putChar 'a' } <interactive>:30:6: error: • Couldn't match expected type ‘IO a1’ with actual type ‘Maybe a0 -> Maybe b0’ • Probable cause: ‘f’ is applied to too few arguments In a stmt of a 'do' block: f In the expression: do { f; putChar 'a' } In an equation for ‘it’: it = do { f; putChar 'a' } }}} with this one: {{{ λ> let f :: forall a. (Monoid a) => forall b. (Monoid b) => Maybe a -> Maybe b; f _ = mempty λ> do { f; putChar 'a' } <interactive>:32:6: error: • Couldn't match expected type ‘IO a1’ with actual type ‘Maybe a0 -> Maybe b0’ • In a stmt of a 'do' block: f In the expression: do { f; putChar 'a' } In an equation for ‘it’: it = do { f; putChar 'a' } }}} The second one doesn't complain about `f` being applied to too few arguments! This is by no means an exhaustive list, as I only examine the parts of the codebase that I could make sense of. -- Ticket URL: http://ghc.haskell.org/trac/ghc/ticket/13311#comment:3 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler
#13311: Audit shady uses of tcSplitSigmaTy -------------------------------------+------------------------------------- Reporter: RyanGlScott | Owner: RyanGlScott Type: task | Status: new Priority: normal | Milestone: 8.4.1 Component: Compiler (Type | Version: 8.1 checker) | Resolution: | Keywords: Operating System: Unknown/Multiple | Architecture: | Unknown/Multiple Type of failure: None/Unknown | Test Case: Blocked By: | Blocking: Related Tickets: | Differential Rev(s): Wiki Page: | -------------------------------------+------------------------------------- Comment (by goldfire): Replying to [comment:3 RyanGlScott]:
When you type this into GHCi:
{{{ λ> let a :: forall a b. (Num a, Num b) => (# a, b #); !a = (# 1, 2 #) }}}
you get this error:
{{{ You can't mix polymorphic and unlifted bindings !a = (# 1, 2 #) Probable fix: add a type signature }}}
I'm confused. That's not an unlifted binding! -- Ticket URL: http://ghc.haskell.org/trac/ghc/ticket/13311#comment:4 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler
#13311: Audit shady uses of tcSplitSigmaTy -------------------------------------+------------------------------------- Reporter: RyanGlScott | Owner: RyanGlScott Type: task | Status: new Priority: normal | Milestone: 8.4.1 Component: Compiler (Type | Version: 8.1 checker) | Resolution: | Keywords: Operating System: Unknown/Multiple | Architecture: | Unknown/Multiple Type of failure: None/Unknown | Test Case: Blocked By: | Blocking: Related Tickets: | Differential Rev(s): Wiki Page: | -------------------------------------+------------------------------------- Comment (by RyanGlScott): Replying to [comment:4 goldfire]:
I'm confused. That's not an unlifted binding!
Good point. But the main point is that there is a discrepancy that can be unveiled simply by nesting `forall`s, not that the error message is off. (BTW, #9140 is where this error message originally comes from.) -- Ticket URL: http://ghc.haskell.org/trac/ghc/ticket/13311#comment:5 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler
#13311: Audit shady uses of tcSplitSigmaTy -------------------------------------+------------------------------------- Reporter: RyanGlScott | Owner: RyanGlScott Type: task | Status: patch Priority: normal | Milestone: 8.4.1 Component: Compiler (Type | Version: 8.1 checker) | Resolution: | Keywords: Operating System: Unknown/Multiple | Architecture: | Unknown/Multiple Type of failure: None/Unknown | Test Case: Blocked By: | Blocking: Related Tickets: | Differential Rev(s): Phab:D3678 Wiki Page: | -------------------------------------+------------------------------------- Changes (by RyanGlScott): * status: new => patch * differential: => Phab:D3678 Comment: Interestingly, I can't produce the "unlifted bindings"-related panic anymore with the GHC 8.2 branch. But the other issue is simple enough to fix: Phab:D3678. -- Ticket URL: http://ghc.haskell.org/trac/ghc/ticket/13311#comment:6 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler
#13311: Audit shady uses of tcSplitSigmaTy
-------------------------------------+-------------------------------------
Reporter: RyanGlScott | Owner: RyanGlScott
Type: task | Status: patch
Priority: normal | Milestone: 8.4.1
Component: Compiler (Type | Version: 8.1
checker) |
Resolution: | Keywords:
Operating System: Unknown/Multiple | Architecture:
| Unknown/Multiple
Type of failure: None/Unknown | Test Case:
Blocked By: | Blocking:
Related Tickets: | Differential Rev(s): Phab:D3678
Wiki Page: |
-------------------------------------+-------------------------------------
Comment (by Ben Gamari
#13311: Audit shady uses of tcSplitSigmaTy -------------------------------------+------------------------------------- Reporter: RyanGlScott | Owner: RyanGlScott Type: task | Status: closed Priority: normal | Milestone: 8.4.1 Component: Compiler (Type | Version: 8.1 checker) | Resolution: fixed | Keywords: Operating System: Unknown/Multiple | Architecture: | Unknown/Multiple Type of failure: None/Unknown | Test Case: | typecheck/should_fail/T13311 Blocked By: | Blocking: Related Tickets: | Differential Rev(s): Phab:D3678 Wiki Page: | -------------------------------------+------------------------------------- Changes (by RyanGlScott): * status: patch => closed * testcase: => typecheck/should_fail/T13311 * resolution: => fixed -- Ticket URL: http://ghc.haskell.org/trac/ghc/ticket/13311#comment:8 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler
participants (1)
-
GHC