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

Fix a bug in tactics preventing split of split #520

Merged
merged 6 commits into from
Oct 21, 2020

Conversation

isovector
Copy link
Collaborator

@isovector isovector commented Oct 20, 2020

The auto tactic attempts to prune unhelpful branches in order to avoid an exponential blowup of search space. On one these optimizations is to not build a data constructor if it doesn't result in new types to solve. For example, we're trying to avoid the following pathological example:

data Tree a = Leaf a | Branch (Tree a) (Tree a)

-- given the following hole:
pureTree :: a -> Tree a
pureTree a = _

-- we DO NOT want to fill it with
pureTree a = Branch _ _

The reasoning here is that both goals in Branch _ _ have type Tree a, which is already the type we're trying to solve, so introducing Branch doesn't make any progress.

This check is performed in the splitAuto tactic, but I got it backwards and it wasn't explicitly tested for. The only code which hit it was pure @[] --- but because [] doesn't have any subgoals, this hit a vacuous case and flipped the result of the bad logic. Two wrongs made a hard to find bug.

This PR:

  1. Fixes the reversed logic in splitAuto
  2. Has a special case for nullary data constructors, fixing the bug cause by vacuousness.
  3. Adds property tests ensuring we can auto our way through any permutation of a tuple (which is where we originally noticed the bug)
  4. Prevents unsafeRender from crashing when unsafeGlobalDynFlags is unset, such as during testing.
  5. Moves tactic solution tracing into the plugin, so it won't run during tests.

@isovector isovector mentioned this pull request Oct 20, 2020
7 tasks
Copy link
Member

@jneira jneira left a comment

Choose a reason for hiding this comment

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

LGTM

@jneira
Copy link
Member

jneira commented Oct 21, 2020

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