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

feat: Support for arbitrary versions of z3 #2820

Merged
merged 15 commits into from
Oct 6, 2022

Conversation

MikaelMayer
Copy link
Member

Added documentation on how to make Dafny work for other versions of Z3 Added automatic overriding of prover options if Z3 version 4.8.7 or newer is detected in the language server

I tested it manually with Z3 version 4.8.7. I verified the version of Z3 was correctly sent to VSCode using the telemetry handler, and that counter-examples were working as usual (which they weren't before if the option wasn't changed)

By submitting this pull request, I confirm that my contribution is made under the terms of the MIT license.

Added documentation on how to make Dafny work for other versions of Z3
Added automatic overriding of prover options if Z3 version 4.8.7 or newer is detected in the language server
Copy link
Member

@atomb atomb left a comment

Choose a reason for hiding this comment

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

Thanks for doing this! I have a few very small changes to request, but it overall looks great.

It'd be hard to test this without installing multiple versions of Z3 in CI, which is probably a bigger step than we want to take quite yet. We probably will do that sometime before long, though, and when we do it might be worth coming back and adding a test for this feature.

/proverOpt:O:model.completion=true.
first failing assertion. Requires specifying the /mv:<file> option as well
as /proverOpt:O:model_compress=false (or /proverOpt:O:model.compact=false if
your version of z3 is >= 4.8.7) and /proverOpt:O:model.completion=true.
Copy link
Member

Choose a reason for hiding this comment

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

We'll probably want to swap what's in parentheses once we adopt a newer version by default.

Copy link
Member

Choose a reason for hiding this comment

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

Or maybe update this text to match what's in the message in DafnyModel.cs, which is more symmetric?

Source/DafnyLanguageServer/DafnyLanguageServer.cs Outdated Show resolved Hide resolved
private static void PublishSolverPath(ILanguageServer server) {
var telemetryPublisher = server.GetRequiredService<ITelemetryPublisher>();
string solverPath;
try {
var proverOptions = new SMTLibSolverOptions(DafnyOptions.O);
proverOptions.Parse(DafnyOptions.O.ProverOptions);
solverPath = proverOptions.ExecutablePath();
HandleZ3Version(telemetryPublisher, proverOptions);
Copy link
Member

Choose a reason for hiding this comment

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

In the very long run, I think we may want to handle solvers other than Z3, and perhaps condition on their versions, too. But that's probably somewhat far in the future. :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed, and then we can rename this method. For now, it specifically do nothing if there is no "z3 version" in the output of the process.

@@ -24,11 +24,11 @@ other aspects that are dissimilar between the pdf and online versions.

Copy link
Member

Choose a reason for hiding this comment

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

Did you mean to include changes to this file as part of this PR?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh yes, that seems unrelated, but I needed this information to update this PR, so I put it in the PR itself.

RELEASE_NOTES.md Outdated
@@ -4,6 +4,7 @@
- feat: Support for parsing Basic Multilingual Plane characters from UTF-8 in code and comments (https://github.com/dafny-lang/dafny/pull/2717)
- feat: Command-line arguments are now available from `Main` in Dafny programs, using `Main(args: seq<string>)` (https://github.com/dafny-lang/dafny/pull/2594)
- feat: Generate warning when 'old' has no effect (https://github.com/dafny-lang/dafny/pull/2610)
- feat: Support arbitrary version of z3 (https://github.com/dafny-lang/dafny/pull/2820)
Copy link
Member

Choose a reason for hiding this comment

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

I wonder whether this would be better as "Support arbitrary versions of z3 when using the /proverOpt:PROVER_PATH=/path/to/local/z3 option. Dafny is still distributed with z3 4.8.5 and uses that version by default."

Copy link
Member Author

Choose a reason for hiding this comment

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

I think Dafny already supports arbitrary version of z3 with this parameter. I was surprised but there was nothing to do there. It's just that model_compress cannot be passed for versions of z3 greater than 4.8.6
What about "Support arbitrary versions of z3 for the language server" ?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I like "Support arbitrary versions of z3 for the language server", but perhaps also with the second sentence included? We could even say something about how we plan to make a newer version the default before too long.

RELEASE_NOTES.md Outdated Show resolved Hide resolved
Copy link
Member

@atomb atomb left a comment

Choose a reason for hiding this comment

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

One tiny additional change and then I think it's good to go!

Source/DafnyCore/DafnyOptions.cs Outdated Show resolved Hide resolved
atomb
atomb previously approved these changes Sep 30, 2022
Copy link
Member

@atomb atomb left a comment

Choose a reason for hiding this comment

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

Looks great!

@keyboardDrummer
Copy link
Member

@atomb Are we planning to figure out how to configure the latest Z3 version so that it performs as well for Dafny as 4.8.5, or do we think there's no such configuration?

@atomb
Copy link
Member

atomb commented Oct 4, 2022

@atomb Are we planning to figure out how to configure the latest Z3 version so that it performs as well for Dafny as 4.8.5, or do we think there's no such configuration?

We don't yet know of a configuration that performs as well (and we have tried several possibilities), but this would give us a way to enable a better configuration if we discover one.

@MikaelMayer MikaelMayer enabled auto-merge (squash) October 4, 2022 19:21
@MikaelMayer MikaelMayer requested a review from atomb October 5, 2022 19:35
@MikaelMayer MikaelMayer merged commit 5e00d8e into master Oct 6, 2022
@MikaelMayer MikaelMayer deleted the feat-support-all-z3-versions branch October 6, 2022 15:43
@MikaelMayer MikaelMayer self-assigned this Oct 6, 2022
@cpitclaudel
Copy link
Member

Looks like the RELEASE_NOTES entry ended up in the wrong place. I will fix that in #2848

@hmijail
Copy link

hmijail commented Nov 23, 2022

Regarding performance of Z3, just a comment to ensure this is in the radar: version 4.8.16 was the first one available natively to MacOS on ARM, before that it's only available for Intel so it runs emulated in those computers. I'd imagine this worsens performance but haven't compared.

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.

5 participants