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

Introduce class MonadSimplify #691

Merged
merged 14 commits into from Jun 9, 2019

Conversation

eviefp
Copy link
Contributor

@eviefp eviefp commented Jun 4, 2019

Reviewer checklist
  • Test coverage: stack test --coverage
  • Public API documentation: stack haddock
  • Style conformance: stylish-haskell

eviefp and others added 7 commits June 4, 2019 18:11
These instances were Technically Correct, but didn't do what one would
like. `mapListT` should replace most uses of `hoist`. No replacement is offered
for `embed`.
@ttuegel
Copy link
Contributor

ttuegel commented Jun 6, 2019

@Vladciobanu I fixed the SMT problems I was having, but I seem to have introduced more failures in unification.

@@ -231,13 +229,8 @@ test_onePathStrategy =
)
, substitution = mempty
}
, Bottom
Copy link
Contributor

Choose a reason for hiding this comment

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

@virgil-serbanuta Is it OK if this test no longer explicitly outputs Bottom? I don't actually know what it represents in this case, but it's in a disjunction anway.

Copy link
Contributor

Choose a reason for hiding this comment

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

In general, for these tests, Bottom means either a branch that we managed to prove, or a branch that ended up as bottom, but we couldn't figure that out early enough (e.g. we applied an axiom, before simplification we didn't know that the result was bottom, but simplification made that clear).

Based on the comments at the top, my guess is that this was the second kind of bottom, perhaps because we applied first the coinductive rule constr11(a) => g(a), then we applied constr11(a) => g(a) to the remainder, without being able to tell that the predicate will become bottom.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the second explanation makes sense, too. The change that precipitated this change is that we are now using a real simplifier here instead of a mock simplifier for predicates, because the mock simplifier was misbehaving. I switched to using a real simplifier instead of trying to debug the mock; this was the only test output which was affected. I think I know what's happening with the mock simplifier now, so if this test change isn't acceptable I can take another look.

Copy link
Contributor

Choose a reason for hiding this comment

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

My comment is too late, but: this change is fine.

@ttuegel
Copy link
Contributor

ttuegel commented Jun 9, 2019

@Vladciobanu I'm going to go ahead and merge this to prevent more conflicts. I think this is already significant progress in the planned refactoring, and it's self-contained (mainly, it just adds MonadSimplify). I think the remaining tasks are:

  • Factor SMTT out of SMT
  • Remove the MonadIO instance of SMT
  • Refactor ReplM onto SMTT IO instead of Simplifier.

@ttuegel ttuegel marked this pull request as ready for review June 9, 2019 14:50
@ttuegel ttuegel changed the title [WIP] Refactor MonadIO out of SMT/Simplifier Introduce class MonadSimplify Jun 9, 2019
@ttuegel ttuegel merged commit 0bc188e into runtimeverification:master Jun 9, 2019
@ttuegel ttuegel added this to Done in Iteration Jun 11, 2019
@eviefp eviefp deleted the refactor-monadio branch September 12, 2019 21:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Iteration
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants