Skip to content

Fix Ac Unification error and Add new integration test #2493

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

Merged
merged 12 commits into from
Mar 26, 2021
Merged
72 changes: 36 additions & 36 deletions kore/src/Kore/Builtin/AssociativeCommutative.hs
Original file line number Diff line number Diff line change
Expand Up @@ -803,25 +803,25 @@ unifyEqualsNormalizedAc
allElements1
allElements2
Nothing
([opaque], []) ->
lift $
unifyEqualsElementLists'
allElements1
allElements2
(Just opaque)
([], [opaque]) ->
lift $
unifyEqualsElementLists'
allElements2
allElements1
(Just opaque)
([ElemVar_ v1], _)
| null opaqueDifference2 ->
lift $
unifyEqualsElementLists'
allElements1
allElements2
(Just v1)
| null allElements1 ->
unifyOpaqueVariable' v1 allElements2 opaqueDifference2
(_, [ElemVar_ v2])
| null opaqueDifference1 ->
lift $
unifyEqualsElementLists'
allElements2
allElements1
(Just v2)
| null allElements2 ->
unifyOpaqueVariable' v2 allElements1 opaqueDifference1
(_, _) -> empty
_ -> empty
let (unifiedElements, unifierCondition) =
Conditional.splitTerm simpleUnifier
lift $ do
Expand Down Expand Up @@ -1204,8 +1204,8 @@ unifyEqualsElementLists ::
[ConcreteOrWithVariable normalized variable] ->
-- | Second structure elements
[ConcreteOrWithVariable normalized variable] ->
-- | Opaque part of the first structure
Maybe (TermLike variable) ->
-- | Opaque element variable of the first structure
Maybe (ElementVariable variable) ->
unifier
( Conditional
variable
Expand Down Expand Up @@ -1274,7 +1274,7 @@ unifyEqualsElementLists
unifyEqualsChildren
firstElements
secondElements
(Just opaque)
(Just opaqueElemVar)
| length firstElements > length secondElements =
-- The second structure does not include an opaque term, so all the
-- elements in the first structure must be matched by elements in the second
Expand All @@ -1301,26 +1301,26 @@ unifyEqualsElementLists
"Duplicated element in unification results"
first
second
Just remainderTerm -> case opaque of
ElemVar_ _
| TermLike.isFunctionPattern remainderTerm -> do
opaqueUnifier <- unifyEqualsChildren opaque remainderTerm
let (opaqueTerm, opaqueCondition) =
Pattern.splitTerm opaqueUnifier
result = unifier `andCondition` opaqueCondition

return (result, [opaqueTerm])
_ ->
error . show . Pretty.vsep $
[ "Unification case that should be handled somewhere else: \
\attempting normalized unification with a \
\non-element-variable opaque term or \
\non-function maps could lead to infinite loops."
, Pretty.indent 2 "first="
, Pretty.indent 4 (unparse first)
, Pretty.indent 2 "second="
, Pretty.indent 4 (unparse second)
]
Just remainderTerm
| TermLike.isFunctionPattern remainderTerm -> do
opaqueUnifier <-
unifyEqualsChildren
(mkElemVar opaqueElemVar)
remainderTerm
let (opaqueTerm, opaqueCondition) =
Pattern.splitTerm opaqueUnifier
result = unifier `andCondition` opaqueCondition
return (result, [opaqueTerm])
_ ->
error . show . Pretty.vsep $
[ "Unification case that should be handled somewhere else: \
\attempting normalized unification with \
\non-function maps could lead to infinite loops."
, Pretty.indent 2 "first="
, Pretty.indent 4 (unparse first)
, Pretty.indent 2 "second="
, Pretty.indent 4 (unparse second)
]
where
unifyWithPermutations =
unifyEqualsElementPermutations
Expand Down
7 changes: 3 additions & 4 deletions kore/src/Kore/Builtin/Map.hs
Original file line number Diff line number Diff line change
Expand Up @@ -499,10 +499,9 @@ internalize ::
TermLike variable ->
TermLike variable
internalize tools termLike
| fromMaybe False (isMapSort tools sort')
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we just removing this check because it is redundant? Or is there another reason?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the check that I asked you about the other day, the one that Evan removed in his PR that I then had to revert. You suggested I leave it in even if it doesn't solve the Issue linked to this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, that is not the check I thought you were removing. It is redundant, and I'm glad to be rid of it, but it doesn't have anything to do with this issue in particular.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, is it fine to leave it in this PR or should I open a new PR?

, -- Ac.toNormalized is greedy about 'normalizing' opaque terms, we should only
-- apply it if we know the term head is a constructor-like symbol.
App_ symbol _ <- termLike
-- Ac.toNormalized is greedy about 'normalizing' opaque terms, we should only
-- apply it if we know the term head is a constructor-like symbol.
| App_ symbol _ <- termLike
, isConstructorModulo_ symbol =
case Ac.toNormalized @NormalizedMap termLike of
Ac.Bottom -> TermLike.mkBottom sort'
Expand Down
7 changes: 3 additions & 4 deletions kore/src/Kore/Builtin/Set.hs
Original file line number Diff line number Diff line change
Expand Up @@ -501,10 +501,9 @@ internalize ::
TermLike variable ->
TermLike variable
internalize tools termLike
| fromMaybe False (isSetSort tools sort')
, -- Ac.toNormalized is greedy about 'normalizing' opaque terms, we should only
-- apply it if we know the term head is a constructor-like symbol.
App_ symbol _ <- termLike
-- Ac.toNormalized is greedy about 'normalizing' opaque terms, we should only
-- apply it if we know the term head is a constructor-like symbol.
| App_ symbol _ <- termLike
, isConstructorModulo_ symbol =
case Ac.toNormalized @NormalizedSet termLike of
Ac.Bottom -> TermLike.mkBottom sort'
Expand Down
1 change: 1 addition & 0 deletions test/issue-2378/Makefile
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
include $(CURDIR)/../include.mk
11 changes: 11 additions & 0 deletions test/issue-2378/test-spec.k
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
module VERIFICATION
imports TEST

endmodule

module TEST-SPEC
imports VERIFICATION

claim b(keys(_M:Map)) => c

endmodule
16 changes: 16 additions & 0 deletions test/issue-2378/test-spec.k.out.golden
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
#Not ( #Exists _0 . #Exists S . {
keys ( _M )
#Equals
S
SetItem ( _0 )
} )
#And
#Not ( {
.Set
#Equals
keys ( _M )
} )
#And
<k>
b ( keys ( _M ) ) ~> _DotVar1 ~> .
</k>
14 changes: 14 additions & 0 deletions test/issue-2378/test.k
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
// Copyright (c) 2020 K Team. All Rights Reserved.

module TEST
imports SET
imports MAP

syntax B ::= b(Set)
| "c"
syntax KItem ::= B

rule b(.Set) => c
rule b(SetItem(_:KItem) S:Set) => b(S)

endmodule