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

Chore: Not generate boogie translation if not verifying #6067

Merged
merged 13 commits into from
Feb 4, 2025
14 changes: 11 additions & 3 deletions Source/DafnyCore/Options/BoogieOptionBag.cs
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,8 @@ public static class BoogieOptionBag {

public static readonly Option<bool> NoVerify = new("--no-verify", "Skip verification");

public static readonly Option<bool> DoBoogieTranslation = new("--do-boogie-translation", "Requires --no-verify. Does translation to Boogie but Boogie won't verify it.");
Copy link
Member

@keyboardDrummer keyboardDrummer Jan 23, 2025

Choose a reason for hiding this comment

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

It seems strange to have an option that can only be used if another option is also used. Why not have the first option imply the second then? Instead of --do-boogie-translation why not have --no-verify-boogie that tells Dafny to translate to Boogie but not verify it.

That way, whenever the user would type --no-verify --do-boogie-translation they only have to type --no-verify-boogie.

Secondly, I would make this option hidden since it only seems useful for the purpose of debugging the Dafny verification pipeline.

Thirdly, why are you adding this option? Is someone missing this? An alternative would be to let --no-verify --bprint ... cause Dafny to translate to Boogie but not verify that Boogie, and --no-verify to not translate to Boogie at all.

Copy link
Member Author

Choose a reason for hiding this comment

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

Let's do the simple way. Since we already have --bprint, we can use it as the flag to do boogie translation, period. I like that idea.

Copy link
Member

Choose a reason for hiding this comment

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

Same for --sprint

Copy link
Member Author

Choose a reason for hiding this comment

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

And --pprint as well. Added


public static readonly Option<bool> HiddenNoVerify = new("--hidden-no-verify",
"Allows building unverified libraries without recording that they were not verified.") {
IsHidden = true
Expand Down Expand Up @@ -107,9 +109,14 @@ static BoogieOptionBag() {
});
DafnyOptions.RegisterLegacyBinding(Cores,
(o, f) => o.VcsCores = f == 0 ? (1 + System.Environment.ProcessorCount) / 2 : (int)f);
DafnyOptions.RegisterLegacyBinding(NoVerify, (options, value) => {
var shouldVerify = !value && !options.Get(HiddenNoVerify);
options.Verify = shouldVerify;
DafnyOptions.RegisterLegacyBinding(NoVerify, (options, dotNotVerify) => {
var shouldVerify = !dotNotVerify && !options.Get(HiddenNoVerify);
options.Verify = shouldVerify; // Boogie won't verify
});
// Previously /dafnyVerify
DafnyOptions.RegisterLegacyBinding(DoBoogieTranslation, (options, doBoogieTranslation) => {
options.DafnyVerify = (!options.Get(NoVerify) && !options.Get(HiddenNoVerify))
|| doBoogieTranslation || options.Get(DeveloperOptionBag.BoogiePrint) != null; // Dafny won't translate the code to Boogie
});
DafnyOptions.RegisterLegacyBinding(VerificationTimeLimit, (o, f) => o.TimeLimit = f);

Expand Down Expand Up @@ -144,6 +151,7 @@ static BoogieOptionBag() {

OptionRegistry.RegisterGlobalOption(BoogieArguments, OptionCompatibility.CheckOptionMatches);
OptionRegistry.RegisterGlobalOption(NoVerify, OptionCompatibility.OptionLibraryImpliesLocalError);
OptionRegistry.RegisterGlobalOption(DoBoogieTranslation, OptionCompatibility.OptionLibraryImpliesLocalError);
OptionRegistry.RegisterOption(HiddenNoVerify, OptionScope.Cli);
OptionRegistry.RegisterOption(Cores, OptionScope.Cli);
OptionRegistry.RegisterOption(VerificationTimeLimit, OptionScope.Cli);
Expand Down
1 change: 1 addition & 0 deletions Source/DafnyCore/Options/DafnyCommands.cs
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ static DafnyCommands() {

public static readonly IReadOnlyList<Option> TranslationOptions = new Option[] {
BoogieOptionBag.NoVerify,
BoogieOptionBag.DoBoogieTranslation,
BoogieOptionBag.HiddenNoVerify,
CommonOptionBag.OptimizeErasableDatatypeWrapper,
CommonOptionBag.TestAssumptions,
Expand Down
72 changes: 72 additions & 0 deletions Source/DafnyDriver.Test/OptionsTest.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,72 @@
using System.CommandLine;
using System.CommandLine.Builder;
using System.CommandLine.Invocation;
using System.CommandLine.Parsing;
using System.Diagnostics;
using System.IO.Pipelines;
using System.Reactive.Concurrency;
using System.Reflection;
using System.Text;
using Microsoft.Dafny;
using Microsoft.Extensions.Logging.Abstractions;
using OmniSharp.Extensions.JsonRpc;
using OmniSharp.Extensions.JsonRpc.Client;
using OmniSharp.Extensions.JsonRpc.Serialization;
using OmniSharp.Extensions.LanguageServer.Protocol.Client.Capabilities;
using OmniSharp.Extensions.LanguageServer.Protocol.Models;
using XunitAssertMessages;
using Command = System.CommandLine.Command;

namespace DafnyDriver.Test;


public class OptionsTest {
[Fact]
public async Task RunFlagsOverride() {
await TestCliRunArgs([], options => {
Assert.True(options.DafnyVerify);
Copy link
Member

Choose a reason for hiding this comment

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

I'm not a fan of tests for values in options., since these are implementation details. In the future we could do a refactoring that removes options.DafnyVerify, and then what would we do with this test?

Copy link
Member Author

@MikaelMayer MikaelMayer Jan 23, 2025

Choose a reason for hiding this comment

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

Implementation details can still be tested by unit tests, and this is the meaning of that.
If we remove options.DafnyVerify, we would simply remove that test (and add new tests to test the new behavior).

Copy link
Member

Choose a reason for hiding this comment

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

But why test like this? You could also test without looking at internals. For example, by using the --progress Symbol flag, you will see different results when using --no-verify and when not using it, and of course you can check that --no-verify --bprint still creates a file.

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 did not know about --progress Symbol until your message, that looks like it would make a nice integration test. I can envision creating a test based on that if you prefer.
But first, what makes you uncomfortable with unit tests like this?

Copy link
Member

Choose a reason for hiding this comment

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

But first, what makes you uncomfortable with unit tests like this?

Concretely, I plan on removing DafnyOptions.DafnyVerify and this test will make that harder. More generally, for tests against implementation it's hard to say how long it takes they need to be replaced. It'd rather not create work further down the line.

Copy link
Member Author

Choose a reason for hiding this comment

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

--progress Symbol is not producing any difference between the two command lines:

> make test-dafny name="rust/datatypes.dfy" action="run -t:rs --progress Symbol --no-verify"
> make test-dafny name="rust/datatypes.dfy" action="run -t:rs --progress Symbol"

The two commands output exactly the same thing.
I see that there is a barely used option options.Get(CommonOptionBag.LogLevelOption); that I could configure so that if it's set to verbose, it will emit something when entering translation. Would that work for you?

Copy link
Member

@keyboardDrummer keyboardDrummer Jan 30, 2025

Choose a reason for hiding this comment

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

Right, --progress is not supported by the commands that have not been ported to the new front-end yet, translate/build/run, so it's ignored.

If options.Get(CommonOptionBag.LogLevelOption) works that's great too.

Assert.True(options.Verify);
return 0;
});
await TestCliRunArgs(["--no-verify"], options => {
Assert.False(options.DafnyVerify);
Assert.False(options.Verify);
return 0;
});
await TestCliRunArgs(["--no-verify", "--do-boogie-translation"], options => {
Assert.True(options.DafnyVerify);
Assert.False(options.Verify);
return 0;
});
await TestCliRunArgs(["--no-verify", "--bprint=-"], options => {
Assert.True(options.DafnyVerify);
Assert.False(options.Verify);
return 0;
});
}

private static async Task TestCliRunArgs(string[] cmdArgs, Func<DafnyOptions, int> optionsCallback) {
var fullArgs = new string[] {
"run"
};
fullArgs = fullArgs.Concat(cmdArgs).Concat(new string[] { "file.dfy" }).ToArray();
var callbackCalled = false;
var newOptionsCallback = (DafnyOptions options) => {
callbackCalled = true;
return optionsCallback(options);
};
Command cmd = RunCommand.Create(newOptionsCallback);
var rootCommand = new RootCommand("Test root command");
rootCommand.AddCommand(cmd);
var builder = new CommandLineBuilder(rootCommand).UseDefaults();
var parser = builder.Build();
TextReader inputReader = new StringReader("");
var outputWriter = new StringWriter();
var errorWriter = new StringWriter();
var console = new WritersConsole(inputReader, outputWriter, errorWriter);
var exitCode = await DafnyNewCli.ExecuteForParser(console, fullArgs, parser);
Assert.Equal<object>("", errorWriter.ToString());
Assert.Equal(0, exitCode);
Assert.True(callbackCalled);
}
}
6 changes: 4 additions & 2 deletions Source/DafnyDriver/Commands/RunCommand.cs
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ static RunCommand() {
Concat(DafnyCommands.ConsoleOutputOptions).
Concat(DafnyCommands.ResolverOptions);

public static Command Create() {
public static Command Create(Func<DafnyOptions, int> continuation = null) {
Copy link
Member

Choose a reason for hiding this comment

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

What is this for?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's to be able to create a Run command but with a continuation different than the one that launches the regular pipeline, to be able to test the parsing of options only. If you have any other way I could achieve the same result, I'd happily consider it.

var result = new Command("run", "Run the program.");
result.AddArgument(DafnyCommands.FileArgument);
result.AddArgument(UserProgramArguments);
Expand All @@ -63,7 +63,9 @@ public static Command Create() {
options.Compile = true;
options.RunAfterCompile = true;
options.ForceCompile = options.Get(BoogieOptionBag.NoVerify);

if (continuation != null) {
return continuation(options);
}
return await SynchronousCliCompilation.Run(options);
});
return result;
Expand Down
6 changes: 5 additions & 1 deletion Source/DafnyDriver/DafnyNewCli.cs
Original file line number Diff line number Diff line change
Expand Up @@ -172,6 +172,10 @@ private static void ProcessOption(InvocationContext context, Option option, Dafn
}

public static Task<int> Execute(IConsole console, IReadOnlyList<string> arguments) {
return ExecuteForParser(console, arguments, Parser);
}

public static Task<int> ExecuteForParser(IConsole console, IReadOnlyList<string> arguments, System.CommandLine.Parsing.Parser parser) {
foreach (var symbol in AllSymbols) {
if (symbol is Option option) {
if (!option.Arity.Equals(ArgumentArity.ZeroOrMore) && !option.Arity.Equals(ArgumentArity.OneOrMore)) {
Expand All @@ -180,7 +184,7 @@ public static Task<int> Execute(IConsole console, IReadOnlyList<string> argument
}
}

return Parser.InvokeAsync(arguments.ToArray(), console);
return parser.InvokeAsync(arguments.ToArray(), console);
}

private static readonly MethodInfo GetValueForOptionMethod;
Expand Down
2 changes: 1 addition & 1 deletion Source/DafnyDriver/WritersConsole.cs
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@

namespace Microsoft.Dafny;

class WritersConsole : IConsole {
public class WritersConsole : IConsole {
public TextReader InputWriter { get; }
public TextWriter ErrWriter { get; }
public TextWriter OutWriter { get; }
Expand Down
2 changes: 2 additions & 0 deletions docs/DafnyRef/UserGuide.md
Original file line number Diff line number Diff line change
Expand Up @@ -2294,6 +2294,8 @@ and what information it produces about the verification process.

* `--no-verify` - turns off verification (for translate, build, run commands)

* `--do-boogie-translation` - turns on translation to Boogie after resolving or if `--no-verify` is set. Automatically set if `--bprint` is set.
Copy link
Member

@keyboardDrummer keyboardDrummer Jan 23, 2025

Choose a reason for hiding this comment

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

Outdated, please remove


* `--verify-included-files` (was `-verifyAllModules`) - verify modules that come from include directives.

By default, Dafny only verifies files explicitly listed on the command
Expand Down
Loading