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

Add BoundedInts to DafnyStdLibs #4713

Merged
merged 17 commits into from
Nov 3, 2023

Conversation

keyboardDrummer
Copy link
Member

@keyboardDrummer keyboardDrummer commented Oct 25, 2023

Description

  1. Add BoundedInts.dfy, BoundedInts.md and BoundedIntExamples.dfy from https://github.com/dafny-lang/libraries to https://github.com/dafny-lang/dafny/tree/master/Source/DafnyStandardLibraries
  2. For Go, allows customizing the dummy value that is used for an {:extern} module, using a new undocumented, only internally used, {:member <name>, <isType>} attribute. This is necessary to let the BoundedIntExamples.dfy pass, because FunctionExamples.dfy, which is part of the same project, does not use the {:extern} module, so Go gives an unused import error. In the future I expect that we change our Go compiler so it only emits imports that are used, which makes the {:member} attribute obsolete and allows us to remove support for it from our codebase without breaking anyone.

How has this been tested?

  • The examples file serves as a test. It contains an extern method that demonstrates which native types the Dafny compiler picks when using int32 in Dafny.

By submitting this pull request, I confirm that my contribution is made under the terms of the MIT license.

@keyboardDrummer keyboardDrummer enabled auto-merge (squash) October 26, 2023 12:36
Copy link
Member

@robin-aws robin-aws left a comment

Choose a reason for hiding this comment

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

Looks great and I like the testing approach! Just a couple of thoughts for later and a question.

@@ -1,172 +1,168 @@
module Demo {
import opened DafnyStdLibs.Wrappers
Copy link
Member

Choose a reason for hiding this comment

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

Good change, but wow did GitHub ever mess up the diff something terrible. :)


module {:extern} Externs {
import opened DafnyStdLibs.BoundedInts
class {:extern} NonDefault {
Copy link
Member

Choose a reason for hiding this comment

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

Why "NonDefault"?

Copy link
Member Author

Choose a reason for hiding this comment

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

My impression is that for Java an extern should not be in the default class since there are no partial classes.

Copy link
Member

Choose a reason for hiding this comment

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

Oh Java does support mixed Dafny and target classes: https://github.com/dafny-lang/dafny/blob/master/Source/IntegrationTests/TestFiles/LitTests/LitTest/comp/Mixed.java And that works fine with default classes, it just looks weird because of the __default name.

This way is just fine though, I just didn't get why the class was named what it was.

Comment on lines +26 to +30
$(DAFNY) test -t:cs build/stdlibexamples.doo examples/BoundedInts/NonDefault.cs --output:build/stdlibexamples
$(DAFNY) test -t:java build/stdlibexamples.doo examples/BoundedInts/Externs/NonDefault.java --output:build/stdlibexamples
$(DAFNY) test -t:go build/stdlibexamples.doo examples/BoundedInts/NonDefault.go --output:build/stdlibexamples
$(DAFNY) test -t:py build/stdlibexamples.doo examples/BoundedInts/NonDefault.py --output:build/stdlibexamples
$(DAFNY) test -t:js build/stdlibexamples.doo examples/BoundedInts/NonDefault.js --output:build/stdlibexamples
Copy link
Member

Choose a reason for hiding this comment

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

Fine for this change but probably worth replacing with a find of all files of the right extension in the next one, and before too long with project file support for this. :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Or look into target specific options in the project file

@robin-aws
Copy link
Member

Almost forgot, make sure you update https://github.com/dafny-lang/dafny/tree/master/Source/DafnyStandardLibraries#available-libraries too!

Might be worth adding a tiny Functions.md for that too.

@robin-aws robin-aws mentioned this pull request Oct 27, 2023
Copy link
Member

@robin-aws robin-aws left a comment

Choose a reason for hiding this comment

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

Just a few more nits, nothing hard-blocking. Happy with the {:dummyImportMember} name. Do update the PR description so the commit message is accurate. :)


module {:extern} Externs {
import opened DafnyStdLibs.BoundedInts
class {:extern} NonDefault {
Copy link
Member

Choose a reason for hiding this comment

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

Oh Java does support mixed Dafny and target classes: https://github.com/dafny-lang/dafny/blob/master/Source/IntegrationTests/TestFiles/LitTests/LitTest/comp/Mixed.java And that works fine with default classes, it just looks weird because of the __default name.

This way is just fine though, I just didn't get why the class was named what it was.

@@ -2,7 +2,7 @@
// RUN: %exits-with 3 %dafny /compile:3 /unicodeChar:0 /spillTargetCode:2 "%s" /compileTarget:go 2> "%t"
// note: putting /compileTarget:go after "%s" overrides user-provided option
// RUN: %OutputCheck --file-to-check "%t" "%s"
// CHECK: GoModuleConversions.go:10:3: "net/url" imported and not used
// CHECK: undefined: GoModuleConversions.ParseURL
Copy link
Member

Choose a reason for hiding this comment

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

Not clear on why this is the error we're getting now, but not a blocker since this is just a negative test.

} else {
isType = true;
if (import.ExternModule != null) {
var attributes = Attributes.Find(import.ExternModule.Attributes, "dummyImportMember");
Copy link
Member

Choose a reason for hiding this comment

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

Definitely glad we're not documenting this officially, but it might be worth adding a note on this attribute to https://github.com/dafny-lang/dafny/blob/master/Source/DafnyStandardLibraries/CONTRIBUTING.md, just so anyone that might need to use it is at least aware of how it works.

@@ -167,12 +167,14 @@ public abstract class SinglePassCompiler {
/// call to the instance Main method in the enclosing class.
/// </summary>
protected abstract ConcreteSyntaxTree CreateStaticMain(IClassWriter wr, string argsParameterName);
protected abstract ConcreteSyntaxTree CreateModule(string moduleName, bool isDefault, bool isExtern, string/*?*/ libraryName, ConcreteSyntaxTree wr);
protected abstract ConcreteSyntaxTree CreateModule(string moduleName, bool isDefault, ModuleDefinition externModule,
Copy link
Member

Choose a reason for hiding this comment

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

Definitely feels a little weird to have a parameter that is "the module being created, but only passed if it's an extern". Consider keeping isExtern and theModule as separate parameters.

Copy link
Member

Choose a reason for hiding this comment

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

Just for my own understanding, are the AST changes unrelated cleanup or necessary somehow?

Copy link
Member Author

Choose a reason for hiding this comment

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

Unrelated, yes

@keyboardDrummer keyboardDrummer merged commit d4109e0 into dafny-lang:master Nov 3, 2023
20 checks passed
@keyboardDrummer keyboardDrummer deleted the boundedInts branch November 6, 2023 09:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants