Skip to content

Fix shadow and pattern warnings #36

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

Merged
merged 2 commits into from
Aug 3, 2015
Merged

Fix shadow and pattern warnings #36

merged 2 commits into from
Aug 3, 2015

Conversation

garyb
Copy link
Member

@garyb garyb commented Aug 3, 2015

unsafeThrow is a bit dodgy, but getting meaningful errors from unsafe failures may be worthwhile? I think we do that in the unsafe FFI in some other places.

@garyb
Copy link
Member Author

garyb commented Aug 3, 2015

I'll use this approach elsewhere too if we're happy with it. I don't really want to make a public unsafeThrow function though, so it will mean duplicating it a bunch of times which probably isn't great either. Thoughts?

@paf31
Copy link
Contributor

paf31 commented Aug 3, 2015

I think unsafeThrow is fine, but I wonder if it should go into different package. Seems like it would be useful elsewhere.

I was planning to create purescript-partial if we went with the Partial type class idea on top of the exhaustivity checker, so maybe that would be a good place to put it, and call it patternMatchFailure or something like that?

@paf31
Copy link
Contributor

paf31 commented Aug 3, 2015

Everything looks great, by the way.

@garyb
Copy link
Member Author

garyb commented Aug 3, 2015

Yeah that makes sense actually. I assume purescript-partial would be a core library?

@paf31
Copy link
Contributor

paf31 commented Aug 3, 2015

Yes I think so. It would probably just define the unsafePartial function we discussed before:

unsafePartial :: forall a. (forall dummy. (Partial) => a) -> a
unsafePartial f = unsafeCoerce f unit

and possibly some helper functions, like something to rethrow the pattern match error with a useful message.

Incidentally, I'd like to be able to remove the forall dummy that's in there. The parser doesn't allow constrained types without a forall right now.

@garyb
Copy link
Member Author

garyb commented Aug 3, 2015

Perhaps we should have it as patternMatchFailure :: Partial => String -> a then too - that way we can use the Partial class without having magic compiler support: to get rid of the warning, you'd use patternMatchFailure, but to use that it imposes the Partial class requirement (unless the unsafePartial coercion is used internally, but we can recommend against that).

@paf31
Copy link
Contributor

paf31 commented Aug 3, 2015

But then every module would have a potential dependency on partial right? That seems like it could lead to unhelpful errors. I was thinking I'd just insert a Partial constraint into an otherwise clause in any partial case expression. That way, you'd get a No instance for Partial error, which we could tidy up in Errors.

@garyb
Copy link
Member Author

garyb commented Aug 3, 2015

Re: dependency, that's true: maybe another option would be to put Partial in the Prelude then?

How would it lead to unhelpful errors though? I think adding a Partial constraint in the compiler would more likely lead to unhelpful errors - unless we're saying Partial is defined in Prim - as otherwise it still needs to "come from" somewhere (and you'd still need to include purescript-partial so you could unsafePartial eventually).

@paf31
Copy link
Contributor

paf31 commented Aug 3, 2015

Yes, I'm suggesting Partial should go in Prim. I'd like to avoid getting errors like Module Prelude not found. We don't have any hard dependencies, even on Prelude, in the compiler right now, do we? Only in PSCi.

@garyb
Copy link
Member Author

garyb commented Aug 3, 2015

Yeah, that's right.

I suppose the advantage of the compiler adding the constraint is that it turns these exhaustivity warnings into actual errors, in that the Partial constraint will need adding to the function's type... but since we're not doing that right now anyway, what should I do about this for now?

We'll end up with an almost universal purescript-partial dependency if I do do what you suggested above, creating patternMatchFailure in there. Should I just go with this approach everywhere for now, and then we can Partialify all the libraries once the compiler support is there?

@paf31
Copy link
Contributor

paf31 commented Aug 3, 2015

Yes, I think that's the best approach. So we'll leave unsafeThrow here for now or create purescript-partial?

@garyb
Copy link
Member Author

garyb commented Aug 3, 2015

Yeah I think so, I'll just duplicate it as needed for now, and then can take it all out again for 0.7.3 or whenever Partial appears.

@paf31
Copy link
Contributor

paf31 commented Aug 3, 2015

Sounds good.

@paf31
Copy link
Contributor

paf31 commented Aug 3, 2015

🚢

garyb added a commit that referenced this pull request Aug 3, 2015
Fix shadow and pattern warnings
@garyb garyb merged commit 8c3c33b into master Aug 3, 2015
@garyb garyb deleted the fix-warnings branch August 3, 2015 16:35
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