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

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions Source/DafnyCore/Options/BoogieOptionBag.cs
Original file line number Diff line number Diff line change
Expand Up @@ -107,9 +107,10 @@ 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
options.DafnyVerify = shouldVerify || options.Get(DeveloperOptionBag.BoogiePrint) != null;
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.

Could you add || options.Get(DeveloperOptionBag.SplitPrint) != null ?

});
DafnyOptions.RegisterLegacyBinding(VerificationTimeLimit, (o, f) => o.TimeLimit = f);

Expand Down
2 changes: 1 addition & 1 deletion Source/DafnyCore/Options/DeveloperOptionBag.cs
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ Print passified Boogie program translated from Dafny
IsHidden = true,
ArgumentHelpName = "file",
};

// This flag is the only one that can trigger /dafnyVerify in the new CLI and I think it's sufficient
public static readonly Option<string> BoogiePrint = new("--bprint",
@"
Print Boogie program translated from Dafny
Expand Down
67 changes: 67 additions & 0 deletions Source/DafnyDriver.Test/OptionsTest.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
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?

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", "--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