#10011: The Data instance for Ratio violates internal invariants. -------------------------------------+------------------------------------- Reporter: ekmett | Owner: ekmett Type: bug | Status: new Priority: normal | Milestone: Component: Core Libraries | Version: 7.10.1-rc1 Resolution: | Keywords: Operating System: Unknown/Multiple | Architecture: Type of failure: None/Unknown | Unknown/Multiple Blocked By: | Test Case: Related Tickets: | Blocking: | Differential Revisions: -------------------------------------+------------------------------------- Description changed by ekmett: Old description:

I found this when Simon was cleaning up unused dependencies in

{{{ https://phabricator.haskell.org/rGHCc409b6f30373535b6eed92e55d4695688d32be9e... }}}

The Data instance for Ratio just uses the raw `(:%)` constructor and doesn't check that the result is reduced to normal form.

It strikes me that the fix is to add back the Integral constraint on the Data instance and to use `(%)` rather than `(:%)` in the `gfoldl` and `gunfold` code.

This restores the invariant and matches the behavior of "virtual constructors" we've used to patch up such problems elsewhere.

New description: I found this when Simon was cleaning up unused dependencies in https://phabricator.haskell.org/rGHCc409b6f30373535b6eed92e55d4695688d32be9e... The Data instance for Ratio just uses the raw `(:%)` constructor and doesn't check that the result is reduced to normal form. It strikes me that the fix is to add back the Integral constraint on the Data instance and to use `(%)` rather than `(:%)` in the `gfoldl` and `gunfold` code. This restores the invariant and matches the behavior of "virtual constructors" we've used to patch up such problems elsewhere. -- -- Ticket URL: http://ghc.haskell.org/trac/ghc/ticket/10011#comment:1 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler

#10011: The Data instance for Ratio violates internal invariants. -------------------------------------+------------------------------------- Reporter: ekmett | Owner: ekmett Type: bug | Status: new Priority: normal | Milestone: 7.10.1 Component: Core Libraries | Version: 7.10.1-rc1 Resolution: | Keywords: Operating System: Unknown/Multiple | Architecture: Type of failure: Incorrect result | Unknown/Multiple at runtime | Test Case: Blocked By: | Blocking: Related Tickets: | Differential Revisions: -------------------------------------+------------------------------------- Changes (by ekmett): * failure: None/Unknown => Incorrect result at runtime -- Ticket URL: http://ghc.haskell.org/trac/ghc/ticket/10011#comment:3 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler

#10011: The Data instance for Ratio violates internal invariants. -------------------------------------+------------------------------------- Reporter: ekmett | Owner: ekmett Type: bug | Status: patch Priority: normal | Milestone: 7.10.1 Component: Core Libraries | Version: 7.10.1-rc1 Resolution: | Keywords: Operating System: Unknown/Multiple | Architecture: Type of failure: Incorrect result | Unknown/Multiple at runtime | Test Case: Blocked By: | Blocking: Related Tickets: | Differential Revisions: Phab:D625 -------------------------------------+------------------------------------- Comment (by Herbert Valerio Riedel ): In [changeset:"79b0d0e633af8302d2dd907663a4a231cd889b67/ghc"]: {{{ #!CommitTicketReference repository="ghc" revision="79b0d0e633af8302d2dd907663a4a231cd889b67" Restore invariant in `Data (Ratio a)` instance The Data instance for `Ratio` just uses the raw `:%` constructor and doesn't check that the result is reduced to normal form. The fix is to add back the `Integral` constraint on the Data instance (which was dropped in c409b6f30373535) and to use `%` rather than `:%` in the `gfoldl` and `gunfold` implementation. This restores the invariant and matches the behavior of "virtual constructors" we've used to patch up such problems elsewhere. This addresses #10011 Reviewed By: ekmett, austin Differential Revision: https://phabricator.haskell.org/D625 }}} -- Ticket URL: http://ghc.haskell.org/trac/ghc/ticket/10011#comment:5 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler

#10011: The Data instance for Ratio violates internal invariants. -------------------------------------+------------------------------------- Reporter: ekmett | Owner: ekmett Type: bug | Status: closed Priority: normal | Milestone: 7.10.1 Component: Core Libraries | Version: 7.10.1-rc1 Resolution: fixed | Keywords: Operating System: Unknown/Multiple | Architecture: Type of failure: Incorrect result | Unknown/Multiple at runtime | Test Case: Blocked By: | Blocking: Related Tickets: | Differential Revisions: Phab:D625 -------------------------------------+------------------------------------- Comment (by Herbert Valerio Riedel ): In [changeset:"22c4d60b0665a15535c0ec9fe0b8e65d2c948e7d/ghc"]: {{{ #!CommitTicketReference repository="ghc" revision="22c4d60b0665a15535c0ec9fe0b8e65d2c948e7d" Revert "Restore invariant in `Data (Ratio a)` instance" This reverts commit 79b0d0e633af8302d2dd907663a4a231cd889b67 due to Compile failed (status 256) errors were: [1 of 2] Compiling A ( A.hs, A.o ) [2 of 2] Compiling Main ( T4491.hs, T4491.o ) T4491.hs:19:11: Illegal data constructor name: ‘%’ When splicing a TH expression: (GHC.Real.%) 11 2 In the splice: $(dataToExpQ (const Nothing) (5.5 :: Rational)) *** unexpected failure for T4491(normal) Therefore re-opening #10011 }}} -- Ticket URL: http://ghc.haskell.org/trac/ghc/ticket/10011#comment:7 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler

#10011: The Data instance for Ratio violates internal invariants. -------------------------------------+------------------------------------- Reporter: ekmett | Owner: Type: bug | Status: new Priority: normal | Milestone: 7.10.1 Component: Core Libraries | Version: 7.10.1-rc1 Resolution: | Keywords: Operating System: Unknown/Multiple | Architecture: Type of failure: Incorrect result | Unknown/Multiple at runtime | Test Case: Blocked By: | Blocking: Related Tickets: | Differential Revisions: Phab:D625 -------------------------------------+------------------------------------- Comment (by Herbert Valerio Riedel ): In [changeset:"3df429e29b6fabda12af71091ba4ad1360f49b44/ghc"]: {{{ #!CommitTicketReference repository="ghc" revision="3df429e29b6fabda12af71091ba4ad1360f49b44" Restore invariant in `Data (Ratio a)` instance (2nd attempt, this time leaving the `Constr` using `":%"`) The Data instance for `Ratio` just uses the raw `:%` constructor and doesn't check that the result is reduced to normal form. The fix is to add back the `Integral` constraint on the Data instance (which was dropped in c409b6f30373535) and to use `%` rather than `:%` in the `gfoldl` and `gunfold` implementation. This restores the invariant and matches the behavior of "virtual constructors" we've used to patch up such problems elsewhere. This addresses #10011 Reviewed By: ekmett, austin Differential Revision: https://phabricator.haskell.org/D625 }}} -- Ticket URL: http://ghc.haskell.org/trac/ghc/ticket/10011#comment:9 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler

#10011: The Data instance for Ratio violates internal invariants. -------------------------------------+------------------------------------- Reporter: ekmett | Owner: Type: bug | Status: new Priority: normal | Milestone: 7.10.1 Component: Core Libraries | Version: 7.10.1-rc1 Resolution: | Keywords: Operating System: Unknown/Multiple | Architecture: Type of failure: Incorrect result | Unknown/Multiple at runtime | Test Case: Blocked By: | Blocking: Related Tickets: | Differential Revisions: Phab:D625 -------------------------------------+------------------------------------- Comment (by ekmett): This work scoped in this ticket is done, but you are right, we should definitely add a regression test to ensure this doesn't backslide in the future. Along the way we found a much less critical, but related issue (ok, really, feature request), which I'll file a separate ticket for -- with an eye towards fixing it on a longer time horizon: I'd eventually like to fix up the "virtual" data constructor we talk about in the Data instance to use `(%)`, since we're using a virtual constructor here, and that would let `gshow` and the like do the right thing, but when we tried that as part of this patch, the fact that `dataToExpQ` currently filters out things that are illegal constructors caused a test failure. Fixing _that_ is a big enough issue to punt to 7.12. Fixing that right might also let us fix things like dataToExpQ for things like Maps, Sets, and other containers that also have to use the virtual constructor model to preserve internal invariants, which would be a pretty big boon to heavy users of template haskell. -- Ticket URL: http://ghc.haskell.org/trac/ghc/ticket/10011#comment:11 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler

#10011: The Data instance for Ratio violates internal invariants. -------------------------------------+------------------------------------- Reporter: ekmett | Owner: Type: bug | Status: new Priority: normal | Milestone: 7.10.1 Component: Core Libraries | Version: 7.10.1-rc1 Resolution: | Keywords: Operating System: Unknown/Multiple | Architecture: Type of failure: Incorrect result | Unknown/Multiple at runtime | Test Case: Blocked By: | Blocking: Related Tickets: | Differential Revisions: Phab:D625 -------------------------------------+------------------------------------- Comment (by ekmett): You only need {{{

...

...

:m + Data.Data Data.Ratio }}}

-- Ticket URL: http://ghc.haskell.org/trac/ghc/ticket/10011#comment:13 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler

#10011: The Data instance for Ratio violates internal invariants. -------------------------------------+------------------------------------- Reporter: ekmett | Owner: Type: bug | Status: new Priority: normal | Milestone: 7.10.1 Component: Core Libraries | Version: 7.10.1-rc1 Resolution: | Keywords: Operating System: Unknown/Multiple | Architecture: Type of failure: Incorrect result | Unknown/Multiple at runtime | Test Case: Blocked By: | Blocking: Related Tickets: | Differential Revisions: Phab:D625 -------------------------------------+------------------------------------- Comment (by ekmett): Basically (==) on Ratio rightfully assumes that the rational is already in lowest terms, and just compares values. {{{

...

...

2 % 4 1 % 2 }}}

The test there sneaks into (1 % 2) with the old illegal Data instance and turns it into (2 :% 4), which isn't reduced, demonstrating that it is possible to produce an illegal Rational with the current instance. You could also other illegal values such as infinities, NaN-like things, negative denominators, etc. before by playing with Data.Data. All of which are ruled out by design elsewhere in the API for Rational. -- Ticket URL: http://ghc.haskell.org/trac/ghc/ticket/10011#comment:15 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler

#10011: The Data instance for Ratio violates internal invariants. -------------------------------------+------------------------------------- Reporter: ekmett | Owner: Type: bug | Status: closed Priority: normal | Milestone: 7.10.1 Component: Core Libraries | Version: 7.10.1-rc1 Resolution: fixed | Keywords: Operating System: Unknown/Multiple | Architecture: Type of failure: Incorrect result | Unknown/Multiple at runtime | Test Case: Blocked By: | tests/numeric/should_run/T10011 Related Tickets: | Blocking: | Differential Revisions: Phab:D625 -------------------------------------+------------------------------------- Changes (by thoughtpolice): * status: new => closed * testcase: => tests/numeric/should_run/T10011 * resolution: => fixed Comment: Testcase added and merged into `ghc-7.10` and `master`. -- Ticket URL: http://ghc.haskell.org/trac/ghc/ticket/10011#comment:17 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler