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

lang: add the readFile built-in #107

Merged
merged 6 commits into from
Jan 15, 2025

Conversation

zerbina
Copy link
Collaborator

@zerbina zerbina commented Jan 12, 2025

Summary

Add the built-in readFile procedure to the source language, for
reading characters/bytes from arbitrary files.

Details

  • add the semantics for readFile. The content of external files is
    unknown, and thus readFile has an indeterminate result
  • add the internal core API readFile and fileSize procedure and
    their implementations as VM host procedures
  • implement the source language readFile as a wrapper around the core
    API fileSize and readFile procedures

In order to keep language semantics and implementation easier, at least
for the moment, attempting to read from a file that doesn't exist or
that is inaccessible for other reasons results in readFile returning
an empty sequence, rather than aborting the program.


Notes For Reviewers

readFile will most likely not stay as a built-in procedure (whose semantics are part of the language definition), but treating it as one is easier at the moment.

@zerbina zerbina added the enhancement New feature or request label Jan 12, 2025
@zerbina zerbina requested a review from saem January 12, 2025 23:06
@zerbina zerbina mentioned this pull request Jan 12, 2025
20 tasks
Copy link
Contributor

@saem saem left a comment

Choose a reason for hiding this comment

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

Left a comment about making the api a bit more robust, but I'm wondering if that's the right move given that the current incarnation is temporary. If you don't think it's worth it feel free to ignore.

@@ -409,6 +410,10 @@ C |- e_1 : All[typ] typ = (SeqTy char)
--------------------------------------- # S-builtin-writeErr
C |- (Call writeErr e_1) : unit

C |- e_1 : All[typ] typ = (SeqTy char)
--------------------------------------- # S-builtin-readFile
C |- (Call readFile e_1) : (SeqTy char)
Copy link
Contributor

Choose a reason for hiding this comment

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

would it be too fussy to go down the go route and have it return a tuple (error + seq)?

Copy link
Collaborator Author

@zerbina zerbina Jan 14, 2025

Choose a reason for hiding this comment

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

I think so, yea. readFile always returning a seq without the ability to reliably detect errors is of course nonsensical in the long run, but having it return a tuple (or union) is tricky.

The first problem is that core.fileSize and core.readFile can fail independently from each other, due to the current inability to lock the accessed file. Reporting an error from the core procedures would require an additional var bool parameter (of sorts) for both, at which point it'd make more sense to use an open/close internal API already.

The second problem, regardless of whether a var bool parameter or open/close API is used, is that both approaches would complicate the implementation of the source language readFile in source2il. The implementation is - in my opinion - quite hard to understand already, and more If guards would only exacerbate this issue. While source2il is going to be thrown out and redone at some point, in my opinion, complexity (and code size) should not grow too much out of hand.

I chose the "return an empty seq when error" solution as a middle ground, where the implementation is simple enough and robust (as in, behaviour is well-defined for all cases), without readFile being completely unusable for the user.

@zerbina zerbina merged commit 0623328 into nim-works:main Jan 15, 2025
5 checks passed
@zerbina zerbina deleted the source-lang-read-file branch January 15, 2025 00:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants