-
Notifications
You must be signed in to change notification settings - Fork 261
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 replaceable modules #4681
Add replaceable modules #4681
Conversation
…into placeholderModule
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.
Loving this feature, and I'm getting pretty comfortable with the terminology/syntax, although I want to give other core maintainers the chance to object.
Just reviewed the feature design and tests/documentation for now, can circle back to review the actual implementation.
The module also defines two forms of `Need`, that check the truth of a predicate and | ||
return a failure value if false. | ||
*/ | ||
module DafnyStdLibs.Wrappers { |
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 bit silly I'd admit, but we should probably stop adding copies of Wrappers
all over the place in the integration tests since #4678 will add an official copy, and having decoys will make searching for the real one annoying. :)
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.
Totally, but your PR isn't merged yet ! I planned on removing this later.
var areEqual := x == y; | ||
print "areEqual: ", areEqual; |
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 a good case for using expect
instead of printing and comparing, I think.
Also I don't love the idea of a test that by design could non-deterministically fail, no matter how small the probability. :) How about expect 0 <= x < 1000000
instead?
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.
Also I don't love the idea of a test that by design could non-deterministically fail
I changed the chance to 10^-30, so it won't fail before our planet disintegrates
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 a good case for using expect instead of printing and comparing, I think.
Not sure what the advantage is, but sure !
...tionTests/TestFiles/LitTests/LitTest/comp/placeholders/placeholderExecutionErrors.dfy.expect
Outdated
Show resolved
Hide resolved
docs/DafnyRef/UserGuide.md
Outdated
@@ -1727,7 +1727,31 @@ Detailed description of the `dafny build` and `dafny run` commands and | |||
the `--input` option (needed when `dafny run` has more than one input file) | |||
is contained [in the section on command-line structure](#command-line). | |||
|
|||
### 13.7.2. C\# | |||
### 13.7.2. Placeholder modules | |||
To allow defining behavior that depending on which target language Dafny is translated to, requires different Dafny code to be implemented, Dafny has placeholder modules. Here follows an example: |
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.
nit: I'd like to tweak this a little to describe the abstract feature first, then the primary motivation. Something like like "To support customizing behavior differently at compile-time for different contexts, Dafny has placeholder modules... This is particularly useful for defining behavior that depends on which target language Dafny is translated to".
I really like that the feature is generic enough to apply to other use cases so let's not sell it short!
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.
In fact I'd promote at least most of this section to the reference manual since it is in fact a language feature, not just a tool feature.
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.
In fact I'd promote at least most of this section to the reference manual since it is in fact a language feature, not just a tool feature.
Not sure where to put it then, there is no "runtime behavior" section, and I think this is a pretty good spot for now.
Source/IntegrationTests/TestFiles/LitTests/LitTest/comp/placeholders/placeholderHappyflow.dfy
Outdated
Show resolved
Hide resolved
Source/IntegrationTests/TestFiles/LitTests/LitTest/comp/placeholders/complex/randomJava.dfy
Outdated
Show resolved
Hide resolved
// RUN: %baredafny run %args %s --target=java --build=%S/Build/build --input %S/wrappers.dfy --input %S/random.dfy --input %S/randomJava.dfy --input %S/Interop/__default.java --spill-target-code > "%t" | ||
// RUN: %baredafny run %args %s --build=%S/Build/build --input %S/wrappers.dfy --input %S/random.dfy --input %S/randomCSharp.dfy --input %S/Interop.cs --spill-target-code >> "%t" |
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.
You can definitely see how nice it would be to have the project file specify all this, and even better to have a default convention for the code layout of a Dafny project with extern code. :)
3bd86a8
to
4229dcd
Compare
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.
Looks great! A few tweaks but otherwise the documentation is good, the tests are good (I would like to see at least one test where the replaceable module is already 100% defined by itself, whatever semantics you think you'd choose)
...ntegrationTests/TestFiles/LitTests/LitTest/comp/replaceables/complex/Inputs/randomCSharp.dfy
Show resolved
Hide resolved
y := DfyRandom.GetRandomNat(1000000); | ||
i := i - 1; | ||
} | ||
expect x != y; |
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.
expect x != y; | |
expect i == 0 || x != y; |
I hope you don't mind too much but I'd feel better for maintenance if the expect was always true :-)
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.
but then it doesn't test the randomness, and in any practical sense it already was always true. Anyways, made the change.
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.
hmmm. Ok. In that case, can you mark Main as decreases *
and that way you can also mark the loop as decreases *
. Then you are truly testing for randomness, but now it's no longer a probability on the test failing or not, it's how long it will take.
…4790) ### Description Adds build support for standard libraries with different Dafny and target language source code per backend, and imports the `FileIO` library. The build support mainly adds a set of `DafnyStandardLibraries-<target>.doo` files, and a common `DafnyStandardLibraries-notarget.doo` file with only the abstract interface. The pattern looks a lot like [replaceable modules](#4681), but is less efficient for now because it re-verifies common code rather than relying on refinement. Please pay particular attention to the documentation updates as this does slightly affect the user experience with standard libraries! Major changes to `FileIO` on the way in: * Refactored to separate the externs into its own module, so it could be defined differently per backend without duplicating the common wrapper logic. * Added support for Go and Javascript. * Removed the comments about additional source files in the `README.md` since it's all automatic now (yay!) Other supporting changes: * Fixes #4782, as otherwise Go was messy to implement and Python nearly impossible. * Added utilities in the Go runtime for converting between `_dafny.Sequence` and `[]byte`. * Added an automatic build of Dafny itself at the end of `make update-binary`, since failing to do that and therefore not picking up the new versions of the `.doo` files was a common gotcha. * Switched CI job that tests the standard libraries from Ubuntu to Mac OS: diffing checked in `.doo` files against fresh builds of them was failing for what looks like whitespace/newline variation. It makes more sense to stick to Mac OS for verification of production Dafny code anyway, however, since most contributors are developing on macs and [verification is not guaranteed to be consistent across platforms](#1807). ### How has this been tested? `StandardLibraries_TargetSpecific.dfy` for the general support, `FileIOExamples.dfy` for `FileIO` itself (mainly the same as the original library, adapted as Dafny tests instead of lit tests) --------- Co-authored-by: Alex Chew <alex-chew@users.noreply.github.com>
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.
Good for me. Thanks.
@@ -30,10 +30,36 @@ public abstract class ExecutableBackend : IExecutableBackend { | |||
public override string ModuleSeparator => Compiler.ModuleSeparator; | |||
|
|||
public override void Compile(Program dafnyProgram, ConcreteSyntaxTree output) { | |||
InstantiatePlaceholders(dafnyProgram); |
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.
InstantiatePlaceholders(dafnyProgram); | |
InstantiateReplaceableModules(dafnyProgram); |
One name per concept makes the code easier to read
…into placeholderModule
…just already did a regular build
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.
Previously approved. Small changes since.
other reviewers got a chance to object
Description
Move some C# code from
Dafny.atg
andParser.frame
to a new file namedParserNonGeneratedPart.cs
, so C# IDEs can provide tooling for this code.Enable the following syntax
Check out one the test-cases for an example of how you define a multi-target application using this feature: https://github.com/dafny-lang/dafny/blob/66c1b1ee414bb4a5060507dbb0dd333697968e02/Source/IntegrationTests/TestFiles/LitTests/LitTest/comp/placeholders/complex/user.dfy
Caveats
Refining the
replaceable
module is not supported, and it will not be supported for some time. There's a conflict between how refinement works and how replaceable modules work.Replaceable modules are implemented as a post-resolution feature. It swaps out references to the replaceable with the replacement. It has to work like this because
dafny verify
should work without the replacement being part of the source, so refinement can not rely on the replacement module being present.Refinement, as it is currently implemented, copies non-abstract parts of the refinee into the refiner during resolution, so you would need access to the replacement module during resolution, if you want to further refine it.
We could change refinement so it does use the replacement module if its available, to influence refinement, but I consider that detrimental to maintainability so I think we should not do this. Refinement being implemented using copying is already problematic because it increases compilation time and generated code size. I think we should change refinement so it uses references to the refinee's members instead of copying them, and then we can also implement refinement of placeholders.
How has this been tested?
By submitting this pull request, I confirm that my contribution is made under the terms of the MIT license.