Opened 3 years ago

Closed 3 years ago

#10011 closed bug (fixed)

The Data instance for Ratio violates internal invariants.

Reported by: ekmett Owned by:
Priority: normal Milestone: 7.10.1
Component: Core Libraries Version: 7.10.1-rc1
Keywords: Cc: core-libraries-committee@…
Operating System: Unknown/Multiple Architecture: Unknown/Multiple
Type of failure: Incorrect result at runtime Test Case: tests/numeric/should_run/T10011
Blocked By: Blocking:
Related Tickets: Differential Rev(s): Phab:D625
Wiki Page:

Description (last modified by ekmett)

I found this when Simon was cleaning up unused dependencies in

https://phabricator.haskell.org/rGHCc409b6f30373535b6eed92e55d4695688d32be9e#10730

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.

Change History (17)

comment:1 Changed 3 years ago by ekmett

Description: modified (diff)

comment:2 Changed 3 years ago by ekmett

Milestone: 7.10.1

comment:3 Changed 3 years ago by ekmett

Type of failure: None/UnknownIncorrect result at runtime

comment:4 Changed 3 years ago by hvr

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

comment:5 Changed 3 years ago by Herbert Valerio Riedel <hvr@…>

In 79b0d0e633af8302d2dd907663a4a231cd889b67/ghc:

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

comment:6 Changed 3 years ago by hvr

Resolution: fixed
Status: patchclosed

comment:7 Changed 3 years ago by Herbert Valerio Riedel <hvr@…>

In 22c4d60b0665a15535c0ec9fe0b8e65d2c948e7d/ghc:

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

comment:8 Changed 3 years ago by hvr

Owner: ekmett deleted
Resolution: fixed
Status: closednew

comment:9 Changed 3 years ago by Herbert Valerio Riedel <hvr@…>

In 3df429e29b6fabda12af71091ba4ad1360f49b44/ghc:

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

comment:10 Changed 3 years ago by simonpj

So is this done now? Do we need a regression test?

comment:11 Changed 3 years ago 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.

comment:12 Changed 3 years ago by hvr

The following was suggested as regression test by Edward:

>>> :set -XScopedTypeVariables -XTypeOperators -XGADTs
>>> :m + Control.Lens Data.Data.Lens Data.Ratio
>>> let bad = gmapT (\(x :: b) -> case eqT :: Maybe (b :~: Integer) of Nothing -> x; Just Refl -> x * 2) (1 % 2) :: Rational in bad == numerator bad % denominator bad
False

comment:13 Changed 3 years ago by ekmett

You only need

>>> :m + Data.Data Data.Ratio

comment:14 Changed 3 years ago by simonpj

By all means go ahead and add the test, but it'd be good to include comments that articulate what exactly it is testing!

Simon

comment:15 Changed 3 years ago 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.

comment:16 Changed 3 years ago by Austin Seipp <austin@…>

In e02ef0e6d4eefa5f065cc1c33795dfa2114cd58e/ghc:

testsuite: add a regression test for #10011

Signed-off-by: Austin Seipp <austin@well-typed.com>

comment:17 Changed 3 years ago by thoughtpolice

Resolution: fixed
Status: newclosed
Test Case: tests/numeric/should_run/T10011

Testcase added and merged into ghc-7.10 and master.

Note: See TracTickets for help on using tickets.