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

Haskell symbolic update rule #1662

Closed
wants to merge 15 commits into from
Closed

Haskell symbolic update rule #1662

wants to merge 15 commits into from

Conversation

andreiburdusa
Copy link
Contributor

@@ -410,6 +410,9 @@ module MAP-KORE-SYMBOLIC [kore,symbolic]

rule #Ceil(@M:Map [@K:KItem]) => {(@K in_keys(@M)) #Equals true} #And #Ceil(@M) #And #Ceil(@K) [anywhere, simplification]

rule (K |-> _ M:Map) [ K <- V ] => (K |-> V M:Map) [simplification]
rule (M:Map) [ K <- V ] => (K |-> V M:Map) [simplification]
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
rule (M:Map) [ K <- V ] => (K |-> V M:Map) [simplification]
rule (M:Map) [ K <- V ] => (K |-> V M:Map) requires notBool( K in_keys M ) [simplification]

@ttuegel ttuegel self-requested a review November 20, 2020 15:09
<k>
assignment ( (MAP:Map X:MyId |-> 1) [ Y:MyId <- 2 ] [ Z:MyId <- 3 ] ) =>
assignmentResult ( MAP Z |-> 3 )
</k> requires Y ==K X andBool Z ==K Y andBool Z ==K X
Copy link
Contributor

Choose a reason for hiding this comment

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

This is redundant. Did gen-tests.sh generate the spec this way?

Suggested change
</k> requires Y ==K X andBool Z ==K Y andBool Z ==K X
</k> requires Y ==K X andBool Z ==K Y

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ttuegel Yes. https://github.com/kframework/k/blob/master/k-distribution/tests/regression-new/map-symbolic-tests/assignment-17-spec.k#L9
The only thing I did with this tests was taking the part from the .out file and put it as the rhs.

assignment ( (MAP:Map X:MyId |-> 1) [ Y:MyId <- 2 ] [ Z:MyId <- 3 ] ) =>
assignmentResult ( MAP Z |-> 3 )
</k>
requires Y ==K X andBool Z ==K Y andBool Z ==K X andBool notBool X in_keys(MAP)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is redundant; it is implied by the left-hand side of the claim:

Suggested change
requires Y ==K X andBool Z ==K Y andBool Z ==K X andBool notBool X in_keys(MAP)
requires Y ==K X andBool Z ==K Y andBool Z ==K X


claim
<k>
assignment ( (MAP:Map X:MyId |-> 1) [ Y:MyId <- 2 ] [ Z:MyId <- 3 ] ) =>
Copy link
Contributor

Choose a reason for hiding this comment

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

This claim doesn't test anything.

Copy link
Member

Choose a reason for hiding this comment

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

To provide some context, these tests were written how they were (with invalid RHSs), so that we would get the actual output of the backend (rather than having the backend try to prove that the RHS actually is semantically equal to the resulting final state). That way we could make sure that both (i) the simplifications we want were happenning, and (ii) the simplifications we don't want were not happenning.

This was to not trust the java backend map equality checks basically, but I think it makes sense to update the tests for the haskell backend because we trust it's routines more.

module ASSIGNMENT-21-SPEC
imports MAP-TESTS

claim
Copy link
Contributor

Choose a reason for hiding this comment

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

This claim doesn't test anything.

module ASSIGNMENT-22-SPEC
imports MAP-TESTS

claim
Copy link
Contributor

Choose a reason for hiding this comment

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

This claim doesn't test anything.

module ASSIGNMENT-24-SPEC
imports MAP-TESTS

claim
Copy link
Contributor

Choose a reason for hiding this comment

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

This claim doesn't test anything.

module ASSIGNMENT-25-SPEC
imports MAP-TESTS

claim
Copy link
Contributor

Choose a reason for hiding this comment

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

This claim doesn't test anything.

@ttuegel
Copy link
Contributor

ttuegel commented Nov 23, 2020

@andreiburdusa I pushed some changes to this branch. I think you are right that the output from proving each spec should be #Top, but I also think we should not edit the specs by hand. I updated the file that the assignment specs are generated from. Could you please update the files for lookup, inkeys, and remove in the same way, and generate the tests?

@andreiburdusa
Copy link
Contributor Author

I didn't add remove-8 and remove-12 from map-symbolic-tests (java) because they gave the error
Found claim with universally-quantified variables appearing only on the right-hand side

@ttuegel
Copy link
Contributor

ttuegel commented Nov 24, 2020

@andreiburdusa We should update those tests to use existentially-quantified variables: ?X.

Copy link
Contributor

@ttuegel ttuegel left a comment

Choose a reason for hiding this comment

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

This looks OK to me. @ana-pantilie please merge this into your pull request and proceed with fixing the right-hand sides of claims, as we discussed.

Comment on lines +429 to +430
//Symbolic in_keys
rule K in_keys(M [ K <- undef ]) => false [simplification]
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
//Symbolic in_keys
rule K in_keys(M [ K <- undef ]) => false [simplification]
// Symbolic in_keys
rule K in_keys(M [ K <- undef ]) => false [simplification]

@ttuegel
Copy link
Contributor

ttuegel commented Jan 4, 2021

This was merged into #1661.

@ttuegel ttuegel closed this Jan 4, 2021
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.

MAP.update should handle symbolic keys
3 participants