Bring polykinds branch up to master#3
Merged
natefaubion merged 2 commits intonatefaubion:polykindsfrom Mar 12, 2020
Merged
Conversation
* 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.
Owner
|
Thanks! I really appreciate this. I think the error is probably because you need to nuke your |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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:
But then, the base
polykindsbranch fails with the same error for me. Might be something with my setup.Feel free to merge it or reject it. Up to you!