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

Java type generics #4832

Merged
merged 27 commits into from
Nov 30, 2020
Merged

Java type generics #4832

merged 27 commits into from
Nov 30, 2020

Conversation

addisoncrump
Copy link
Contributor

@addisoncrump addisoncrump commented Nov 28, 2020

Using type generics like this makes it more feasible to implement operations in Java with fewer casts and much easier to use with the var keyword. There are a few breaking changes that I'd like to review with a contributor and a few questions about typing (as I am not familiar with some of the features of z3, e.g. enum types vs finite domains, Optimize, Fixedpoint, etc). Gonna go through and add comments on the PR for each of these.

@ghost
Copy link

ghost commented Nov 28, 2020

CLA assistant check
All CLA requirements met.

}


/**
* Create a new array sort.
**/
public ArraySort mkArraySort(Sort[] domains, Sort range)
public <R extends Sort> ArraySort<?, R> mkArraySort(Sort[] domains, R range)
Copy link
Contributor Author

@addisoncrump addisoncrump Nov 28, 2020

Choose a reason for hiding this comment

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

No real way to extract the domain sort here. Java type generics aren't quite as powerful as C++ templates and there's no tuple typing.

Native.mkBound(nCtx(), index, ty.getNativeObject()));
}

/**
* Create a quantifier pattern.
**/
public Pattern mkPattern(Expr... terms)
public Pattern mkPattern(Expr<?>... terms)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there anything we can say consistently about the types of the terms in a pattern? Not sure how this is restricted in z3.

@@ -75,7 +76,7 @@ public void Add(BoolExpr ... constraints)
/**
* Handle to objectives returned by objective functions.
**/
public static class Handle {
public static class Handle<R extends Sort> {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can this be more narrowly defined to R extends ArithSort?

@@ -110,7 +111,7 @@ public Expr getUpper()
* and otherwise is represented by the expression {@code value + eps * EPSILON},
* where {@code EPSILON} is an arbitrarily small real number.
*/
public Expr[] getUpperAsVector()
public Expr<?>[] getUpperAsVector()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can this be typed to R instead of ??

@@ -150,19 +150,19 @@ public String toString()
* Return an objective which associates with the group of constraints.
*
**/
public Handle AssertSoft(BoolExpr constraint, int weight, String group)
public Handle<?> AssertSoft(BoolExpr constraint, int weight, String group)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Requires explicit casting by user because there's no input to base the result type with; I think this is a reasonable requirement on the user. Won't break existing code.

Copy link
Contributor Author

@addisoncrump addisoncrump left a comment

Choose a reason for hiding this comment

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

Because Expr<FPSort> and Expr<RealSort> have the same parameters after type erasure, it's not possible to have them permissively typed. There might be a workaround with a rename (e.g. mkFPtoFP_FP, mkFPtoFP_Real), but this is kind of gross and in most cases it's likely acceptable for it to just be cast by the user.

Copy link
Contributor Author

@addisoncrump addisoncrump left a comment

Choose a reason for hiding this comment

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

Like the previous issue between Expr<FPSort> and Expr<RealSort>, these two conflict after type erasure. Same notes as above.

@addisoncrump addisoncrump marked this pull request as ready for review November 29, 2020 00:17
@addisoncrump
Copy link
Contributor Author

I think this is just about ready. Only breaking changes are where things are explicitly typed by a cast where they should not be. Things are made more permissive for different types of Expr and there's forced consistency between various operations, e.g. mkAdd will no longer accept Exprs of two different subtypes without cast (though this would only be breaking if someone was already using var keywords).

I'll add an example of why this makes life so much better for all of us later this evening, gotta get food.

@addisoncrump
Copy link
Contributor Author

You can diff JavaExample.java and Java13Example.java to see how auto-typing + functional with streams makes things a lot easier to use.

@@ -1649,18 +1472,17 @@ public void treeExample(Context ctx) throws TestFailedException
// / forest ::= nil | cons(tree, forest)
// / tree ::= nil | cons(forest, forest)
// / </remarks>
public void forestExample(Context ctx) throws TestFailedException
@SuppressWarnings({"unchecked", "unused", "UnusedAssignment"})
public <Tree, Forest> void forestExample(Context ctx) throws TestFailedException
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I especially encourage you to look at this to get a better understanding of why these changes are such a quality of life improvement.

@addisoncrump
Copy link
Contributor Author

Looks like the pipelines are failing because it's attempting to compile Java13Example with Java 8.

The code that modifies z3 proper is all Java 8 compatible, but the example code is theoretically 11+ (though I've only tested it with 13). The script or the build environment could be modified, but I think that's a little beyond my decision.

@NikolajBjorner
Copy link
Contributor

These updates look great.
There was a decision to have sorts of expressions reflected in sub-types. This is both manifested in Java and .Net.
It forces writing application code that is statically type correct, but then ends up requiring a bunch of coercions.
It is my understanding that this update retains the subtyping, but avoids some of the end-user hassle.

It would also break other client code that would have to be ported to handle this new discipline.
Given that it is a breaking change, I will wait a little week before merging in case someone using the Java bindings have substantial issues.

The Java13 example could be excluded from the build or the pipelines would somehow have to be configured to use the newer, non-default, version of Java. I am not sure how exactly to configure the pipelines with a non-default version of Java so I suggest to omit the Java13 example in the first round.

@addisoncrump
Copy link
Contributor Author

As far as I know, there are technically no breaking changes to already compiled code, as these casts are removed during type erasure, I did not modify the underlying un-generic types, and generically typed return values are strictly subtypes of what was originally returned. As for the code which will be compiled after this change, the only breaking changes are to when sorts are explicitly declared as wider than what they are, e.g. Sort s = ctx.getIntSort(), which are then used to type the return values of a mkConst or similar. Technically I think I can work around these too and make it have zero breaking changes (!), so let me see what I can do.

I'll just /* */ out Java13Example.java.

@NikolajBjorner
Copy link
Contributor

NikolajBjorner commented Nov 30, 2020

Technically I think I can work around these too and make it have zero breaking changes (!), so let me see what I can do.

I don't think it is worth it. Clients should either use "var" or the more specific types. I don't see a reason to support something that is a step back.

@addisoncrump
Copy link
Contributor Author

It boils down to whether you prefer completeness or correctness:

Sort s = ctx.getIntSort();
IntExpr e = (IntExpr) ctx.mkConst("a", s);

This fails to compile, even though it's sound. On the other hand, this also becomes permissible:

Sort s = ctx.getIntSort();
BitVecExpr e = (BitVecExpr) ctx.mkConst("a", s);

Which would compile with the change, but it would be incorrect. Note that, were s specifically typed to IntSort, this would not compile.

Which case would be more apt? With the change, you sacrifice some correctness, but it's compatible with older versions as unchecked type generic casting.

@addisoncrump
Copy link
Contributor Author

Actually, after testing this, that change makes more problems than it solves. Disregard.

@addisoncrump
Copy link
Contributor Author

Rather than outright deleting Java13Example, I replaced it with JavaGenericExample which is Java 8 compatible (used some scripting to undo var, then removed some API features introduced in Java 11). Let's see what happens in CI and hopefully that's that.

@NikolajBjorner
Copy link
Contributor

It boils down to whether you prefer completeness or correctness:

I don't see any need to change your pull request: the changes required in JavaExample.java are benign and I consider them warranted. I think other clients will be well served with making similar changes.

@addisoncrump
Copy link
Contributor Author

Okay, that's everything. Let me know if you find anything that doesn't make perfect sense.

@NikolajBjorner NikolajBjorner merged commit 3bca1fb into Z3Prover:master Nov 30, 2020
@NikolajBjorner
Copy link
Contributor

Super!

@addisoncrump addisoncrump deleted the type-generics branch November 30, 2020 19:21
shonfeder pushed a commit to apalache-mc/apalache that referenced this pull request Jul 5, 2021
shonfeder pushed a commit to apalache-mc/apalache that referenced this pull request Jul 7, 2021
…ub.tudo-aqua-z3-turnkey-4.8.10

- Bump z3-turnkey from 4.8.9 to 4.8.10
- Adapt types as needed to accommodate Z3Prover/z3#4832 (upcasting some types, and supplying some needed type params).
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