Skip to content

Improved error messages in the constraint solver#2230

Merged
paf31 merged 2 commits intomasterfrom
tc-errors
Jul 26, 2016
Merged

Improved error messages in the constraint solver#2230
paf31 merged 2 commits intomasterfrom
tc-errors

Conversation

@paf31
Copy link
Contributor

@paf31 paf31 commented Jul 16, 2016

Behold:

  No type class instance was found for

    Issue1310.Inject (Eff
                        ( console :: CONSOLE
                        | t0
                        )
                     )
                     (Eff
                        ( oops :: Oops
                        | eff1
                        )
                     )

  The instance head contains unknown type variables. Consider adding a type annotation.

while checking that expression inj (log "Oops")
  has type forall eff.
             Eff
               ( oops :: Oops
               | eff
               )
               Unit
while applying a function inj
  of type forall a g f. (Inject f g) => f a -> g a
  to argument log "Oops"
in value declaration main

where eff1 is a rigid type variable
        bound at line 18, column 1 - line 18, column 23
      t0 is an unknown type

This probably needs rebasing, I wrote it on the plane.

paf31 added 2 commits July 15, 2016 16:40
Behold:

  No type class instance was found for

    Issue1310.Inject (Eff
                        ( console :: CONSOLE
                        | t0
                        )
                     )
                     (Eff
                        ( oops :: Oops
                        | eff1
                        )
                     )

  The instance head contains unknown type variables. Consider adding a type annotation.

while checking that expression inj (log "Oops")
  has type forall eff.
             Eff
               ( oops :: Oops
               | eff
               )
               Unit
while applying a function inj
  of type forall a g f. (Inject f g) => f a -> g a
  to argument log "Oops"
in value declaration main

where eff1 is a rigid type variable
        bound at line 18, column 1 - line 18, column 23
      t0 is an unknown type
@paf31
Copy link
Contributor Author

paf31 commented Jul 16, 2016

Perhaps we should give up on trying to separate the various AST modules, and just create one big Types module. Here, it was pretty much unavoidable, and I had to merge the errors types into the AST module. Merging in Binders would let us fix the String hack I came up with for the Partial error messages too.

It would probably help discoverability overall too.

@paf31
Copy link
Contributor Author

paf31 commented Jul 18, 2016

Fixes #1866 and #1433

@garyb
Copy link
Member

garyb commented Jul 19, 2016

Merging in binders would mean we need to merge CoreFn (and probably CoreImp when it's done) in with the input AST also, which I'm not so sure would be great? I don't know.

This is great though 👍 🎉

@paf31 paf31 merged commit 4828b5d into master Jul 26, 2016
@paf31 paf31 deleted the tc-errors branch July 26, 2016 02:50
joneshf added a commit to joneshf/purescript that referenced this pull request Feb 16, 2020
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.
joneshf added a commit that referenced this pull request Mar 12, 2020
* Move `SimpleErrorMessage` to `Errors`

In 4828b5d, we moved
`Language.PureScript.Errors.SimpleErrorMessage` to
`Language.PureScript.AST.Declarations`. Per the comment in the Pull
Request:
#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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants