feat(rt): add os/absolute-path and os/canonical-path - #699
Open
mparrett wants to merge 2 commits into
Open
Conversation
Collaborator
|
The split is right, and the reasoning for not merging them into one |
Collaborator
Author
|
Follow-up on this os series: please update the README |
Grenadine's host contract wants both, and let-go has no path namespace at all: nothing resolves a relative path, and nothing collapses two names for one file into one string. The two differ in a way worth keeping distinct rather than merging into a single "normalize" call. absolute-path is lexical — it resolves against the working directory and cleans traversal without touching disk, so it answers for a file that does not exist yet, which is what a caller naming something it is about to create needs. canonical-path additionally resolves symlinks, which can only be done by reading the filesystem, so it requires the path to exist and in exchange gives a string that compares and keys correctly. Both go in os alongside rename and delete-tree rather than opening a path namespace for two functions. If the surface grows — join, basename, dirname — a path namespace becomes the better home and these should move with it. Stacks on the os/rename + os/delete-tree branch; both edit the same region of pkg/rt/os.go. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
filepath.Abs cleans ".." lexically, so absolutizing before resolving symlinks collapses ".." against the link's parent instead of the target's. canonical-path therefore reported ENOENT for a path the kernel opens fine: with a/link -> b/c, the input a/link/../target.txt resolved to a/target.txt and failed, where the correct answer is b/target.txt. That is the one guarantee the native sells, and the failure mode is the misleading kind — a hard error for a readable file, hitting exactly the callers told to use this as a comparison key. EvalSymlinks walks a relative path against the cwd itself, so resolving first and absolutizing after costs nothing. The regression test builds its path by concatenation, since filepath.Join would clean the case away before the native saw it. Both natives now refuse an empty path, matching delete-tree. filepath.Abs answers "" with the cwd, so an unset variable produced a plausible path rather than an error. The lg deftest for canonical-path was a false green: both its assertions held from Clean alone, before any filesystem access, and its "relative path" case passed two absolute paths. Replace it with the half lg can honestly check without creating symlinks — that a relative input resolves against the working directory — and leave symlink resolution to the Go tests, which can build links. Add the missing case that absolute-path leaves a symlink unresolved where canonical-path resolves it, which is the distinction the two exist to draw and was untested. Also correct the doc comment: absolute-path is lexical with respect to its argument, not free of filesystem access — it reads the process cwd, which is why it can fail. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on #698; review that first. The delta here is the two path functions.
let-go has no path namespace, so nothing resolves a relative path and nothing collapses two names for one file into a single string. Both are slots in Grenadine's host contract (#688).
Why two functions and not one normalize
They answer different questions, and merging them would force every caller to accept the stricter one.
os/absolute-pathis lexical with respect to its argument. It resolves against the working directory and cleans traversal without looking the path up, so it answers for a path that does not exist yet. That is what a caller naming a file it is about to create needs, and a symlink in the path stays a symlink. It does read the process cwd, so it is not free of filesystem access and can fail when the working directory has been removed.os/canonical-pathalso resolves symlinks. Following a link can only be done by looking, so this one requires the path to exist. In exchange it gives a string that compares and keys correctly: two names for one file produce one answer, which is the property that makes it the right form for a cache key or an identity check.A missing path is an error from
canonical-pathrather than a cleaned-up guess, since there is no honest answer. Both refuse an empty path, matchingos/delete-treeon the parent branch:filepath.Abs("")answers with the cwd, so an unset variable would produce a plausible path instead of an error.Resolution order
canonical-pathresolves symlinks before absolutizing, and the order is load-bearing.filepath.Abscleans..lexically, so absolutizing first collapses..against the link's parent rather than the target's. Witha/link→b/c, the inputa/link/../target.txtbecomesa/target.txtand errors, where the answer isb/target.txt. The first draft of this PR had it the wrong way round and reportedENOENTfor a path the kernel opens fine; there is a regression test, and it builds its path by concatenation becausefilepath.Joinwould clean the case away before the native saw it.Placement
Both go in
osbesiderenameanddelete-treerather than opening apathnamespace for two functions. If the surface grows tojoin,basename, anddirname, apathnamespace becomes the better home and these should move with it. Happy to start that namespace here instead if you'd rather set the shape now.Verification
pkg/rt/os_paths_test.go— 12 tests: resolution against cwd, traversal cleaning, an already-absolute path, symlink resolution, two names collapsing to one string, the missing-path divergence between the two, argument guards, the..-after-symlink regression, and thatabsolute-pathleaves a symlink unresolved wherecanonical-pathresolves it. The symlink cases skip where the platform refuses symlinks, and compare against anEvalSymlinksof the target rather than the raw path, sincet.TempDiritself sits under a symlink on darwin.test/os_paths_test.lg— the same surface from lg, including the lexical-vs-filesystem distinction.linux/amd64,js/wasm,wasip1/wasm, andplan9/amd64.One caveat on browser
js/wasm: Go'swasm_exec.jsshim throwsENOSYSfromprocess.cwd()andfs.lstat(), soabsolute-pathfails on relative input there andcanonical-pathfails on any input. That matchesos/cwd,os/lsandos/stat, which are already dead on that target, so it is not a regression — but unlike them these two carry doc comments making unconditional promises, and I can add a caveat if you'd rather they said so.wasip1andplan9are both fine: wasip1 tracks a real cwd and has workingLstat, and plan9 implementsevalSymlinksasLstatplusClean.Still open from the contract survey
:bytes->utf8and:byte-countare left out because they need a decision rather than a function. Bytes arevm.Stringtoday (read-bytesreturns one), sobytes->utf8is identity and the real question is whether an invalid-UTF-8 check belongs on it.byte-countis a genuine gap —countis rune-based, so(count "héllo")is 5 where the byte length is 6, and lg has no way to ask for the latter. Where it belongs is the open part:count's neighbours are in core, which there is active work to shrink.