-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Add high level bindings for js #6048
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some small comments at a quick glance.
* Added more high level apis
Re this comment comment:
I don't think it actually does end up cumbersome, if you provide a default and encourage users to use that as the name if they only have one context. That's what I did here and it ends up working just fine, without the end user ever having to name the generic parameter explicitly (unless of course they're working with multiple contexts, but then you want to distinguish between |
Signed-off-by: Nikolaj Bjorner <[email protected]>
* Added tests to JS Api
src/api/js/src/high-level/types.ts
Outdated
Z3_tactic, | ||
} from '../low-level'; | ||
|
||
export type AnySort<Name extends string = string> = SortRef<Name> | BoolSortRef<Name>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You almost certainly don't want to default this to string
either, because it means the types will be too permissive if you ever forget the type parameter. For example, here you've dropped the type parameter and so accepts anything which satisfies ExprRef<string>
, even stuff from other contexts, as in
const one = api.createContext('one');
const two = api.createContext('two');
let sort: SortRef<'one'> = one.BoolSort();
sort.cast(two.Bool('a')); // typechecks, but shouldn't
I think it makes sense to pick a particular string to be the default name and use that as the default value for the type parameter.
(This isn't just about maintaining the library - users who are working with multiple contexts are going to run into this also. If you only have one context, having that use the default name will mean never needing to think about the type parameter; if you have multiple contexts, you really want to make sure the type system is enforcing that they can't mix, which means not having permissive defaults.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we have to do run-time Contexts checks anyways, I've decided to specifically optimize for user convenience rather than strict type correctness.
If we go for correctness, all functions written by the users would have to be generic to work correctly.
// This is nice to write
async function allSolutions(api: Z3WithContext, solver: Solver): Promise<Model[]> {
solver.push();
const results = [];
while ((await solver.check()) === sat) {
const model = solver.model();
results.push(model);
solver.add(api.Or(...model.decls().map(decl => decl.neq(model.get(decl)))));
}
solver.pop();
return results;
}
// This is harder
async function allSolutionsGeneric<ContextName extends string>(
api: Z3WithContext<ContextName>,
solver: Solver<ContextName>,
): Promise<Model<ContextName>[]> {
// ...
}
It gets even more unwieldy with complicated types
function createMapping(api: Z3WithContext, bools: BoolRef[], ints: ArithRef[]): AstMap<BoolRef, ArithRef> {
const map = new api.AstMap<BoolRef, ArithRef>();
for (const [bool, int] of zip(bools, ints)) {
map.set(bool, int);
}
return map;
}
function createMappingGeneric<ContextName extends string>(
api: Z3WithContext<ContextName>,
bools: BoolRef<ContextName>[],
ints: ArithRef<ContextName>[],
): AstMap<BoolRef<ContextName>, ArithRef<ContextName>, ContextName> {
const map = new api.AstMap<BoolRef<ContextName>, ArithRef<ContextName>>();
for (const [bool, int] of zip(bools, ints)) {
map.set(bool, int);
}
return map;
}
Having a default name for context will not alleviate the problem either.
export interface Solver<Name extends string = 'main'> {
/* ... */
}
declare function allSolutions(api: Z3WithContext, solver: Solver): Promise<Model[]>;
// This will now fail to compile, you have to make all of your functions generic anyways
allSolutions(api, new api.createContext('myContext').Solver())
Also should we have a default name, the same omission problem you've noticed could occur as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we go for correctness, all functions written by the users would have to be generic to work correctly.
Only if working with multiple contexts. If you're only dealing with a single context, and you name it 'main'
, then you can just write Solver
and it'll work. And programs dealing with multiple contexts are precisely the ones which benefit from having accurate type checks, so only the programs which benefit will have to pay that cost.
Having a default name for context will not alleviate the problem either.
It will alleviate it as long as users who only need a single context are encouraged to use the default name as the name of their context, right?
Also should we have a default name, the same omission problem you've noticed could occur as well.
I'm not following, say more?
let defApis = Object.create(null); | ||
let functions = []; | ||
let enums = Object.create(null); | ||
// these are function types I can't be bothered to parse |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this comment predates the macro Z3_DECLARE_CLOSURE.
You can use Z3_DECLARE_CLOSURE to initialize this table.
@ritave I have a few more comments on the design, but if you're still iterating I'm happy to wait until things settle down, if you'd prefer. |
@bakkot Now's a good time for feedback. The design is settling and I'll be adding BitVectors next |
rmf('a.exe') | ||
else: | ||
rmf('a.out') | ||
# Windows |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these changes are fine, but could you add them in a self-contained pull request?
They seem to work regardless of the new work on js/ts.
It is better to get such independent pieces in earlier instead of in a single bulk.
@@ -0,0 +1,383 @@ | |||
import assert from 'assert'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW: We are in the process of creating a supported version of the z3 guide that uses the SMTLIB front-end. It could also mix in js code, not previously possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting, do you mean a guide with interactive examples?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
right.
Several years ago there were two guides exposed: one using SMTLIB2 formulas written into a text box. The other wrapped around Python. They used different backends and required hosting. This had to be discontinued.
With wasm bindings there need not be two different tutorials. The self-contained web pages can host examples in text and examples in code.
Z3_tactic, | ||
} from '../low-level'; | ||
|
||
export type AnySort<Name extends string = any> = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still feel quite strongly that the only appropriate default for these type parameters is 'main'
, or some other constant string. If you disagree, I'd like to talk about it rather than just continuing on as-is.
As is, the following typechecks, and it really shouldn't:
function divMod(a: ArithRef, b: ArithRef) {
return [a.div(b), a.mod(b)];
}
let [div, mod] = divMod((new api.Context('one').Int('a')), new api.Context('two').Int('b'));
I am happy to send a PR or push a commit which fixes this if you'd like.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I played around with some examples and I also strongly feel having a default context name is confusing for end users. For this PR I'll leave it as is.
My goal is to have partial API merged after finishing BitVec
and adding documentation, after that you could make a new PR if you want and I'll help you out with making the best experience for end users.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I played around with some examples and I also strongly feel having a default context name is confusing for end users.
Can you say more about why? And do you think there's a case where it's confusing even if you're only ever using one context, or is the confusion something which only arises when using multiple contexts?
}); | ||
|
||
it('proves x = y implies g(x) = g(y)', async () => { | ||
const { Solver, Int, IntSort, Function, Implies, Not } = new api.Context('main'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's very strange for Context
to be a class here; createContext
made more sense. In our usage a context is logically a namespace, not a class instance: it's a bag of names which you're probably going to immediately destructure, not an object you're going to pass around.
Again, happy to send a PR fixing this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The same pattern occurs in .Net APIs.
const { context } = createContext()
is not cleaner either
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
C# is a different language with different idioms than JavaScript. In JavaScript, it is very unusual to use a class for producing a bag of names. For example, new Proxy
gives you an instance, but there is a nearby API called Proxy.revokable
which returns two named things, and it is not called with new
.
const { Solver, Int, IntSort, Function, Implies, Not } = api.creatContext('main')
is definitely cleaner than using new api.Context
in that place.
If there is a reason to treat a context
object as something other than a bag of names, maybe that's a reason to make it a class. Is there such a reason?
// We want to bind functions and operations to `this` inside Context | ||
// So that the user can write things like this and have it work: | ||
// ``` | ||
// const { And, Or } = new Context('main'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you make everything closed over rather than accessing properties of this
, then you don't need to bind
anything and this code will still work. That will be a much nicer implementation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you explain more on what you mean?
The API should still work like this
const ctx = new Context('main');
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bind
is only necessary for methods which refer to this
, and there is no reason to do so for this API. (Really, there's rarely a good reason to use bind
with code you're writing - it's more for taking an existing method-based API and making it function-based. If you're designing the API, you can just make it function-based in the first place.)
That is, if instead of having
class ContextImpl {
_assertContext(...ctxs: (Context | { ctx: Context })[]) {
ctxs.forEach(other => assert('ctx' in other ? this === other.ctx : this === other, 'Context mismatch'));
}
isAst(obj: unknown): obj is Ast {
const r = obj instanceof AstImpl;
r && this._assertContext(obj);
return r;
}
}
return { Context: ContextImpl };
you had
function createContext(name) {
let ctx = new ContextImpl(name);
function _assertContext(...ctxs: (Context | { ctx: Context })[]) {
ctxs.forEach(other => assert('ctx' in other ? ctx === other.ctx : ctx === other, 'Context mismatch'));
}
return {
isAst(obj: unknown): obj is Ast {
const r = obj instanceof AstImpl;
r && _assertContext(obj); // note lack of `this`
return r;
}
};
}
return { Context: createContext };
then everything would work without bind
. And, in fact, you could still write const ctx = new Context('main')
, if you really wanted to.
@bakkot @NikolajBjorner This is as much as I could write during my evenings and weekends. I believe it would be good to merge this PR in current state, even if the high-level is not fully complete. What's included works and is somewhat documented. More PRs should be added to continue development of high-level APIs. Personally in the future I'd like to add support for Arrays when I have more time. Ready for review. |
very cool! |
@@ -47,6 +49,11 @@ def parse_options(): | |||
default=False, | |||
help='Include ML/OCaml API documentation' | |||
) | |||
parser.add_argument('--js', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
was this tested?
I am trying to reproduce it for the documentation build. See #6155.
At this point trying to produce the --js documentation on top of an emsdk build (not sure if this dependency is required).
An alternative approach may be to decentralize the documentation build and include it in the build-wasm GitHub action to produce an artifact (and wishful thinking to copy it automatically to z3prover.github.io, though I have myself been entirely unsuccessful in getting any pre-cooked action to work properly).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@NikolajBjorner I did test it beforehand, yes.
The emsdk is not required to build the documentation, and as you've noticed - it's actually a detriment due to shadowing of Node version. Running separate from emsdk build would be easiest, but if you want to do everything inside one build, the documentation should be built before emsdk activate
is called. Same way the Typescript is built before emsdk in the Github Action
@bakkot has written a correct solution to the second problem in #6155 - currently there is an unwritten expectation that the developer has run npm install
in src/api/js
directory. npm --prefix=src/api/js ci
also works and is preferred. Another solution is to quick fail inside the mk_api_doc.py
script by checking whether src/api/js/node_modules
folder exists (it's added during npm install
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, thanks it is getting closer now.
Currently some path discrepancy: the relative path used in jest.ts doesn't work with the setup in mk_api_doc.py.
Z3_solver_to_string and related methods for displaying the state of objects seem to be cool to expose over high-level. z3/src/api/js/src/high-level/types.ts Line 383 in 59d47e3
As a JS novice plan to be hitting on the API in coming days. https://microsoft.github.io/z3guide/docs/Z3%20JavaScript/test |
( Any other things you recommend exposing? I don't know enough about the API to know which things are most helpful. |
ha! shameful, looking in the wrong file. The js playground still needs several iterations and fixes, very bleeding edge first push a few minutes ago. |
OK, I found a first epsilon 907dc2c |
Nice! Feel free to ping if you find other major deficiencies (other than the user propagators, which will require more effort to get working in the JS bindings). By the way, towards making the bindings more useful, a question for you: if a thread is killed (from the outside, not with If killing a thread is an OK thing to do, I can add a helper for cancelling long-running work. |
if killing a thread doesn't interfere with locks held by the thread :-), then it would be fine modulo memory leaks. |
user propagators aren't even done for Python. It is better to leave those aside for some time as the API matures from experiences. The main experiences now are over C++ and .Net. The python API is usable except for "fresh()" (required for quantifiers) and some newer features (decision callbacks). |
Right now the JS build is using |
single threaded is an oxymoron: for timeouts there is a threadpool that invokes a callback when the timeout expires. |
Yes, the JS build does have multiple threads in the case of timeouts, as you point out. But I was hoping that building with I guess I'll just implement it and see if it works. |
As someone embracing panic age, I can say it isn't that timer queues are any new. But they are timeless. |
There is invariably going to be "minor" deficiencies that get exposed when the API is used by a novice. const x = Z3.Int.const('x'); Overal, I would like to see if I can add a good set of examples to this page: https://microsoft.github.io/z3guide/docs/Z3%20JavaScript/more%20tests/ and then establish tutorial material that is interactive for a more polished set of pages in the next weeks.
The python (on pyodide it seems) is further away as integrating a micropip package is taking me some time to troubleshoot (a combination of not being particularly good at build systems and the situation that z3 is quite fat compared to other mixed mode packages). |
I strongly recommend using TypeScript, which will catch errors of that form at compile time. For example, the above would have failed with It's possible to do this client-side, for the demo site, although it is a little bit involved. I can put a PR together at some point if I have time. |
I'm currently looking into improving the bindings to support Arrays and Quantifier expressions and have started working on this locally. @ritave you mentioned earlier that you wished to work on this as well in the future. Have you thought about how to best implement the type system with Arrays? i.e. I imagined we would want the definition to be something like |
@walnutwaldo I haven't looked into arrays yet. Currently the whole type system is somewhat self-referencing, but if you help it by casting it into proper shape it works properly. I'd say it's worth trying to do a quick experiment if your type works. |
@ritave It seems to be working in the draft PR I opened here: #6392 So far it is just output/range type inference that is carried with the Array type. It probably wouldn't be too hard to add in domain type inference from here. Since I know you and @bakkot contributed a good amount to the original implementation of the bindings, if you could take a quick look to make sure the changes generally follow the style/practices you set out, I'd be glad to finish up the implementation of Array bindings. |
@walnutwaldo It's pretty hectic right now, the earliest I could take a look is 24th Oct |
Is anyone aware of any benefits of having both |
@walnutwaldo If I remember correctly, it's copying internal C++ hierarchy |
I've started writing high level bindings for javascript for my own needs based on z3py, but I've noticed discussion #6046.
Let's decide how to proceed, but I'm creating this draft PR for posterity sake.