Opened 8 years ago

Closed 8 years ago

#5419 closed bug (fixed)

integer-gmp no longer exports S# and J#

Reported by: mikhail.vorozhtsov Owned by: igloo
Priority: highest Milestone: 7.4.1
Component: libraries (other) Version: 7.3
Keywords: Cc: johan.tibell@…, pumpkingod@…
Operating System: Unknown/Multiple Architecture: Unknown/Multiple
Type of failure: None/Unknown Test Case:
Blocked By: Blocking:
Related Tickets: Differential Rev(s):
Wiki Page:

Description

Commit d8f636ec19a21cc4c22a8d1b45217436838aeebb effectively removed the constructors from the package export list and libraries that depend on them (text and HsOpenSSL here) are now broken.

Change History (12)

comment:1 Changed 8 years ago by tibbe

Cc: johan.tibell@… added

comment:2 Changed 8 years ago by tibbe

I don't quite understand the motivation for this change:

No need to export Integer from GHC.Integer.GMP.Internals

This caused an odd dependency in the module hierarchy.

Could someone please elaborate on what this odd dependency is?

We do need to export the constructors somewhere as some algorithms can only be implemented efficiently in terms of them (e.g. hexadecimal encoding of Integers).

comment:3 Changed 8 years ago by tibbe

The dependency chain used to be: GHC.Integer -> GHC.Integer.GMP.Internals -> GHC.Integer.Type.

The dependency chain is now: GHC.Integer -> GHC.Integer.Type -> GHC.Integer.GMP.Internals.

If I understand Ian's intentions right he thinks it's odd for a module in the GHC.Integer.GMP namespace to appear between two modules from the GHC.Integer namespace. Fair enough. Then I suggest we add GHC.Integer.Internals that exports the Integer constructors.

comment:4 Changed 8 years ago by tibbe

Here's a list of packages that are currently broken:

  • blaze-textual-0.2.0.4
  • fixed-precision-0.4.0
  • bytestring-show-0.3.5
  • text-0.11.1.5
  • text-format-0.3.0.5

I don't quite understand the namespace design of integer-gmp: The whole package is for GMP like things (which we derive from the package name) but only parts of the module hierarchy mention GMP. This makes me a bit confused as to where the Integer constructors should be exported from.

comment:5 Changed 8 years ago by tibbe

After thinking about this for a while I've convinced myself that we want to export the constructors from GHC.Integer.GMP.Internals. GHC.Integer and its submodules (except GMP in the case of integer-gmp and Simple in the case of integer-simple) represent the API shared between the two packages. If you want to import something that's specific to one package then you should mention GMP or Simple explicitly. With this reasoning the GHC.Integer.Type module in each package should be moved into the implementation specific namespace (GMP or Simple) and be exported (in part) through GMP.Internals or Simple.Internals.

comment:6 Changed 8 years ago by simonpj

Ian is away, until the middle of nex week. You refactoring makes sense to me.

It's tricky trying to simultaneously support

  • Two different implementations of Integer. That suggests that the representation of Integer should be opaque.
  • Optimisations for small integers, which needs to see the representation.

The two goals are in tension. I believe the uses of S# in client packages are there to optimise the small-integer case (can anyone confirm?).

I wonder whether a way to square this circle would be to have an abstract pseudo-constructor function whose type is that of S#:

 smallInteger :: Int# -> Integer

The idea is:

  • The compiler desugars small Integer constants into uses of smallInteger.
  • Rewrite rules can match on (smallInteger x).
  • smallInteger is inlined late.
  • Two different implementations of the Integer API could implement smallInteger quite differently.

As I understan Johan's proposed change, client packages that mentioned S# would not build against a different Integer implementation. With the above idea, they would.

But I've learned that it's easy to miss something in this territory.

Simon Ian and I were already discussing something like

  bigInteger :: ByteString -> Integer

for big integer constants, to avoid the absurd stuff in #5284. The ByteString (or MachAddr or something, I forget what) is supposed to be the bit representation of the Integer. Different implementations of Integer would mangle it in different ways to build their respective internal representations. Simon

comment:7 Changed 8 years ago by tibbe

Here's and example use of the internals:

https://github.com/bos/text/blob/master/Data/Text/Lazy/Builder/Int.hs#L95

Note that the Integer value being pretty-printed here typically won't be known statically, so optimizing constants won't help here.

Hashing Integers is another important case. We can hash an Integer much faster if we can first hash the Int# component and then zip over the ByteArray#.

We could create a projection function of type Integer -> (Int#, ByteArray#) which would be a simple constructor unwrapping in integer-gmp and some kind of conversion in integer-simple.

Note that packages can access S# directly but still be portable to other integer implementation by using CPP.

comment:8 Changed 8 years ago by simonpj

Well, maybe hashing and printing should be part of the common API, supported by all implementations of Integer.

Or, if you want something super-optimised, a library author might want to depend on a particular implementation of the Integer API, in which case s/he is free to do so.

But you probably don't want to do so accidentally.

comment:9 Changed 8 years ago by tibbe

I actually think the way it used to work was OK. If you wanted to depend on a particular implementation you could import e.g. GHC.Integer.GMP.Internals. I agree in principle that the (internal) module dependencies were a bit wacky, but it seems hard to avoid them given the requirements of PrelNames to name a module exported by all integer-* packages and require that this module must define the data type in question, instead of just re-export it.

comment:10 Changed 8 years ago by igloo

Milestone: 7.4.1
Owner: set to igloo
Priority: normalhighest

comment:11 Changed 8 years ago by pumpkin

Cc: pumpkingod@… added

This would also make the mpfr bindings Ed Kmett and I are working on nicer, as mpfr has dedicated functions for interoperating with gmp integers (it's built on top of gmp).

comment:12 Changed 8 years ago by igloo

Resolution: fixed
Status: newclosed

Fixed by:

commit 65c73842bca2f075b65f418a5ff34753b185e0d7
Author: Ian Lynagh <igloo@earth.li>
Date:   Thu Aug 25 11:18:07 2011 +0100

    Export Integer(..) from GHC.Integer.GMP.Internals again; fixes #5419

    The GMP primitives are now in GHC.Integer.GMP.Prim instead.
Note: See TracTickets for help on using tickets.