search

ku-fpg / sunroof-compiler

Monadic Javascript Compiler
BSD 3-Clause "New" or "Revised" License
73 stars 6 forks source link

Add ambiguous types language extension #41

Closed niilohlin closed 9 years ago

niilohlin commented 9 years ago

Sunroof would not compile without the ambiguous types language extension.

jbracker commented 9 years ago

What GHC version are you trying to compile it with?

niilohlin commented 9 years ago

7.8.3

jbracker commented 9 years ago

I have pulled in your fix for 7.8.3

But: GHC prior to 7.8 does not support that language extension. Therefore I had to make it conditional: 64c261c4df705e292b5b083ee10eeec9455d3875

jbracker commented 9 years ago

Can you please check if this fix works with GHC 7.8? I don't have it installed currently. I could just confirm that the code works for GHC 7.6

niilohlin commented 9 years ago

Yes it works on GHC 7.8.3 for me, but i had to change the "build-depends: base >= 4.3.1 && < 4.7"to"build-depends: base >= 4.3.1 && < 4.8"in the .cabal file for it to compile. Date: Sat, 29 Nov 2014 13:42:29 -0800 From: notifications@github.com To: sunroof-compiler@noreply.github.com CC: niil.94@hotmail.com Subject: Re: [sunroof-compiler] Add ambiguous types language extension (#41)

Can you please check if this fix works with GHC 7.8? I don't have it installed currently. I could just confirm that the code works for GHC 7.6

— Reply to this email directly or view it on GitHub.

                  =
RyanGlScott commented 9 years ago

Strangely enough, I was able to build sunroof-compiler successfully on GHC 7.8.3 even without AllowAmbiguousTypes, although if it fixes the build for someone else, then I'm all for it. You do need to bump the upper version bound of base, though.

jbracker commented 9 years ago

The upper version limit for base and the usage of the package tagged are tied together.

In version 4.7.0.0 base introduced the module Data.Proxy, which provides the data type Proxy a which is used in a couple of places (e.g. here and here).

Previous to version 4.7.0.0 of base that data type is/was provided by the package tagged in the module Data.Proxy.

From GHC 7.6 to 7.8 the version of base changed from 4.6 to 4.7.

Which explains why compiling sunroof with GHC 7.6 on my machine led me to introduce the depency tagged once more (and yes, I was confused why this flaw is within the released version, but I did not check the code on hackage).

However, noticing the changes that occured in base I had the (rather stupid) idea of imposing an upper version limit on base. I had the faint fear of this leading to ambigious module resolution problems.

It seems to work to just leave the dependency and remove the upper version limit on base, as it seems to work just fine with GHC 7.8 and also works with GHC 7.6. Though sooner or later it probably would be good to remove the tagged dependency for GHC 7.8+, since it seems to be obsolete with the most recent versions of GHC. I guess doing this properly would require conditional build dependencies in the cabal file.

RyanGlScott commented 9 years ago

Sorry about the tagged confusion; I definitely jumped the gun by removing it. Commit bece9b75547b2cad69b12824f86eda2a816c5dbd changes the .cabal file so that it only declares a dependency on tagged if the version of GHC is less than 7.7.

RyanGlScott commented 9 years ago

Actually, I think I've figured out what is causing this ambiguous types problem. The issue does not exist in HEAD, since I can compile Language.Sunroof.Types without AllowAmbiguousTypes on GHC 7.8.3 without issues. I encounter the issue when trying to build from Hackage, though, likely due to an (old) line of code from Language.Sunroof.Types:

-- | Functions may be the result of a branch.
instance (SunroofArgument a, Sunroof r) => IfB (JSContinuation a) where
  ifB = jsIfB

Because r isn't used to parameterize any types, it is ambiguous. In HEAD, however, this code has since been fixed, so AllowAmbiguousTypes should no longer be necessary.

jbracker commented 9 years ago

I have relaxed the upper version limits on base again (479cd1dcb55eaaa6f3797f345416e5b3632b1516, 75c543bb1b6af09cba10a18c48730bf95801b53c). And it does compile without AllowAmbiguousTypes using GHC 7.6. Can you confirm it compiles without in GHC 7.8?

@niilohlin: Did you install from Hackage or the GitHub repository? Can you check if it compiles from the repository without the AllowAmbiguousTypes extension?

wojtnar commented 9 years ago

I just pulled afresh from github, compiled perfectly with ghc 7.8.3. AmbiguousTypes not needed. Thanks.

niilohlin commented 9 years ago

@jbracker It compiles without AllowAmbiguousTypes from the repo.