-
Notifications
You must be signed in to change notification settings - Fork 266
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: add basic file I/O to runtimes #3018
Conversation
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.
Having to write something like Test/runtime/file-io/FileIO.java.dfy in order to do I/O is not a great user experience. Is it possible to have a single (target-independent) .dfy file in the library or natively part of the core language that links with whatever runtime is included for that target?
I agree that needing to include target-dependent files in order to do I/O, isn't a great user experience (even if those files are defined in the |
OK. So, it would be OK if your intent is to have the same API but different files to be included from the library, with file names per target language (e.g. FileIO-java.dfy). |
But still - it looks like java and CS# could be combined. And the module names do not have to be different. Make it as easy as possible to switch between target languages. I'd even argue for allowing extern declarations that are activated by a target language and allowing multiple such externs. |
Yes, that's mostly the approach I took here - a singular API defined as an abstract module, with concrete refining modules per target language that differ in the
That's not true because Java uses
That's a good point. I've simplified the module names so that the concrete modules can all be imported under the same name, and only the
I agree, and there is discussion about such a feature here: #2397. But we opted to keep that out of scope for this initial file I/O support, which is intentionally basic. If/when that feature is added then we can simplify these modules accordingly. |
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.
- Do you intend to swallow the exception stack?
- Please write the FileIO files precisely as they will be in the library -- currently they are test files. Even better would be to put them in a separate folder.
2a) And then you can, in each FileIO file, document a simple use of the IO methods (i.e. what file to include, what module to import) - {:compile false} is unnecessary for method stubs that do not have bodies
- Did you and Robin decide to just read/write bytes and not read/write strings? The more common use case is to get text, not bytes from files.
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.
Just some high-level feedback for now, will probably have more after we split up the test files as I've suggested (assuming that makes sense)
} | ||
|
||
method INTERNAL_ReadBytesFromFile(path: string) | ||
returns (isError: bool, bytesRead: seq<bv8>, errorMsg: string) |
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.
Do you have a strong opinion about making this isError
rather than isSuccess
? I know the idiom in Go is to return ok
, which this reminds me of.
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.
I concur with wondering about the correct sign of this output.
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.
I don't have a strong opinion either way, but since this an internal API (users of the file I/O API should never see/use this) I'm not too concerned either way.
@@ -420,3 +420,35 @@ class defaults: | |||
real = staticmethod(BigRational) | |||
pointer = staticmethod(lambda: None) | |||
tuple = staticmethod(lambda *args: lambda: tuple(a() for a in args)) | |||
|
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.
I would prefer to keep the runtime as small as possible. Can we move those into a separate file in the file-io
directory, until this becomes an actual language 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.
@alex-chew and I revisited our reasoning for putting this logic in the runtime, and came to the same conclusion. The original motivation was piggybacking on the existing "distribution" of native code, but I think it's not worth the coupling cost, especially since it forces using refinement to customize the extern declaration per language.
@alex-chew is going to move this to a pure dafny-lang/libraries
PR that includes the native code as well. The downside will be needing to manually include a target language artifact like DafnyIO.jar
to build and run Dafny code that uses this library, but that pain should go away in the long term as we add better support for Dafny projects and dependencies, especially when you add separate compilation.
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.
Your call, but for me, including the IO routines in the runtime is well worth not having to scrounge around for the relevant FileIO.xx for each target. At least have dafny run
include these extra files automatically.
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 the new PR there is no implementation in the runtime, so @fabiomadge's original concern in this thread is resolved.
Your call, but for me, including the IO routines in the runtime is well worth not having to scrounge around for the relevant FileIO.xx for each target. At least have dafny run include these extra files automatically.
In my opinion the upsides of keeping the file I/O libraries independent from a particular Dafny version range, and of keeping the runtimes small, outweigh the cost of needing to specify an extra --input libraries/src/FileIO/FileIO.xx
flag (or otherwise integrating the file into one's build process, if using separate compilation). But having dafny run
include the extra files automatically is an interesting idea and I think it can be implemented later as a non-breaking change.
As discussed in discussions here and offline, I'm moving this work out of the Dafny language/runtime and into the |
This was not intentional; in this PR exception stack traces were only preserved in C#. In the new PR, all implementations preserve stack traces.
Good call; I've done this in the new PR.
I've noted this as a TODO in the new PR.
Thanks, I've removed these in the new PR.
Yes, we wanted to keep the complexity of handling text encoding out of this MVP. I expect that we'll be able provide APIs that "do the right thing" by default once Dafny strings are Unicode by default. For now, users can encode/decode as needed using the existing Unicode modules in the libraries repo. |
This PR adds basic file I/O facilities (read bytes from a file, write bytes to a file) to the C#, Java, Javascript, and Python runtimes. These facilities are a prerequisite for implementing file I/O in the
libraries
repo, as requested here: dafny-lang/libraries#50(See
Test/runtime/file-io/README.md
for a description of the test files and the structure/purpose of the wrapping modules.)By submitting this pull request, I confirm that my contribution is made under the terms of the MIT license.