From 08e202e9c30025eea12dcef29e5a42e594cfa3d7 Mon Sep 17 00:00:00 2001 From: Ryan Hagenson Date: Fri, 2 Jul 2021 20:40:57 -0500 Subject: [PATCH 1/3] Add FilePath constructor split RFC --- text/0000-filepath-constructor.md | 116 ++++++++++++++++++++++++++++++ 1 file changed, 116 insertions(+) create mode 100644 text/0000-filepath-constructor.md diff --git a/text/0000-filepath-constructor.md b/text/0000-filepath-constructor.md new file mode 100644 index 00000000..08163f2c --- /dev/null +++ b/text/0000-filepath-constructor.md @@ -0,0 +1,116 @@ +- Feature Name: Separate `FilePath` constructor from `AmbientAuth` +- Start Date: 2021-07-02 +- RFC PR: +- Pony Issue: + +# Summary + +It should be without error to constructor a `FilePath` when `AmbientAuth` is provided. The current single constructor uses a union of `(FilePath | AmbientAuth)` to limit capabilities and access for the newly created `FilePath`. Insufficient capabilities or access allows the first half of this union, `FilePath`, to fail, however the latter half `AmbientAuth` can never fail. + +# Motivation + +Beyond making using `FilePath` more convenient, this change will allow use cases where a path is guaranteed such as constructing a path to the root directory. Currently, all `FilePath` must be constructed within a try-else block. Importantly, `FilePath` does not guarantee its operations will succeed, however a `FilePath` is merely a combination of some directory path and capabilities. No changes to __using__ a `FilePath` are intended by this change. + +# Detailed design + +The suggested change is to split the implementation in two around the current match statement with `create` using `AmbientAuth` and a new constructor called `from` using an existing `FilePath`. + +Current constructor (documentation excluded): + +```pony + new val create( + base: (FilePath | AmbientAuth), + path': String, + caps': FileCaps val = recover val FileCaps .> all() end) + ? + => + caps.union(caps') + + path = match base + | let b: FilePath => + if not b.caps(FileLookup) then + error + end + + let tmp_path = Path.join(b.path, path') + caps.intersect(b.caps) + + if not tmp_path.at(b.path, 0) then + error + end + tmp_path + | let b: AmbientAuth => + Path.abs(path') + end +``` + +Will become: + +```pony + new val create( + base: AmbientAuth, + path': String, + caps': FileCaps val = recover val FileCaps .> all() end) + => + caps.union(caps') + + path = Path.abs(path') +``` + +and + +```pony + new val from( + base: FilePath, + path': String, + caps': FileCaps val = recover val FileCaps .> all() end) + ? + => + caps.union(caps') + if not base.caps(FileLookup) then + error + end + + let tmp_path = Path.join(base.path, path') + caps.intersect(base.caps) + + if not tmp_path.at(base.path, 0) then + error + end + path = tmp_path +``` + +# How We Teach This + +Nothing about our teaching should need to change, however this change will allow early tutorials to avoid introducing try-else blocks and partial functions for longer if they so choose. For example, with a separate `FilePath` constructor an early tutorial could construct a `FilePath` for a file in the current directory containing a simple message, constructor the necessary `File`, and call `File.read()` or `File.lines`, none of which would be partial functions after this change -- such a program is just beyond the complexity of the ever ubiquitous "Hello, World" initial lesson. + +Advanced users of Pony, should have little difficulty switching based on the simple statistics from stdlib below. + +- Uses of `FilePath`: 166 +- Uses of `FilePath(...)` from `AmbientAuth`: 37 +- Uses of `_ as FilePath` typing: 19 +- Uses of `FilePath(...)` not from `AmbientAuth`: 29 +- Uses referring to `FilePath` for its type (e.g., within unions): 53 +- Uses excluding the above (mostly documentation): 28 + +# How We Test This + +At least one additional unit test is recommended as nearly all existing tests use `AmbientAuth`. It should suffice for this added unit test to first build a path via `AmbientAuth` then build a second path from the first. + +Existing unit tests will need to change as the common phrase `let filepath = FilePath(h.env.root as AmbientAuth, path)?` will not fail. As well, `var tmp_dir: (FilePath | None) = ...` will likely not be needed as this is a defense against `FilePath` failing within a try statement. + +# Drawbacks + +- Breaks existing code +- Added maintenance cost +- May lead to passing `AmbientAuth` around excessively since building from an existing path (`FilePath.from...)`) will become less appealing + +I am not sure we can change how this RFC is implemented to minimize any of the above. The first two are expected with additional breaking code as important as a constructor. The latter is a training issue so is best handled by better educational materials around Pony for _why_ it would be a "bad idea" to pass `AmbientAuth` around excessively. + +# Alternatives + +Keeping the implementation as it currently is written. There is nothing wrong with the existing implementation, this RFC is a suggestion for making the file package slightly easier to use. + +# Unresolved questions + +Does this have farther reaching affects beyond what is initially seen? Are there current patterns outside of stdlib for managing `FilePath`s? From 389984fd106b46a020731c14e75353c974930334 Mon Sep 17 00:00:00 2001 From: Ryan Hagenson Date: Wed, 14 Jul 2021 20:32:49 -0500 Subject: [PATCH 2/3] Add FileAuth suggestion --- text/0000-filepath-constructor.md | 33 +++++++++++++++++++------------ 1 file changed, 20 insertions(+), 13 deletions(-) diff --git a/text/0000-filepath-constructor.md b/text/0000-filepath-constructor.md index 08163f2c..6008c6e0 100644 --- a/text/0000-filepath-constructor.md +++ b/text/0000-filepath-constructor.md @@ -1,19 +1,19 @@ -- Feature Name: Separate `FilePath` constructor from `AmbientAuth` +- Feature Name: Split `FilePath` constructor to guarantee constructor with `AmbientAuth` - Start Date: 2021-07-02 - RFC PR: - Pony Issue: # Summary -It should be without error to constructor a `FilePath` when `AmbientAuth` is provided. The current single constructor uses a union of `(FilePath | AmbientAuth)` to limit capabilities and access for the newly created `FilePath`. Insufficient capabilities or access allows the first half of this union, `FilePath`, to fail, however the latter half `AmbientAuth` can never fail. +It should be without error to constructor a `FilePath` when `AmbientAuth` is available. The current constructor uses a union of `(FilePath | AmbientAuth)` to limit capabilities and access for the newly created `FilePath`. Insufficient capabilities or access allows the first half of this union, `FilePath`, to fail, however the latter half `AmbientAuth` can never fail. # Motivation -Beyond making using `FilePath` more convenient, this change will allow use cases where a path is guaranteed such as constructing a path to the root directory. Currently, all `FilePath` must be constructed within a try-else block. Importantly, `FilePath` does not guarantee its operations will succeed, however a `FilePath` is merely a combination of some directory path and capabilities. No changes to __using__ a `FilePath` are intended by this change. +Beyond making `FilePath` more convenient, this change will allow use cases where a path is guaranteed such as constructing a path to the root directory. Currently, all `FilePath` objects must be constructed within a try-else block. Importantly, `FilePath` does not guarantee its post-construction operations will succeed, however a `FilePath` is merely a combination of some directory path and capabilities. No changes to __using__ a `FilePath` are intended by this change, only construction is affected. # Detailed design -The suggested change is to split the implementation in two around the current match statement with `create` using `AmbientAuth` and a new constructor called `from` using an existing `FilePath`. +The suggested change is to split the implementation in two around the current match statement with `create` using a new package-level root authority `FileAuth` (created from `AmbientAuth`) and a new constructor called `from` using an existing `FilePath`. Current constructor (documentation excluded): @@ -48,7 +48,7 @@ Will become: ```pony new val create( - base: AmbientAuth, + base: FileAuth, path': String, caps': FileCaps val = recover val FileCaps .> all() end) => @@ -80,9 +80,19 @@ and path = tmp_path ``` +Definition of `FileAuth`: + +```pony +primitive FileAuth + new create(from: AmbientAuth) => + None +``` + +This matches the net package design of using a package-level root authority rather than `AmbientAuth` directly. + # How We Teach This -Nothing about our teaching should need to change, however this change will allow early tutorials to avoid introducing try-else blocks and partial functions for longer if they so choose. For example, with a separate `FilePath` constructor an early tutorial could construct a `FilePath` for a file in the current directory containing a simple message, constructor the necessary `File`, and call `File.read()` or `File.lines`, none of which would be partial functions after this change -- such a program is just beyond the complexity of the ever ubiquitous "Hello, World" initial lesson. +Nothing about our teaching should need to change, however this change will allow early tutorials to avoid introducing try-else blocks and partial functions for longer if they so choose. For example, with a separate `FilePath` constructor an early tutorial could construct a `FilePath` to a file containing a simple message in the current directory, construct the necessary `File`, and call `File.read()` or `File.lines()`, none of which would be partial functions after this change -- such a program is just beyond the complexity of the ever ubiquitous "Hello, World" initial lesson. Advanced users of Pony, should have little difficulty switching based on the simple statistics from stdlib below. @@ -95,22 +105,19 @@ Advanced users of Pony, should have little difficulty switching based on the sim # How We Test This -At least one additional unit test is recommended as nearly all existing tests use `AmbientAuth`. It should suffice for this added unit test to first build a path via `AmbientAuth` then build a second path from the first. +All unit tests with need to be updated as nearly all existing tests use `AmbientAuth`. At least one additional unit test is recommended to check that serial construction first via `FileAuth` then via `FilePath.from(...)` using the first construction's `FilePath` is error-free when both are attempting to access an identical path. -Existing unit tests will need to change as the common phrase `let filepath = FilePath(h.env.root as AmbientAuth, path)?` will not fail. As well, `var tmp_dir: (FilePath | None) = ...` will likely not be needed as this is a defense against `FilePath` failing within a try statement. +Existing unit tests will need to change as the common phrase `let filepath = FilePath(h.env.root as AmbientAuth, path)?` will no longer work. As well, `var tmp_dir: (FilePath | None) = ...` will likely not be needed in as many places as this is a defense against `FilePath` failing within a try statement. # Drawbacks - Breaks existing code - Added maintenance cost -- May lead to passing `AmbientAuth` around excessively since building from an existing path (`FilePath.from...)`) will become less appealing - -I am not sure we can change how this RFC is implemented to minimize any of the above. The first two are expected with additional breaking code as important as a constructor. The latter is a training issue so is best handled by better educational materials around Pony for _why_ it would be a "bad idea" to pass `AmbientAuth` around excessively. # Alternatives -Keeping the implementation as it currently is written. There is nothing wrong with the existing implementation, this RFC is a suggestion for making the file package slightly easier to use. +Keeping the implementation as it currently is written. There is nothing overtly wrong with the existing implementation, this RFC is a suggestion for making the files package easier to use. # Unresolved questions -Does this have farther reaching affects beyond what is initially seen? Are there current patterns outside of stdlib for managing `FilePath`s? +Does this have farther reaching affects beyond what is initially seen? Are there current patterns outside of stdlib for managing `FilePath`s? Do we have any need for sub-authorities below `FileAuth`? From 8cd920ebd8e27ef027e3a64c9f06ea117d2a9dec Mon Sep 17 00:00:00 2001 From: Ryan Hagenson Date: Thu, 29 Jul 2021 20:41:50 -0500 Subject: [PATCH 3/3] Fix typo/wording in Summary --- text/0000-filepath-constructor.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/text/0000-filepath-constructor.md b/text/0000-filepath-constructor.md index 6008c6e0..0df85d76 100644 --- a/text/0000-filepath-constructor.md +++ b/text/0000-filepath-constructor.md @@ -5,7 +5,7 @@ # Summary -It should be without error to constructor a `FilePath` when `AmbientAuth` is available. The current constructor uses a union of `(FilePath | AmbientAuth)` to limit capabilities and access for the newly created `FilePath`. Insufficient capabilities or access allows the first half of this union, `FilePath`, to fail, however the latter half `AmbientAuth` can never fail. +It should be without error to construct a `FilePath` when `AmbientAuth` is available as `FilePath` is a pairing between some path string and capabilities on that path. For capabilities, the current constructor uses a union of `(FilePath | AmbientAuth)`. Insufficient capabilities or access allows the first half of this union, `FilePath`, to fail, however the latter half, `AmbientAuth`, can never fail. # Motivation