Skip to content

Bring polykinds branch up to master#3

Merged
natefaubion merged 2 commits intonatefaubion:polykindsfrom
joneshf:joneshf/polykinds
Mar 12, 2020
Merged

Bring polykinds branch up to master#3
natefaubion merged 2 commits intonatefaubion:polykindsfrom
joneshf:joneshf/polykinds

Conversation

@joneshf
Copy link

@joneshf joneshf commented Mar 12, 2020

As mentioned in the PR to split into packages, I'd offered to do this merge. Here it is!

This fails locally for me with:

ests: user error (Error found:
in module Data.Show
at /home/joneshf/programming/purescript/purescript/tests/support/bower_components/purescript-prelude/src/Data/Show.purs:53:10 - 53:22 (line 53, column 10 - line 53, column 22)

  Could not match kind
        
    Type
        
  with kind
              
    Constraint
              

while checking that type IsSymbol key
  has kind Constraint
in type class instance
                                                         
  Data.Show.ShowRecordFields (Cons key focus rowlistTail)
                             row                         
                                                         

See https://github.com/purescript/documentation/blob/master/errors/KindsDoNotUnify.md for more information,
or to contribute content related to this error.

)
                               

But then, the base polykinds branch fails with the same error for me. Might be something with my setup.

Feel free to merge it or reject it. Up to you!

joneshf added 2 commits March 11, 2020 17:40
* Move `SimpleErrorMessage` to `Errors`

In 4828b5d, we moved
`Language.PureScript.Errors.SimpleErrorMessage` to
`Language.PureScript.AST.Declarations`. Per the comment in the Pull
Request:
purescript#2230 (comment),
we did this because we wanted to move
`Language.PureScript.Errors.ErrorMessageHint` into
`Language.PureScript.AST.Declarations.Expr`.

Thing is, we didn't have to move everything just to move
`Language.PureScript.Errors.ErrorMessageHint`. In fact, by making this
move, we forced the AST to require knowledge about every type of error
that can be created. For example, the AST has to know about parse
errors (something that should happen before creating an AST) and code
generation errors (something that should happen after creating an AST).
In an ideal world, the AST would only know about errors that come about
from modifications to the AST itself.

The real issue that this led to was that the AST had to know about the
errors that came about from the CST. Given that the CST is the surface
syntax of the AST, that requirement seems a bit off. With the
dependencies inverted this way, it's hard to decouple the AST from the
CST.

We take a step towards the ideal world by moving
`Language.PureScript.AST.Declarations.SimpleErrorMessage` back into
`Language.PureScript.Errors`. This should allow us to extract the AST
from the `purescript` package itself such that it can be built upon in
isolation from the rest of the package.

We should revisit why
`Language.PureScript.AST.Declarations.SimpleErrorMessage` is so massive
in the future. It will probably make sense to separate the data type
into the different categories of errors so those errors can live closer
to where they should be constructed.

* Split the constants based on Prim and Prelude

Prim stuff is part of PureScript the language, Prelude is part of the
PureScript conventions. PureScript the language doesn't require an
existence of Prelude in order to work properly. Instead of mixing these
two levels, we separate the constants so we can pull the Prim constants
out with the AST.

In order to not break everything, we re-export the constants from the
original module. We should put a timer on this and remove the re-exports
after that point so we don't wind up with a bunch of cruft like the
other re-export modules.

* Extract AST to its own package

We've gone quite a ways with a sinle package for everything PureScript
related. This has led to incredibly long compile times if you want to
consume PureScript the library, as you have to compile everything no
matter how much you want to really use. To help with downstream
consumers, we start splitting into packages of useful functionality.

We start with the AST as this _should_ be the very basis of everything
PureScript related. From the AST, someone ought to be able to build
packages atop it that add more functionality to the PureScript language.
For instance, we can split out the CST as a package that builds atop the
AST and adds more functionality (namely, parsing).

It's maybe a little disheartening that there don't appear to be any
tests around the AST. Maybe with things separated out a bit more, it'll
be more inviting to write some tests.

The modules we move here might not be all that intuitive, but they seem
to be the core of what the PureScript AST is all about. The only
questionable modules are the `Control.Monad.Supply` and
`Control.Monad.Supply.Class` modules. We could flip the dependency in
`Language.PureScript.Names` and move those values into the
`Control.Monad.Supply.Class` module. It's unclear which way to go.

Since the `Control.Monad.Supply.Class` module isn't in the
`Language.PureScript` hierarchy, it might make sense for it to stay in
the top-level. However, it might be too general for that and should
ideally live in its own package (maybe even outside the project?).

We also use PackageImports judiciously. When trying to figure out which
dependencies went where, it was much harder than it should have been to
figure this stuff out. When trying to figure out which dependencies were
used and necessary, it was even harder to figure what the situation was.
Finally, when trying to figure out where `Control.Monad.Supply.Class`
came from, it took a frustratingly long amount of time to realize it was
a module we had defined in the project.

For all of these reasons, we use PackageImports to help future us know
which dependencies are used, and where. Of course there are tools like
weeder (https://github.com/ndmitchell/weeder) to help with finding
unused dependencies and whatnot. But, it's also good to know statically
what is being used and to allow other tools to be used.

Finally, we update the license-generator to work with this split off
package. We might have to do more work before we're done, but we can at
least generate the license file in a similar manner to how we do at the
top-level.

* Extract the CST to a package

We extract out the CST to its own package that builds atop the AST. This
should make it a bit easier to build other tooling atop the CST. In
particular, anything that might need to parse PureScript files can now
depend on this package to do so.

Since this new package only depends on `purescript-ast` (and its
transitive dependencies), it can be built in a reasonable amount of
time.

It's important to note that we don't pull out `Language.PureScript.CST`.
That module is straddling the line between a re-exporting "prelude" and
an adapter between `purescript-ast` and `purescript`. Since it deals
with errors, and errors currently exist at the top-level, we cannot yet
move this module down into `purescript-cst`.

Probably what needs to happen is that we make a split between
`Language.PureScript.CST` being a re-exporter and
`Language.PureScript.CST` doing adapter work. Not clear which way to go
on this one as both seem viable avenues.

It's really great to see a test suite around some of this deep down
functionality. Maybe we can use this example to feed back into the AST
and make a test suite there.

* Move sensible extensions into `default-extensions`

Per review, we want our extension strategy to be that we have a uniform
language across the codebase with the exception of questionable
extensions like `TemplateHaskell`. We move as many sensible extensions
to the `default-extensions` field in `package.yaml` so we start building
up our uniform language.

There are three other extensions we don't move at the moment: `CPP`,
`DeriveAnyClass`, and `ImplicitParams`.

* `CPP` - This extension is just as questionable (if not more so) as
    `TemplateHaskell`. Following a similar reasoning, we force people to
    turn it on explicitly.

    It's only used in the `Language.PureScript.Crash` module. That
    module might be better off if we stopped casing on the compiler
    version. It doesn't seem like we support older versions of GHC
    anymore, so we might be better off dumping the extension outright.
* `DeriveAnyClass` - This extension is known to interact weirdly with
    `GeneralizedNewtypeDeriving`:
    https://gitlab.haskell.org/ghc/ghc/wikis/commentary/compiler/deriving-strategies.
    Since we use `GeneralizedNewtypeDeriving` in more places, and
    `DerivingStrategies` exists to address the issues with
    `DeriveAnyClass`, we don't turn this on across the entire codebase.
* `ImplicitParams` - Similar to `CPP`, this extension is only used in
    the `Language.PureScript.Crash` module. And similarly, if we don't
    support older versions of GHC, we can drop this extension outright.

If any of this is the wrong decision, we can always change it.

* Move sensible extensions to `default-extensions`

Similar to the motivation behind moving `purescript-ast`s extensions to
the `default-extensions` field of its `package.yaml`, we follow suit and
move the sensible extensions to `purescript-cst`s `default-extensions`
field of its `package.yaml`.

We ended up dropping `MonoLocalBinds`, as the module compiles without
the extension.

* Only generate license for `purescript` package

The license generator exists for the sake of distributing the licenses
that the `purescript` binary uses. We don't need to use the generator
for the other packages. We want the other packages to have a normal
license file.

We also roll back the changes to the `Makefile`.

* Move `purescript-ast` to version 0.1.0.0

Per review, this seems like a way to go that should foster each package
being able to evolve in a way that isn't hindered by the rest of the
packages at large.

* Move `purescript-cst` to version 0.1.0.0

Per review, this seems like a way to go that should foster each package
being able to evolve in a way that isn't hindered by the rest of the
packages at large.

* Remove re-export compatibility module

We only kept `Language.PureScript.Constants` around so we could figure
out the changes while we were splitting out `purescript-ast`. Now that
things have settled down a bit, we remove the module and use the other
imports directly.

* Setup sdists for each package

Now that we have more than one package, we have to do things a little
different. `stack sdist` will create an sdist for each package. This
means we end up with a directory that looks like:

```console
$ tree sdist-test
sdist-test
├── purescript-0.13.6.tar.gz
├── purescript-ast-0.1.0.0.tar.gz
└── purescript-cst-0.1.0.0.tar.gz

0 directories, 3 files
```

If we try to use `tar -xzf sdist-test/purescript-*.tar.gz -C sdist-test
--strip-components=1`, the glob will expand to all three sdists and fail
with a cryptic error:

```console
$ tar -xzf sdist-test/purescript-0.13.6.tar.gz sdist-test/purescript-ast-0.1.0.0.tar.gz sdist-test/purescript-cst-0.1.0.0.tar.gz -C sdist-test --strip-components=1
tar: sdist-test/purescript-ast-0.1.0.0.tar.gz: Not found in archive
tar: sdist-test/purescript-cst-0.1.0.0.tar.gz: Not found in archive
tar: Exiting with failure status due to previous errors
```

We change to mirroring the directory structure within the `sdist-test`
directory, and running that way.

It's a bit unfortunate that the top-level package has to be handled
differently, but this should help us move forward.
@natefaubion
Copy link
Owner

natefaubion commented Mar 12, 2020

Thanks! I really appreciate this. I think the error is probably because you need to nuke your .test-modules folder.

@natefaubion natefaubion merged commit eb8e202 into natefaubion:polykinds Mar 12, 2020
@joneshf joneshf deleted the joneshf/polykinds branch March 13, 2020 04:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants