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.

Sign up for GitHub

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

Jump to bottom

Makes dictionary argument exclusion logic in Tactic plugin more robust #508

Merged
merged 9 commits into from
Nov 25, 2020
Merged

Makes dictionary argument exclusion logic in Tactic plugin more robust #508

merged 9 commits into from
Nov 25, 2020

Conversation

konn
Copy link
Collaborator

@konn konn commented Oct 17, 2020 •
edited
Loading

It now uses isPredTy to detect dictionary arguments in dataConInstOrigArgTys', which gives us a more robust way to reject dictionary and coercion arguments simultaneously.
Fixes isovector#32.

@konn konn mentioned this pull request Oct 17, 2020
Closed
@konn
Copy link
Collaborator Author

konn commented Oct 17, 2020

Hmm, seems failing with >= 8.8... I remembered that some change(s) have occurred in GHC 8.8 and some (seemingly unnecessary) tweaks needed to detect PredTypes correctly. I will fix that.

@konn konn marked this pull request as draft October 17, 2020 06:58
@konn
Copy link
Collaborator Author

konn commented Oct 17, 2020

Hmm, just replacing dataConInstArgTys with dataConInstOrigArgTys results in panic as before. We need more careful look on this...

@jneira jneira requested a review from isovector October 19, 2020 05:29
@konn
Copy link
Collaborator Author

konn commented Nov 23, 2020

Sorry for leaving this pull-request as Draft for so long time!
I think I've just located the cause of the bug: we must call dataConInstOrigArgTys function with not only universal type variables (i.e. the explicit type arguments of GADTs) but also existential ones.

Hopefully, recent commits should address this situation properly, so it can be merged when the CI runs successfully!

@konn konn marked this pull request as ready for review November 23, 2020 06:29
@konn
Copy link
Collaborator Author

konn commented Nov 23, 2020

OK, it seems All the CI passed! Ready for reviews.

isovector
isovector reviewed Nov 23, 2020
View reviewed changes
Copy link
Collaborator

@isovector isovector left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor change and a quick question of clarification, but otherwise lgtm!

plugins/tactics/src/Ide/Plugin/Tactic/CodeGen.hs Outdated Show resolved Hide resolved
@isovector
Copy link
Collaborator

Happy to merge as soon as CI goes green!

@jneira jneira merged commit cb9a38b into haskell:master Nov 25, 2020
@konn konn deleted the tactic-exclude-coercions branch November 25, 2020 06:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@isovector isovector isovector approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Split correctly on datatypes with existential type variables
3 participants
@konn @isovector @jneira