estdlib: add filename functions and init:get_arguments/0#2347
Conversation
PR Review —
|
| # | Severity | Area | Summary |
|---|---|---|---|
| 1 | Major | join/1 (exposed via join/2) |
Trailing separator in final component not stripped |
| 2 | Major | rootname/2 |
Strips extension even when it is the whole basename after /, corrupting the name into its parent dir |
| 3 | Minor | basename/1 |
Strips all trailing slashes; OTP strips only one |
| 4 | Minor | basename/2 |
Strips Ext even when name had a single trailing slash |
| 5 | Minor | rootname/1 |
Leading-dot dotfile with no directory: OTP returns [] |
| 6 | Nit | CHANGELOG.md |
Omits basename/2 and rootname/2 |
| 7 | Nit | test_init.erl |
Asserts get_plain_arguments() =:= [] on BEAM too |
1. Major — join does not strip a trailing separator from the result
normalize/1 only collapses repeated /; it never removes a final /. OTP does.
OTP AtomVM
filename:join(["a/b///c/"]) -> "a/b/c" "a/b/c/"
filename:join(["a/"]) -> "a" "a/"
filename:join(["a", ""]) -> "a" "a/"
filename:join(["", ""]) -> [] "/"
filename:join("a/", "b/") -> "a/b" "a/b/"
Note the module docstring literally claims "Redundant directory separators are
removed from the result", and OTP’s own doc example says filename:join(["a/b///c/"]) -> "a/b/c", but AtomVM currently returns "a/b/c/"— so this contradicts the module's own contract.
All existing test_join/test_join2 cases already match OTP, so this fix only
extends correctness; it does not regress any committed assertion.
Fix:
--- a/libs/estdlib/src/filename.erl
+++ b/libs/estdlib/src/filename.erl
@@ do_join
-do_join(_Left, Right) when hd(Right) =:= $/ ->
+do_join(Left, []) ->
+ normalize(Left);
+do_join(_Left, Right) when hd(Right) =:= $/ ->
normalize(Right);
do_join(Left, Right) ->
normalize(Left ++ "/" ++ Right).
@@ normalize
-normalize([]) ->
- [];
-normalize([$/, $/ | Rest]) ->
- normalize([$/ | Rest]);
-normalize([C | Rest]) ->
- [C | normalize(Rest)].
+normalize(Name) ->
+ strip_join_trailing_slash(collapse_slashes(Name)).
+
+%% @private
+%% Collapse every run of consecutive "/" into a single "/".
+collapse_slashes([]) ->
+ [];
+collapse_slashes([$/, $/ | Rest]) ->
+ collapse_slashes([$/ | Rest]);
+collapse_slashes([C | Rest]) ->
+ [C | collapse_slashes(Rest)].
+
+%% @private
+%% Remove a single trailing "/", except for the root "/" itself.
+strip_join_trailing_slash([$/]) ->
+ [$/];
+strip_join_trailing_slash(Name) ->
+ case lists:reverse(Name) of
+ [$/ | Rest] -> lists:reverse(Rest);
+ _ -> Name
+ end.Suggested regression tests: join(["a/"]) -> "a", join(["a",""]) -> "a",
join(["",""]) -> "", join("a/","b/") -> "a/b", join(["a/b///c/"]) -> "a/b/c".
2. Major — rootname/2 corrupts a hidden filename into its parent directory
rootname/2 is plain suffix stripping. OTP intentionally does not strip Ext
when the matched extension begins immediately after a / (i.e. it is the whole
basename). The current code turns a valid file path into its containing directory.
OTP AtomVM
filename:rootname("a/.erl", ".erl") -> "a/.erl" "a/" <-- wrong
filename:rootname("a/.bashrc",...) -> "a/.bashrc" "a/" <-- wrong
This is worse than the .bashrc quirk in #5 because the output is a different,
existing directory.
Fix (keep strip_suffix/2 as-is for basename/2; do not reuse it here):
--- a/libs/estdlib/src/filename.erl
+++ b/libs/estdlib/src/filename.erl
@@ rootname/2
rootname(Name, Ext) ->
- strip_suffix(Name, Ext).
+ strip_root_suffix(Name, Ext).
+
+%% @private
+%% Like strip_suffix/2, but do not strip when Suffix is the whole basename
+%% (i.e. the character before it is a "/"), matching OTP rootname/2.
+strip_root_suffix(Name, Suffix) ->
+ NameLen = length(Name),
+ SuffixLen = length(Suffix),
+ PrefixLen = NameLen - SuffixLen,
+ case NameLen >= SuffixLen andalso lists:nthtail(PrefixLen, Name) =:= Suffix of
+ true ->
+ case PrefixLen > 0 andalso lists:nth(PrefixLen, Name) =:= $/ of
+ true -> Name;
+ false -> lists:sublist(Name, PrefixLen)
+ end;
+ false ->
+ Name
+ end.3. Minor — basename/1 strips all trailing slashes; OTP strips only one
OTP treats only a single trailing / as "directory" marker; with 2+ trailing
slashes the last component is empty.
OTP AtomVM
filename:basename("a//") -> [] "a"
filename:basename("a/b//") -> [] "b"
filename:basename("trailing/slash///") -> [] "slash"
(basename("a/") -> "a" and basename("/") -> "" already agree; dirname/1
agrees with OTP in all these cases.)
Fix (also removes the redundant case lists:reverse(Name) of Reversed -> ...):
--- a/libs/estdlib/src/filename.erl
+++ b/libs/estdlib/src/filename.erl
@@ strip_trailing_slashes
strip_trailing_slashes(Name) ->
- case lists:reverse(Name) of
- Reversed ->
- lists:reverse(skip_slashes(Reversed))
+ case lists:reverse(Name) of
+ [$/ | Rest] -> lists:reverse(Rest);
+ Reversed -> lists:reverse(Reversed)
end.4. Minor — basename/2 divergence around exactly one trailing slash
OTP does not strip Ext when the original name ended in exactly one trailing slash;
the current code calls basename/1 first and then strips.
OTP AtomVM
filename:basename("a/b.erl/", ".erl") -> "b.erl" "b"
filename:basename("a/", "a") -> "a" []
Fix (applies on top of #3):
--- a/libs/estdlib/src/filename.erl
+++ b/libs/estdlib/src/filename.erl
@@ basename/2
basename(Name, Ext) ->
Base = basename(Name),
- strip_suffix(Base, Ext).
+ case has_single_trailing_slash(Name) of
+ true -> Base;
+ false -> strip_suffix(Base, Ext)
+ end.
+
+%% @private
+has_single_trailing_slash(Name) ->
+ case lists:reverse(Name) of
+ [$/, $/ | _] -> false;
+ [$/ | _] -> true;
+ _ -> false
+ end.5. Minor — rootname/1 leading-dot dotfile with no directory
OTP returns [] for a name that is a leading-dot dotfile with no directory prefix.
This is an OTP oddity (note extension(".bashrc") -> []), but it is long-standing
observable behavior.
OTP AtomVM
filename:rootname(".") -> [] "."
filename:rootname(".bashrc") -> [] ".bashrc"
(With a directory prefix they already agree: rootname("a/.bashrc") -> "a/.bashrc";
rootname("..") -> "." and rootname(".a.b") -> ".a" also agree.)
This is the lowest-value fix — arguably not worth matching an OTP bug. If matching is
desired:
--- a/libs/estdlib/src/filename.erl
+++ b/libs/estdlib/src/filename.erl
@@ rootname/1
rootname(Name) ->
- case extension_split(Name) of
- {Root, _Ext} -> Root
+ case leading_dot_only(Name) of
+ true -> [];
+ false ->
+ case extension_split(Name) of
+ {Root, _Ext} -> Root
+ end
end.
+
+%% @private
+leading_dot_only([$. | Rest]) ->
+ not lists:member($/, Rest) andalso not lists:member($., Rest);
+leading_dot_only(_) ->
+ false.6. Nit — CHANGELOG omits basename/2 and rootname/2
The commit exports and implements basename/2 and rootname/2, but the CHANGELOG
entry only lists the /1 variants and join/2.
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
-- Added `filename:dirname/1`, `filename:basename/1`, `filename:extension/1`, `filename:rootname/1` and `filename:join/2`
+- Added `filename:dirname/1`, `filename:basename/1,2`, `filename:extension/1`, `filename:rootname/1,2` and `filename:join/2`7. Nit — test_init assumes BEAM is launched without plain args
init:get_arguments/0 -> [] is consistent with the existing AtomVM stubs for
get_argument/1 and get_plain_arguments/0. The test's comment says the suite runs
without plain arguments on both AtomVM and BEAM. The assertion
[] = init:get_plain_arguments(),is brittle if the harness ever passes -extra/plain args on BEAM. Not blocking if the
CI invocation is fixed and known; otherwise shape-check it (as check_flags/1 already
does for get_arguments/0) rather than asserting [].
What is already correct
- Every committed assertion in
test_filename.erlandtest_init.erlmatches real OTP. dirname/1matches OTP across all fuzzed inputs, including multi-trailing-slash cases.extension/1matches OTP (hidden files, dot-in-non-final-component,foo.,.a.b, etc.).extension_split/extension_lengthhandle the "hidden file has no extension" rule correctly.init:get_arguments/0returning[]is consistent with AtomVM's otherinitstubs.
Highest-value next patch
Finding #1 (join) and #2 (rootname/2) — both are true public-contract violations
with straightforward fixes and regression tests.
Signed-off-by: Paul Guyot <pguyot@kallisys.net>
There was a problem hiding this comment.
All findings resolved — LGTM ✅
The contributor amended the commit (now 68d40259) and addressed every finding. I re-verified the new implementation against your real OTP checkout: zero mismatches across a ~60-input × functions fuzz, and every committed test's expected literal matches real OTP output exactly.
| # | Finding | Status |
|---|---|---|
| 1 | join trailing-separator normalization |
✅ Fixed — strip_join_trailing_slash + collapse_separators |
| 2 | rootname/2 corrupting name into parent dir |
✅ Fixed — strip_root_suffix/4 guards the /-before-Ext case |
| 3 | basename/1 stripping all trailing slashes |
✅ Fixed — now "" for a//, a/b// (single-slash rule) |
| 4 | basename/2 single-trailing-slash case |
✅ Fixed — unified basename/3 scanner mirrors OTP |
| 5 | rootname/1 leading-dot dotfile → [] |
✅ Fixed — leading_dot_only/1 |
| 6 | CHANGELOG omissions | ✅ Fixed — now lists basename/1,2 and rootname/1,2 |
| 7 | test_init brittle [] assertion |
✅ Fixed — now shape-checks plain args |
Bonus improvements they added (also verified correct vs OTP)
joinnow drops/./components (a/./b→a/b,a/.//./b→a/b) while correctly keeping leading./and trailing/.— matches OTP exactly.basename/2cross-componentExt(basename("/a/b", "/b")→"a",basename("a//b", "/b")→"") — matches OTP.- Solid new test coverage for all these edge cases.
No remaining issues. The implementation is now a faithful OTP subset including the quirky edge cases.
These changes are made under both the "Apache 2.0" and the "GNU Lesser General
Public License 2.1 or later" license terms (dual license).
SPDX-License-Identifier: Apache-2.0 OR LGPL-2.1-or-later