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

HOTFIX: equal?/2 should not assert #18

Merged
merged 1 commit into from
Jul 2, 2018
Merged

HOTFIX: equal?/2 should not assert #18

merged 1 commit into from
Jul 2, 2018

Conversation

expede
Copy link
Member

@expede expede commented Jul 2, 2018

Root Issue

  • equal?/2 began using assert internally as of 640c0fd.
  • This library assumes that functions avoid immediately blowing up errors (unless they have a !)
    • In proper functional style, we prefer keeping helpers "dumb" and delegate control to the caller
  • Critically, we need to be able to check equal?(a, b) or equal?(x, y)
    • Embedding an assert makes this impossible

Steps to Reproduce

On my machine, this does not occur when compiling Witchcraft with fresh deps and _build. I was able to reproduce with a new application that used Witchcraft as a dependency. This typically fails on Ord.ex prop checks.

Edited deps/type_class/lib/type_class/property.ex, removing the asserts, and everything works. The offending line requires checking two possible options with an or. Since assert will blow up on a single failure, this makes any boolean logic impossible.

Still Need Better Fail Reporting

While the errors generated by assert were more detailed, they were often of little use. For example, this doesn't tell me anything about the values that lead to failure:

Assertion with == failed
code:  assert left == right
left:  :equal
right: :lesser

Better error messages would be very useful. It's not new information that the prop-testing system could be improved, and this should be explored.

History

@SchrodingerZhu reported that Algae was unable to build, failing compile-time prop checks.

Some discussion occurred in that thread. For some reason, I did not receive an email until 5 days later. This isn't the first time this has happened. I will explore my spam filtering and GitHub email settings.

@expede expede added the WIP label Jul 2, 2018
@expede expede self-assigned this Jul 2, 2018
@expede expede removed the WIP label Jul 2, 2018
@expede expede merged commit 36611c2 into master Jul 2, 2018
@expede expede deleted the revert-assert branch July 2, 2018 21:40
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.

None yet

1 participant