-
Notifications
You must be signed in to change notification settings - Fork 267
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 several new-style command-line options #3568
Changes from all commits
d8e97ee
888d60d
1283f95
409c6f8
417f5ea
05d3bf3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,11 +2,15 @@ | |
using System.CommandLine; | ||
using System.IO; | ||
using System.Linq; | ||
using Microsoft.Boogie; | ||
|
||
namespace Microsoft.Dafny; | ||
|
||
public class CommonOptionBag { | ||
|
||
public static readonly Option<int> ErrorLimit = | ||
new("--error-limit", () => 5, "Set the maximum number of errors to report (0 for unlimited)."); | ||
|
||
public static readonly Option<bool> ManualLemmaInduction = | ||
new("--manual-lemma-induction", "Turn off automatic induction for lemmas."); | ||
|
||
|
@@ -45,6 +49,21 @@ datatype with a single non-ghost constructor that has a single | |
"Allow variables to be read before they are assigned, but only if they have an auto-initializable type or if they are ghost and have a nonempty type.") { | ||
}; | ||
|
||
public static readonly Option<bool> IsolateAssertions = new("--isolate-assertions", @"Verify each assertion in isolation."); | ||
|
||
public static readonly Option<List<string>> VerificationLogFormat = new("--log-format", $@" | ||
Logs verification results using the given test result format. The currently supported formats are `trx`, `csv`, and `text`. These are: the XML-based format commonly used for test results for .NET languages, a custom CSV schema, and a textual format meant for human consumption. You can provide configuration using the same string format as when using the --logger option for dotnet test, such as: --format ""trx;LogFileName=<...>""); | ||
|
||
The `trx` and `csv` formats automatically choose an output file name by default, and print the name of this file to the console. The `text` format prints its output to the console by default, but can send output to a file given the `LogFileName` option. | ||
|
||
The `text` format also includes a more detailed breakdown of what assertions appear in each assertion batch. When combined with the {CommonOptionBag.IsolateAssertions.Name} option, it will provide approximate time and resource use costs for each assertion, allowing identification of especially expensive assertions.".TrimStart()) { | ||
ArgumentHelpName = "configuration" | ||
}; | ||
|
||
public static readonly Option<uint> SolverResourceLimit = new("--resource-limit", @"Specify the maximum resource limit (rlimit) value to pass to Z3. Multiplied by 1000 before sending to Z3."); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How is a user supposed to use this option? There's no guidance for how to pick a value. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll add more thorough help text in a later PR. |
||
public static readonly Option<string> SolverPlugin = new("--solver-plugin", @"Specify a plugin to use to solve verification conditions (instead of an external Z3 process)."); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would use the name There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The only kind of plugin Boogie supports is a solver plugin, and I'd prefer that Dafny users not need to know about the existence of Boogie. So I'd prefer to leave it with the current naming. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I think If a user doesn't know about Boogie then they have to use So, I suggest we either not add There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Has anyone ever used a solver plugin? And are there any instructions on how to do it? Although there is a SMT-lib standard, solvers differ quite a bit in ways that end up mattering that this would be a very non-trivial exercise. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm inclined on this one to not expose it in the new CLI and see if anyone complains. |
||
public static readonly Option<string> SolverLog = new("--solver-log", @"Specify a file to use to log the SMT-Lib text sent to the solver."); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Based on this comment: #3537 (comment) "The option that actually came up in our use case was /normalizeOptions:0, which makes the generated SMT files much easier to debug. Hopefully in the new CLI there remains a way to do this." It seems like just exposing the raw SMT doesn't help users much. I suggest adding There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Many Dafny users I've talked to want to be able to look at the generated SMT (or store it to send to a different solver). I wouldn't entirely be opposed to making it hidden, but I'd want to make sure that the standard help has some sort of message in the spirit of "there are other, more advanced options, too. Used --help-hidden to see them". It's used often enough that I'd be reluctant to make it hard to find. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I understand that some options are used only for development debugging of solver performance. But in general having hidden options smacks of being patriarchal "we know best what you need" to the users. Many tools have lots of options -- for myself (as a user) it is helpful to browse the list to see what is possible, but not dig into them unless I need them. I would bias towards not hiding options unless they real are deeply technical. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I don't feel that way. There are many options that are only relevant to Dafny language developers, take printing out the resolved Dafny program for example. As a user, for each option in the help that I read that isn't useful to me, I will be less excited to keep reading the help, and there's a higher chance I will miss options that are relevant.
That sounds good, although I wouldn't say 'more advanced options' but 'options meant for debugging the Dafny language toolchain'
Could this be part of a Dafny toolchain developer use-case? |
||
|
||
public static readonly Option<IList<string>> Libraries = new("--library", | ||
@" | ||
The contents of this file and any files it includes can be referenced from other files as if they were included. | ||
|
@@ -164,6 +183,7 @@ static CommonOptionBag() { | |
DafnyOptions.RegisterLegacyBinding(WarnMissingConstructorParenthesis, | ||
(options, value) => { options.DisallowConstructorCaseWithoutParentheses = value; }); | ||
DafnyOptions.RegisterLegacyBinding(WarningAsErrors, (options, value) => { options.WarningsAsErrors = value; }); | ||
DafnyOptions.RegisterLegacyBinding(ErrorLimit, (options, value) => { options.ErrorLimit = value; }); | ||
DafnyOptions.RegisterLegacyBinding(VerifyIncludedFiles, | ||
(options, value) => { options.VerifyAllModules = value; }); | ||
|
||
|
@@ -197,6 +217,18 @@ static CommonOptionBag() { | |
|
||
DafnyOptions.RegisterLegacyBinding(Verbose, (o, v) => o.CompileVerbose = v); | ||
DafnyOptions.RegisterLegacyBinding(DisableNonLinearArithmetic, (o, v) => o.DisableNLarith = v); | ||
|
||
DafnyOptions.RegisterLegacyBinding(VerificationLogFormat, (o, v) => o.VerificationLoggerConfigs = v); | ||
DafnyOptions.RegisterLegacyBinding(IsolateAssertions, (o, v) => o.VcsSplitOnEveryAssert = v); | ||
DafnyOptions.RegisterLegacyBinding(SolverResourceLimit, (o, v) => o.ResourceLimit = v); | ||
DafnyOptions.RegisterLegacyBinding(SolverPlugin, (o, v) => { | ||
if (v is not null) { | ||
o.ProverDllName = v; | ||
o.TheProverFactory = ProverFactory.Load(o.ProverDllName); | ||
} | ||
}); | ||
DafnyOptions.RegisterLegacyBinding(SolverLog, (o, v) => o.ProverLogFilePath = v); | ||
|
||
DafnyOptions.RegisterLegacyBinding(EnforceDeterminism, (options, value) => { | ||
options.ForbidNondeterminism = value; | ||
options.DefiniteAssignmentLevel = 4; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -18,21 +18,24 @@ static ICommandSpec() { | |
|
||
public static IEnumerable<Option> FormatOptions => new Option[] { | ||
CommonOptionBag.Check, | ||
CommonOptionBag.StdIn, | ||
CommonOptionBag.Verbose, | ||
CommonOptionBag.FormatPrint, | ||
DeveloperOptionBag.UseBaseFileName | ||
}.Concat(ParserOptions); | ||
|
||
public static IReadOnlyList<Option> VerificationOptions = new Option[] { | ||
CommonOptionBag.StdIn, | ||
CommonOptionBag.RelaxDefiniteAssignment, | ||
BoogieOptionBag.VerificationTimeLimit, | ||
CommonOptionBag.VerifyIncludedFiles, | ||
CommonOptionBag.ManualLemmaInduction, | ||
CommonOptionBag.SolverPath, | ||
CommonOptionBag.DisableNonLinearArithmetic, | ||
CommonOptionBag.IsolateAssertions, | ||
BoogieOptionBag.BoogieArguments, | ||
CommonOptionBag.VerificationLogFormat, | ||
CommonOptionBag.SolverResourceLimit, | ||
CommonOptionBag.SolverPlugin, | ||
CommonOptionBag.SolverLog, | ||
}.ToList(); | ||
|
||
public static IReadOnlyList<Option> TranslationOptions = new Option[] { | ||
|
@@ -59,13 +62,15 @@ static ICommandSpec() { | |
}); | ||
|
||
public static IReadOnlyList<Option> ParserOptions = new List<Option>(new Option[] { | ||
CommonOptionBag.StdIn, | ||
BoogieOptionBag.Cores, | ||
CommonOptionBag.Libraries, | ||
CommonOptionBag.Plugin, | ||
CommonOptionBag.Prelude, | ||
Function.FunctionSyntaxOption, | ||
CommonOptionBag.QuantifierSyntax, | ||
CommonOptionBag.UnicodeCharacters, | ||
CommonOptionBag.ErrorLimit, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shouldn't this be in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, you're right. I was mistakenly thinking it affected other sorts of errors, too. |
||
}); | ||
|
||
public static IReadOnlyList<Option> ResolverOptions = new List<Option>(new Option[] { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
// RUN: %exits-with 4 %baredafny verify %args --error-limit=2 "%s" > "%t" | ||
// RUN: %diff "%s.expect" "%t" | ||
method m(x: int) { | ||
if x == 1 { | ||
assert x != 1; | ||
} else if x == 2 { | ||
assert x != 2; | ||
} else if x == 3 { | ||
assert x != 3; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,4 @@ | ||
errorLimit.dfy(5,13): Error: assertion might not hold | ||
errorLimit.dfy(7,13): Error: assertion might not hold | ||
|
||
Dafny program verifier finished with 0 verified, 2 errors |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
// RUN: %baredafny verify %args --solver-log="log.smt2" --resource-limit 10 "%s" > "%t" | ||
// RUN: %diff "%s.expect" "%t" | ||
// RUN: %OutputCheck --file-to-check "log.smt2" "%s" | ||
// CHECK: rlimit 10000 | ||
method m() { | ||
assert 1 + 1 == 2; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
|
||
Dafny program verifier finished with 1 verified, 0 errors |
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.
Shouldn't this be called
verification-error-limit
, since it limits only verification errors?Also, the use-case of the option is unclear to me. If it's only used for testing then it would be better if it's a hidden option. What's the reason that the default is not unlimited? Is that to improve performance? If yes, then it would be good to explain that in the description of this option.
Right now, if I read the description of this option then I'm confused as to why the reported errors are limited to begin with.
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 also think the default should be unlimited, and that perhaps the option isn't needed at all. My goal for now, though, was to replicate existing functionality in the new CLI, rather than change it. Since the default is to limit the number of errors presented, having an option to raise or eliminate that limit seems important.
Calling it
--verification-error-limit
makes sense. I think I'm going to have another PR to add some more options and could change it then.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 can be quite important. If you encode a method as one big VC, then it reports one assertion failure. To get more assertion failures you adjust the VC to prevent the failure you have already seen and rerun. You can continue this until no more failures are found. That iteration takes time and a user might well want to stop after some small number. The problem is that the order in which results are found is non-deterministic. There are arguments for a default either way -- when I was working with production code, I would set it to a small number for a faster debug cycle. No limit only works for users if the results are presented as they are found. I vote to keep it. Like Aaron, I did not know it was only for verification errors.
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.
To be clear, I think this is what Boogie does automatically.
So it's indeed a performance related option.
Why is that a problem?
If we keep it then we should explain why we have it.
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 non-determinism causes this problem. Suppose you run with an error-limit of 1, find a problem and "fix" it. Then run again. You get a different problem. Did you fix the first one or did you randomly get a different problem?
Non-determinism is also a problem for tests.(And is why I write tests with just one problem, though that does not capture the full range of uses)
And yes, I would count it as a (user) performance optimization.
The user manual says this: limits the number of verification errors reported per procedure. Default is 5; 0 means as many as possible; a small positive number runs faster but a large positive number reports more errors per run
To me that concisely captures the value and tradeoff of the option