-
Notifications
You must be signed in to change notification settings - Fork 263
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: Dafny Main method to accept optional seq<string> argument #2594
Conversation
DRCok: I would not use : separated arguments because arguments often contain : |
I opened the discussion so feel free to propose arguments. I already succinctly explained my default rationale of picking an array instead of a seq, because
Arguments I can think of in favor of sequence by default are
The first one is possible. What do other people think about that? I've implemented it.
|
Usually a repeated CLI option overrules an earlier one. To me that argues (a bit) against /arg I see the main purpose of this feature being to support creating executables from Dafny that when run standalone can take arguments (like "real" programs). The use case of having arguments when run from "dafny run ..." should be supported, but is less important to me and I'm willing to keep it simple with the quoted argument list. |
We already broke this "usually" with plugins. It's possible to specify multiple plugins via repeated |
importDummyWriter.WriteLine("var _ = os.Args"); | ||
} else { | ||
importDummyWriter.WriteLine("var _ {0}.{1}", id, DummyTypeName); | ||
} |
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 looks odd — what happens if someone has a Dafny module called os
?
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.
Then there will be a compilation failure. What would be the alternative?
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.
A clean error produced by Dafny; but what is bothering me is the special case on the string os
. We could store tuples in the list of modules (maybe ("os", "Args")
and (id, DummyTypeName)
)
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.
What do you mean by "we could store tuples in the list of modules"?
One could make this error go away by suppressing these dummy calls.
But overall, I think that if this is a problem down the road, the Go compiler will have to compile a Dafny module named "os" to something else, since "os" is a base package in Go anyway.
@@ -3603,7 +3613,7 @@ private class SpecialNativeType : UserDefinedType { | |||
verb = string.Format("build -o \"{0}\"", output); | |||
} | |||
|
|||
var args = string.Format("{0} \"{1}\"", verb, targetFilename); | |||
var args = string.Format("{0} \"{1}\"", verb, targetFilename) + DafnyOptions.O.ArgsStringExtra; |
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 isn't safe; arguments shouldn't be concatenated as strings
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.
That's unfortunately the only way to launch a ProcessInfo
:
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.
Thankfully that's not right :) Here's an example of how to do it:
dafny/Source/AutoExtern.Test/Tests.cs
Lines 16 to 31 in 5058d5a
var startInfo = new ProcessStartInfo("dotnet") { | |
UseShellExecute = false, | |
WorkingDirectory = minimalDirectory | |
}; | |
startInfo.ArgumentList.Add("../AutoExtern.dll"); | |
startInfo.ArgumentList.Add("Library.csproj"); // Project file | |
startInfo.ArgumentList.Add("NS"); // Namespace | |
startInfo.ArgumentList.Add("Library.dfy.template"); // Template | |
startInfo.ArgumentList.Add("CSharpModel.dfy"); // Target for CSharpModel.dfy | |
startInfo.ArgumentList.Add("Library.dfy"); // Target file | |
startInfo.ArgumentList.Add("Library1.cs"); // Source file | |
startInfo.ArgumentList.Add("Library2.cs"); // Source file | |
var dotnet = Process.Start(startInfo); |
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.
Oh that's very interesting. I changed the existing behavior to also adapt to this.
wBody.WriteLine($"dafnyArgs.set(0, dafny.DafnySequence.asString(\"java\"));"); | ||
wBody.WriteLine($"for (int i = 0; i < args.length; i++) dafnyArgs.set(i + 1, dafny.DafnySequence.asString(args[i]));"); | ||
wBody.WriteLine($"dafny.DafnySequence<? extends dafny.DafnySequence<? extends Character>> result = dafny.DafnySequence.fromArray(type, dafnyArgs);"); | ||
wBody.WriteLine($"{DafnyHelpersClass}.withHaltHandling(() -> {{ {companion}.__Main(result); }} );"); |
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.
Please move this to DafnyRuntime
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.
Syntax looks good to me, but I would much prefer to add a function each runtime, rather than having ad-hoc code in each compiler
# Conflicts: # RELEASE_NOTES.md
…y-lang/dafny into feat-compile-arguments-main
@@ -2467,7 +2467,7 @@ protected class ClassWriter : IClassWriter { | |||
} | |||
nodeProcess.StandardInput.Write(targetProgramText); | |||
if (callToMain != null && DafnyOptions.O.RunAfterCompile) { | |||
nodeProcess.StandardInput.WriteLine("require('process').argv = [\"\", " + string.Join(",", DafnyOptions.O.MainArgs.Select(ToStringLiteral)) + "];"); | |||
nodeProcess.StandardInput.WriteLine("require('process').argv = [\"node\", " + string.Join(",", DafnyOptions.O.MainArgs.Select(ToStringLiteral)) + "];"); |
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.
Don't we need Javascript quoting here (instead of Dafny quoting?)
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.
And more importantly, can't we pass these arguments on node's command line directly?
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 is JavaScript quoting (ToStringLiteral is a compiler to JavaScript function)
More importantly, the problem is that passing anything in command line (e.g. node arg1 arg2) will attempt to load the file, so this is also why the javascript compiler's run method is pushing everything in the standard input.
# Conflicts: # Source/Dafny/DafnyOptions.cs # docs/DafnyRef/Options.txt
Fixes #1116
Ability to write in a file
program.dfy
(it works for all previous cases of main methods)and then run it, either with
or by compiling it first
Design decisions:
seq<string>
because in C++, Dafny arrays don't have a length, so we need to handle sequences, otherwise no one would be able to handle arguments in C++-- arg1 arg2
in the command line to pass two extra arguments after the option/compile:4
DafnyOptions.MainArgs
and is aList<string>
, and it has an automatic conversion to string-like arguments for processes usingDanfyOptions.ArgsStringExtra
/compile:2
By submitting this pull request, I confirm that my contribution is made under the terms of the MIT license.