Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow getting type errors without having to kill the interpreter #48

Merged
merged 3 commits into from
Jan 29, 2018

Conversation

Wizek
Copy link
Contributor

@Wizek Wizek commented Jan 15, 2018

Fixes #24

Also, can anyone think of a better name for the getType function?

@mvdan
Copy link
Contributor

mvdan commented Jan 16, 2018

If it's "similar to typeChecks, but gives more information", perhaps name it something similar? Also, what is the string that may be returned if there isn't an error - the same as typeOf?

If it only ever returned an error, you could call it typeCheckError. But if it can return something else too, perhaps typeCheckWithError.

Also please squash your commits when you're done.

@Wizek
Copy link
Contributor Author

Wizek commented Jan 16, 2018

Yes, Right contains the inferred valid type, just like typeOf returns. I thought the most powerful API would be one that returns Either, so we can get the most info with a single round trip to the interpreter. Also, if that info is not needed, one can still throw it away with isRight and isLeft and similar combinators.

@mvdan
Copy link
Contributor

mvdan commented Jan 16, 2018

Sure, makes sense. My Haskell/GHC fu is simply not good enough to realise whether this kind of optimization makes sense here. Perhaps GHC is smart enough to deduplicate part of the work if it's split into two separate expressions.

In any case, this just needs a better name to be merged. getType is too confusing given that we already have typeOf. I'd go with typeCheckWithError, but it's up to you.

@Wizek
Copy link
Contributor Author

Wizek commented Jan 16, 2018

Yes, I like the direction of your suggestion of typeCheckWithError, but I wasn't sure about it till now since the implementation I wrote can return the valid type too.

I just had the idea of typeChecksWithDetails which I think is more accurate, so I'll be renaming to this, then squashing and pushing.

Exporting the latter to make it more possible for others to catch errors too
Also added a relevant note on `-fdefer-type-errors`
@mvdan
Copy link
Contributor

mvdan commented Jan 18, 2018

Travis was down yesterday, so I restarted the CI builds to get it green.

By the way, why are you exporting onCompilationError?

@Wizek
Copy link
Contributor Author

Wizek commented Jan 18, 2018

My thinking on that export was multi-fold:

  • As I write in the commit message: "Exporting the latter to make it more possible for others to catch [interpreter] errors too"

  • Although, since then looking into it a bit more it seems that anyone also importing Control.Monad.Catch variant of catch can redefine the same combinator.

  • But then again, why would we want to make people have to redefine it if the code already exists in the library?

    • At best, they'll have to discover it digging through the source (no mention of it on haddock) and copy-paste;
    • at worst they would think it doesn't exist and have them rediscover and write this function by themselves.

    For example, I had no idea that the Interpreter monad made productive use of catchable/recoverable exceptions that didn't leave that thread in an undefined error state before stumbling onto functions like this in the source.

  • I also have a near-fundamental disagreement with hiding internal, already working, potentially useful code unreachable for users of libraries. Perhaps, if this is unstable code, it could be exposed as Hint.Internal.Foo if we don't want to make it part of the officially supported API set.

Any thoughts on the above?

@mvdan
Copy link
Contributor

mvdan commented Jan 18, 2018

As I write in the commit message

Whoops, missed that. GitHub doesn't tell you when someone updates the commits in a pull request, and it's really annoying.

Perhaps, if this is unstable code, it could be exposed as Hint.Internal.Foo if we don't want to make it part of the officially supported API set.

Let's do that. Not because it could be unstable or break in the future, but rather because it means we don't have to think too hard about the API just yet. We can change the name or signature at a later time without worry.

@mvdan
Copy link
Contributor

mvdan commented Jan 25, 2018

Ping, @Wizek?

@Wizek
Copy link
Contributor Author

Wizek commented Jan 27, 2018

I'm giving this another go and wrestling with haddock at the moment.

For some reason onCompilationError doesn't show up in the docs when I try this:

-- | In this module we intend to export some internal functions.
--
-- __Important note__: the authors of this library imply no assurance whatsoever
-- of the stability or functionality of the API exposed here, and compatibility
-- may break even by minor version changes. Rely on these at your
-- own risk.
--
-- The reason for showing them here is to aid discoverability
-- of already written code and prevent having to reinvent the wheel from
-- scratch if said wheel is already invented.
--
-- Some [to be finished...]

module Hint.Internal (module ReExport) where

import Hint.Typecheck as ReExport (onCompilationError)

Any ideas?

p.s. this may be relevant: haskell/haddock#563

@gelisam
Copy link
Contributor

gelisam commented Jan 27, 2018 via email

@Wizek
Copy link
Contributor Author

Wizek commented Jan 27, 2018

That did the trick, thanks @gelisam!

and export onCompilationError through it instead
@Wizek
Copy link
Contributor Author

Wizek commented Jan 27, 2018

Pushed. Thanks again @gelisam for the rapid response.

I've also been wondering if there was a simple/easy way of exporting the whole of Hint.* under the Hint.Internal.* namespace. Perhaps with HaRe, though I haven't had the fortune to give that a try yet.

And at any rate that question is quite tangential to this PR, so I wouldn't want to hold this up longer for that. Can be done separately at any time.

@mvdan mvdan merged commit fa57ce8 into haskell-hint:master Jan 29, 2018
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.

3 participants