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

Do not use type class instances which are not exported #605

Merged
merged 2 commits into from
Sep 21, 2014
Merged

Do not use type class instances which are not exported #605

merged 2 commits into from
Sep 21, 2014

Conversation

paf31
Copy link
Contributor

@paf31 paf31 commented Sep 21, 2014

Fixes #594

And maybe also the other bug, not sure....

@garyb Mind reviewing this one, please?

@coveralls
Copy link

Coverage Status

Coverage increased (+0.03%) when pulling 1baf09e on 594 into caa1dc6 on master.

@garyb
Copy link
Member

garyb commented Sep 21, 2014

Looks about right to me, and probably does fix the other bug too. 👍

@paf31
Copy link
Contributor Author

paf31 commented Sep 21, 2014

It does. Resolves #562.

Does the code look ok? The change is actually quite simple - it just looks for the appropriate export reference for the instance and then marks the TypeClassDictionaryInScope as exported, so that it isn't imported in an ImportDeclaration later on.

@garyb
Copy link
Member

garyb commented Sep 21, 2014

Yep, the diff made things look worse than they are because of introducing gos etc, but it all looked sensible to me. 👍

paf31 added a commit that referenced this pull request Sep 21, 2014
Do not use type class instances which are not exported
@paf31 paf31 merged commit 73a023f into master Sep 21, 2014
@paf31 paf31 deleted the 594 branch September 21, 2014 19:17
@puffnfresh
Copy link
Contributor

Instances should never be referred to by name. How are we meant to export them?

@paf31
Copy link
Contributor Author

paf31 commented Sep 21, 2014

They're not referred to by name. They get exported implicitly if the class and types are exported. The problem was that the import mechanism didn't check that correctly.

@puffnfresh
Copy link
Contributor

Awesome stuff :shipit:

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.

Instances don't get exported when module exports are provided
4 participants