Preliminary Elixir support#5
Conversation
📝 WalkthroughWalkthroughAdds Elixir query parsing (dotted/module shorthand, optional arity), Elixir module-name utilities, .ex scanner to populate supported-functions data, CLI flags (--ex/--erl) and help/README updates, build/install tweaks, and expanded EUnit tests. ChangesElixir Support for AtomVM Spectrometer
Sequence Diagram (high-level Elixir scan + query flow): sequenceDiagram
participant CLI
participant parse_query_string
participant normalize_module_name
participant scan_atomvm_repo
participant scan_elixir_libs
participant parse_elixir_file
participant supported_data
CLI->>parse_query_string: receive query (Elixir.GPIO.digital_read/1)
parse_query_string->>normalize_module_name: normalize module token
CLI->>scan_atomvm_repo: run updater scan
scan_atomvm_repo->>scan_elixir_libs: discover libs/exavmlib
scan_elixir_libs->>parse_elixir_file: read .ex, extract def exports
parse_elixir_file->>supported_data: record {Module,Fun,Arity}
parse_query_string->>supported_data: lookup matches
supported_data->>CLI: return filtered/formatted lines
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/spectrometer_atomvm.erl`:
- Around line 547-549: The bug: when Arity is the atom all you call
format_function_line(Fun, [], ...) which renders a blank "/" arity; change the
handling so the arity all is preserved and rendered explicitly (e.g. "all" or
"*" ) instead of converting to []. Update format_function_line so there is an
explicit clause for format_function_line(Fun, all, Platform, Since) (or detect
is_atom(Arity) and Arity == all) and ensure the downstream arity-formatting
logic prints a clear wildcard token (reference format_function_line and any
helper that formats the arity used by it); apply the same change to the other
occurrence around the 560-564 region so "all" never becomes an empty arity
string.
- Around line 335-337: Update the invalid-format error string to mention the
supported dot syntax; replace the tuple {error, "Invalid format. Use
Module:Function or Module:Function/Arity"} (the branch handling DotPos) with a
message that includes both Module:Function[/Arity] and Module.Function[/Arity]
so users see the dot syntax is supported.
In `@src/spectrometer_help.erl`:
- Line 189: Update the help text for the "--ex" flag to accurately describe its
behavior: it filters by Elixir module type rather than rewriting names. Replace
the string "Show only Elixir functions (strip Elixir. prefix)" with a concise,
correct description such as "Show only Elixir modules" or "Filter output to
Elixir modules (does not rename or strip module prefixes)"; locate and edit the
literal string in spectrometer_help.erl (the line containing the "--ex"
description) and ensure the new wording does not promise name normalization.
In `@src/spectrometer_updater.erl`:
- Around line 1088-1105: parse_elixir_file/4 currently attributes all def
exports to the file basename; instead parse module scopes from the source and
assign each export to its enclosing module/impl. Change the export discovery so
that find_elixir_exports (or a new find_elixir_exports_with_modules) scans Lines
for "defmodule" and "defimpl" blocks, tracks the active module/implementation
name (handling nested modules and multiple module blocks in one file), and
returns exported funs as tuples {ModuleNameAtom, FunName, Arity}; then in
parse_elixir_file/4 build Key = {ModuleAtom, FunName, Arity} from the parsed
ModuleNameAtom (not filename) and store maps:put(Key, {Platforms, Since}, A).
Ensure module name strings are converted to "Elixir.<Name>" atoms the same way
ModAtom was before and preserve existing behavior if no module is found.
In `@src/spectrometer_utils.erl`:
- Around line 406-434: Change detection and normalization so we no longer infer
"Elixir." from capitalization: update is_elixir_module_name/1 to only return
true when the atom/string starts with the literal "Elixir." (remove the
leading-uppercase branch), and stop having normalize_module_name/1 add the
"Elixir." prefix based on a regex match of an uppercase first letter; instead
introduce normalize_module_name/2 (normalize_module_name(Str, ElixirFlag)) where
ElixirFlag (provided by the caller who already knows they used Elixir dot
syntax) controls whether to prepend "Elixir." and make the existing
normalize_module_name/1 delegate to normalize_module_name/2 with ElixirFlag =
false so lowercase names like "json" or "Elixir.json" are handled correctly and
only explicit Elixir input gets prefixed.
In `@test/spectrometer_elixir_tests.erl`:
- Around line 81-94: Tests call spectrometer_atomvm:format_function_line but
only ensure it doesn't crash; update each test
(format_function_line_integer_arity_test, format_function_line_all_arities_test,
format_function_line_arities_list_test) to capture the return value and assert
expected output: call spectrometer_atomvm:format_function_line(...) -> Result
and use e.g. ?assertEqual(ExpectedString, Result) or appropriate assertion to
verify the formatted string contains the function name, the arity or arities,
the platform list, and the version tag (e.g. <<"v0.5.0">> / <<"v0.6.0">>), and
add separate assertions for different arity representations (integer, all, list)
to lock expected formatting.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6893d9bf-cf9d-43c7-918b-6cba77f50b6a
📒 Files selected for processing (11)
README.mdpriv/supported_functions.datarebar.config.scriptrelease/install.insrc/atomvm_spectrometer.erlsrc/spectrometer_atomvm.erlsrc/spectrometer_help.erlsrc/spectrometer_updater.erlsrc/spectrometer_utils.erltest/atomvm_spectrometer_tests.erltest/spectrometer_elixir_tests.erl
8e2567b to
e057a16
Compare
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
release/install.in (1)
117-117:⚠️ Potential issue | 🟠 Major | ⚡ Quick winEnsure destination parent exists before copy.
Without creating
${dest_dir}(or at least${prefix}/lib), fresh installs can fail on prefixes missing alibdirectory.Suggested fix
+echo_run mkdir -p "${dest_dir}" echo_run cp -fr "${tmp_dir}" "${dest_dir}"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@release/install.in` at line 117, Before the cp command (echo_run cp -fr "${tmp_dir}" "${dest_dir}"), ensure the destination parent directory exists by creating it with mkdir -p; for example, run mkdir -p "${dest_dir}" (or mkdir -p "${prefix}/lib" if you prefer to ensure the common parent) so the subsequent echo_run cp -fr "${tmp_dir}" "${dest_dir}" cannot fail on fresh prefixes missing a lib directory.src/spectrometer_atomvm.erl (1)
295-297:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate query usage text to include dot syntax.
Parser accepts
Module.Function[/Arity], but usage still only advertises colon format.Suggested fix
- io:format( - standard_error, "Usage: query Module:Function[/Arity]\n", [] - ), + io:format( + standard_error, + "Usage: query Module:Function[/Arity] | Module.Function[/Arity]\n", + [] + ),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/spectrometer_atomvm.erl` around lines 295 - 297, Update the usage message printed by the io:format call in spectrometer_atomvm.erl to reflect that the parser accepts both colon and dot syntax; change the string currently showing "Module:Function[/Arity]" to include the dot form (for example "Module:Function[/Arity] or Module.Function[/Arity]") so callers see both accepted formats when the usage text is printed by the function that does io:format(standard_error, ...).
♻️ Duplicate comments (1)
test/spectrometer_elixir_tests.erl (1)
87-113:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFormatter tests still don't validate formatted output.
These tests only assert
ok, so formatting regressions can still pass undetected. Please assert emitted content (function name/arity/platform/since) rather than just non-crash behavior.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/spectrometer_elixir_tests.erl` around lines 87 - 113, The tests currently only assert ok and don't verify the formatted output; update each test (format_function_line_integer_arity_test, format_function_line_all_arities_test, format_function_line_arities_list_test) to capture the string emitted by spectrometer_atomvm:format_function_line and assert it equals the expected formatted line containing the function name (test_fun), the correct arity rendering ("1", "*" for all, "1,2,3" for lists), the platform(s) (e.g., esp32, stm32) and the "since" version (e.g., "v0.5.0"/"v0.6.0"); use an output-capture helper (EUnit capture_io or equivalent) around the call to format_function_line and replace the ?assertEqual(ok, ...) with an assertion that the captured string matches the exact expected output.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@release/install.in`:
- Around line 98-99: The current install script only runs rm -rf
"${prefix}/lib/atomvm_spectrometer" if sh
"${prefix}/lib/atomvm_spectrometer/uninstall.sh" fails, which can leave stale
files when uninstall.sh exits successfully but doesn't remove everything; change
the sequence so the uninstall script is executed and then always force-remove
the directory (i.e., run the uninstall.sh and subsequently run rm -rf
"${prefix}/lib/atomvm_spectrometer" regardless of uninstall.sh exit status) to
guarantee a clean reinstall.
In `@src/atomvm_spectrometer.erl`:
- Around line 372-375: The parse_supported_args clauses currently insert a
string key "filter" into the options map, violating the atom-keyed opts_map()
contract; change both clauses to update the map with an atom key instead, e.g.
replace Opts#{"filter" => erlang_only} and Opts#{"filter" => elixir_only} with
Opts#{filter => erlang_only} and Opts#{filter => elixir_only} in the
parse_supported_args function so the map uses the atom filter key consistently
with opts_map().
In `@src/spectrometer_atomvm.erl`:
- Line 453: Replace the string-key lookup for the filter option with an atom-key
lookup: change the maps:get("filter", Opts, undefined) call that assigns Filter
to use the atom key (maps:get(filter, Opts, undefined)) so the Opts map stays
consistent with atom-keyed options and the parser-side storage; update the
assignment to Filter accordingly.
In `@src/spectrometer_updater.erl`:
- Around line 1241-1252: count_arity/1 currently strips whitespace and splits on
all commas, which over-counts when commas appear inside nested (), [], or {};
replace this by implementing a scanner (e.g., count_top_level_args/3) that
iterates the CleanArgs string, tracks nesting depth for '()', '[]', and '{}' and
only treats commas at depth 0 as argument separators, returning 0 for empty
strings and otherwise 1 + top-level-comma-count; update count_arity/1 to call
this new function and return the correct arity.
In `@test/spectrometer_otp_tests.erl`:
- Around line 99-101: The cleanup callbacks currently return a zero-arity fun
instead of performing cleanup (e.g., fun(TempDir) -> fun() ->
spectrometer_utils:purge_dir(TempDir) end end), so temp dirs are never removed;
fix by removing the nested fun wrapper and invoke
spectrometer_utils:purge_dir(TempDir) directly in the callback (replace the
outer fun to call spectrometer_utils:purge_dir/1 rather than returning another
fun), and apply the same change to all similar instances (the blocks around
lines referencing spectrometer_utils:purge_dir in this file).
In `@test/spectrometer_reporter_tests.erl`:
- Around line 211-214: The CSV line-count assertion is nondeterministic because
binary:split(Content, <<"\n">>, [global]) can produce a trailing empty element
if Content ends with a newline; update the test to remove empty trailing entries
after splitting (e.g. call binary:split(Content, <<"\n">>, [global, trim]) or
filter out <<>> entries from Lines) before asserting length(Lines) equals 3 so
the assertion is stable; change the code around the variable Lines (and the
binary:split call that uses Content) accordingly.
---
Outside diff comments:
In `@release/install.in`:
- Line 117: Before the cp command (echo_run cp -fr "${tmp_dir}" "${dest_dir}"),
ensure the destination parent directory exists by creating it with mkdir -p; for
example, run mkdir -p "${dest_dir}" (or mkdir -p "${prefix}/lib" if you prefer
to ensure the common parent) so the subsequent echo_run cp -fr "${tmp_dir}"
"${dest_dir}" cannot fail on fresh prefixes missing a lib directory.
In `@src/spectrometer_atomvm.erl`:
- Around line 295-297: Update the usage message printed by the io:format call in
spectrometer_atomvm.erl to reflect that the parser accepts both colon and dot
syntax; change the string currently showing "Module:Function[/Arity]" to include
the dot form (for example "Module:Function[/Arity] or Module.Function[/Arity]")
so callers see both accepted formats when the usage text is printed by the
function that does io:format(standard_error, ...).
---
Duplicate comments:
In `@test/spectrometer_elixir_tests.erl`:
- Around line 87-113: The tests currently only assert ok and don't verify the
formatted output; update each test (format_function_line_integer_arity_test,
format_function_line_all_arities_test, format_function_line_arities_list_test)
to capture the string emitted by spectrometer_atomvm:format_function_line and
assert it equals the expected formatted line containing the function name
(test_fun), the correct arity rendering ("1", "*" for all, "1,2,3" for lists),
the platform(s) (e.g., esp32, stm32) and the "since" version (e.g.,
"v0.5.0"/"v0.6.0"); use an output-capture helper (EUnit capture_io or
equivalent) around the call to format_function_line and replace the
?assertEqual(ok, ...) with an assertion that the captured string matches the
exact expected output.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e458c6f8-98c7-47d2-a0b2-39810fbf6f57
📒 Files selected for processing (16)
README.mdpriv/supported_functions.datarebar.config.scriptrelease/install.insrc/atomvm_spectrometer.erlsrc/spectrometer_atomvm.erlsrc/spectrometer_help.erlsrc/spectrometer_updater.erlsrc/spectrometer_utils.erltest/atomvm_spectrometer_tests.erltest/spectrometer_elixir_tests.erltest/spectrometer_otp_tests.erltest/spectrometer_reporter_tests.erltest/spectrometer_scanner_tests.erltest/spectrometer_updater_tests.erltest/spectrometer_utils_tests.erl
✅ Files skipped from review due to trivial changes (3)
- README.md
- src/spectrometer_help.erl
- priv/supported_functions.data
e057a16 to
27e2dba
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
src/spectrometer_atomvm.erl (1)
54-55: ⚡ Quick winHide the test-only helper from xref.
The pipeline is already warning that
format_function_line_string/4is an unused export. If this is only for tests, gate the export behind a test macro or add a local xref ignore so code-quality stays clean.Also applies to: 583-624
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/spectrometer_atomvm.erl` around lines 54 - 55, The export format_function_line_string/4 is only used in tests and is causing xref warnings—wrap its export in a test guard (e.g. -ifdef(TEST) ... -endif) so it’s only exported during tests, and apply the same pattern to other test-only helpers in the block around lines 583-624; locate the export for format_function_line_string/4 and the other test helpers and move each -export(...) into an -ifdef(TEST) / -endif pair (or alternatively remove the export and add an -ifdef(TEST) export declaration) so the functions remain available to tests but are hidden from xref in normal builds.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@release/install.in`:
- Line 110: Remove the stray leading pipe character from the comment line that
currently reads "|# Extract the embedded tar.gz" so it becomes a normal shell
comment "# Extract the embedded tar.gz" (open release/install.in, find the exact
string "|# Extract the embedded tar.gz" and delete the leading "|" to fix the sh
syntax error).
In `@src/spectrometer_updater.erl`:
- Around line 1257-1266: The clause patterns in count_top_level_commas/3 are
using one-character strings (e.g. "(") but Char is an integer codepoint; change
the case patterns to Erlang character literals (use $() for "(", $[ for "[", ${
for "{", $) for ")", $] for "]", $} for "}", and $, for ",") so delimiters and
the top-level comma clause (when Depth == 0) will match the integer Char values
correctly.
- Around line 1126-1130: The defimpl handling currently only uses the protocol
name from find_elixir_module_def/1 and then builds the module atom in
find_elixir_exports/4 as list_to_atom("Elixir." ++ ModName), which omits the
for: target (so e.g. Enumerable, for: List becomes Elixir.Enumerable instead of
Elixir.Enumerable.List); update the logic so the parsed defimpl record from
find_elixir_module_def/1 carries the target (for) identifier and
find_elixir_exports/4 composes the full module atom by appending the target
(e.g. "Elixir." ++ ProtocolName ++ "." ++ TargetName) before calling
list_to_atom, ensuring defimpl entries map to Elixir.Protocol.Target (also
adjust the related branch at the second occurrence around the
find_elixir_exports/4 usage to use the same composed ModAtom).
In `@test/atomvm_spectrometer_tests.erl`:
- Around line 458-472: The tests parse_supported_args_erl_test_ and
parse_supported_args_ex_test_ are using a string key for maps:get("filter",
Opts) but atomvm_spectrometer:parse_args returns a map with an atom key :filter;
update both assertions to use the atom key (e.g., maps:get(filter, Opts) or
Opts# maps access) so they compare erlang_only and elixir_only against the
atom-keyed value returned by parse_args.
In `@test/spectrometer_elixir_tests.erl`:
- Around line 87-106: Tests call
spectrometer_atomvm:format_function_line_string/4 which returns iodata;
normalize Output with iolist_to_binary/1 (e.g. OutputBin =
iolist_to_binary(spectrometer_atomvm:format_function_line_string(...))) and
compare against a binary expected (<<" test_fun/1 (all since: v0.5.0)\n">>,
etc.) in format_function_line_integer_arity_test,
format_function_line_all_arities_test and format_function_line_arities_list_test
so assertions don't fail across CI matrices.
In `@test/spectrometer_updater_tests.erl`:
- Around line 972-979: The test fails because count_arity/1 in module
spectrometer_updater treats the whole argument string as one token when spaces
are present; change count_arity/1 to return 0 for the empty string, otherwise
split the input on commas, trim whitespace from each token, ignore empty tokens
and return the count of non-empty tokens (so inputs like "x, y" and " x , y
" yield 2). Locate and update the spectrometer_updater:count_arity/1
implementation referenced by the failing test in spectrometer_updater_tests to
use comma-based splitting and token trimming before counting.
---
Nitpick comments:
In `@src/spectrometer_atomvm.erl`:
- Around line 54-55: The export format_function_line_string/4 is only used in
tests and is causing xref warnings—wrap its export in a test guard (e.g.
-ifdef(TEST) ... -endif) so it’s only exported during tests, and apply the same
pattern to other test-only helpers in the block around lines 583-624; locate the
export for format_function_line_string/4 and the other test helpers and move
each -export(...) into an -ifdef(TEST) / -endif pair (or alternatively remove
the export and add an -ifdef(TEST) export declaration) so the functions remain
available to tests but are hidden from xref in normal builds.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2e5da10b-6765-4c15-8720-716027ade152
📒 Files selected for processing (16)
README.mdpriv/supported_functions.datarebar.config.scriptrelease/install.insrc/atomvm_spectrometer.erlsrc/spectrometer_atomvm.erlsrc/spectrometer_help.erlsrc/spectrometer_updater.erlsrc/spectrometer_utils.erltest/atomvm_spectrometer_tests.erltest/spectrometer_elixir_tests.erltest/spectrometer_otp_tests.erltest/spectrometer_reporter_tests.erltest/spectrometer_scanner_tests.erltest/spectrometer_updater_tests.erltest/spectrometer_utils_tests.erl
✅ Files skipped from review due to trivial changes (2)
- README.md
- src/spectrometer_help.erl
| count_top_level_commas([Char | Rest], Depth, Count) -> | ||
| case Char of | ||
| "(" -> count_top_level_commas(Rest, Depth + 1, Count); | ||
| "[" -> count_top_level_commas(Rest, Depth + 1, Count); | ||
| "{" -> count_top_level_commas(Rest, Depth + 1, Count); | ||
| ")" -> count_top_level_commas(Rest, Depth - 1, Count); | ||
| "]" -> count_top_level_commas(Rest, Depth - 1, Count); | ||
| "}" -> count_top_level_commas(Rest, Depth - 1, Count); | ||
| "," when Depth == 0 -> count_top_level_commas(Rest, Depth, Count + 1); | ||
| _ -> count_top_level_commas(Rest, Depth, Count) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate and print the exact implementation around the reported lines
FILE="src/spectrometer_updater.erl"
echo "== file =="
echo "$FILE"
echo "== lines 1240-1290 =="
nl -ba "$FILE" | sed -n '1240,1295p'
# Also find the definition header to confirm arity and any callers/assumptions
echo "== rg: definition of count_top_level_commas/3 =="
rg -n "count_top_level_commas/3|count_top_level_commas\(" "$FILE"
# Extract any surrounding functions that call count_top_level_commas/3 (small neighborhood)
echo "== search callers (limited context) =="
rg -n "count_top_level_commas\(" "$FILE" | head -n 50Repository: UncleGrumpy/atomvm_spectrometer
Length of output: 198
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="src/spectrometer_updater.erl"
echo "== lines 1235-1275 =="
awk 'NR>=1235 && NR<=1275 {printf "%d:%s\n", NR, $0}' "$FILE"
echo "== function headers (maybe multiple arities) =="
rg -n "count_top_level_commas\s*\(" "$FILE"
echo "== show exact case clause block containing the delimiter patterns =="
rg -n "case Char of" "$FILE" -n || true
# Print a small region around the first occurrence
line=$(rg -n "case Char of" "$FILE" | head -n1 | cut -d: -f1)
if [ -n "${line:-}" ]; then
start=$((line-5))
end=$((line+25))
echo "== around case Char of (lines $start-$end) =="
awk -v s=$start -v e=$end 'NR>=s && NR<=e {printf "%d:%s\n", NR, $0}' "$FILE"
fi
echo "== search for how Char is produced/pattern-matched (call sites) =="
# Look for where count_top_level_commas/3 is called and what the first argument is
rg -n "count_top_level_commas\s*\(" "$FILE"Repository: UncleGrumpy/atomvm_spectrometer
Length of output: 4206
Fix delimiter matching in count_top_level_commas/3 (use Erlang character literals)
Char is an integer codepoint from [Char | Rest] over CleanArgs (built with {return, list}), but the case patterns use one-character strings ("(", "[", etc.), which won’t match; delimiters are therefore never recognized.
Suggested fix
count_top_level_commas([Char | Rest], Depth, Count) ->
case Char of
- "(" -> count_top_level_commas(Rest, Depth + 1, Count);
- "[" -> count_top_level_commas(Rest, Depth + 1, Count);
- "{" -> count_top_level_commas(Rest, Depth + 1, Count);
- ")" -> count_top_level_commas(Rest, Depth - 1, Count);
- "]" -> count_top_level_commas(Rest, Depth - 1, Count);
- "}" -> count_top_level_commas(Rest, Depth - 1, Count);
- "," when Depth == 0 -> count_top_level_commas(Rest, Depth, Count + 1);
+ $( -> count_top_level_commas(Rest, Depth + 1, Count);
+ $[ -> count_top_level_commas(Rest, Depth + 1, Count);
+ ${ -> count_top_level_commas(Rest, Depth + 1, Count);
+ $) -> count_top_level_commas(Rest, Depth - 1, Count);
+ $] -> count_top_level_commas(Rest, Depth - 1, Count);
+ $} -> count_top_level_commas(Rest, Depth - 1, Count);
+ $, when Depth == 0 -> count_top_level_commas(Rest, Depth, Count + 1);
_ -> count_top_level_commas(Rest, Depth, Count)
end;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/spectrometer_updater.erl` around lines 1257 - 1266, The clause patterns
in count_top_level_commas/3 are using one-character strings (e.g. "(") but Char
is an integer codepoint; change the case patterns to Erlang character literals
(use $() for "(", $[ for "[", ${ for "{", $) for ")", $] for "]", $} for "}",
and $, for ",") so delimiters and the top-level comma clause (when Depth == 0)
will match the integer Char values correctly.
| format_function_line_integer_arity_test() -> | ||
| % Test that format_function_line_string handles integer arity correctly | ||
| Output = spectrometer_atomvm:format_function_line_string( | ||
| test_fun, 1, all, <<"v0.5.0">> | ||
| ), | ||
| ?assertEqual(" test_fun/1 (all since: v0.5.0)\n", Output). | ||
|
|
||
| format_function_line_all_arities_test() -> | ||
| % Test that "all" arity renders as "*" instead of empty string | ||
| Output = spectrometer_atomvm:format_function_line_string( | ||
| test_fun, all, [esp32], <<"v0.5.0">> | ||
| ), | ||
| ?assertEqual(" test_fun/* (esp32 since: v0.5.0)\n", Output). | ||
|
|
||
| format_function_line_arities_list_test() -> | ||
| % Test that list of arities renders correctly | ||
| Output = spectrometer_atomvm:format_function_line_string( | ||
| test_fun, [1, 2, 3], [stm32], <<"v0.6.0">> | ||
| ), | ||
| ?assertEqual(" test_fun/1/2/3 (stm32 since: v0.6.0)\n", Output). |
There was a problem hiding this comment.
Normalize iolist output before equality assertions in format-line tests.
format_function_line_string/4 returns iodata in current behavior, so direct string equality is failing across CI matrices.
Suggested fix
- Output = spectrometer_atomvm:format_function_line_string(
+ Output = iolist_to_binary(spectrometer_atomvm:format_function_line_string(
test_fun, 1, all, <<"v0.5.0">>
- ),
- ?assertEqual(" test_fun/1 (all since: v0.5.0)\n", Output).
+ )),
+ ?assertEqual(<<" test_fun/1 (all since: v0.5.0)\n">>, Output).
- Output = spectrometer_atomvm:format_function_line_string(
+ Output = iolist_to_binary(spectrometer_atomvm:format_function_line_string(
test_fun, all, [esp32], <<"v0.5.0">>
- ),
- ?assertEqual(" test_fun/* (esp32 since: v0.5.0)\n", Output).
+ )),
+ ?assertEqual(<<" test_fun/* (esp32 since: v0.5.0)\n">>, Output).
- Output = spectrometer_atomvm:format_function_line_string(
+ Output = iolist_to_binary(spectrometer_atomvm:format_function_line_string(
test_fun, [1, 2, 3], [stm32], <<"v0.6.0">>
- ),
- ?assertEqual(" test_fun/1/2/3 (stm32 since: v0.6.0)\n", Output).
+ )),
+ ?assertEqual(<<" test_fun/1/2/3 (stm32 since: v0.6.0)\n">>, Output).🧰 Tools
🪛 GitHub Actions: Build and Test / 0_build-and-test (28).txt
[error] 92-92: Test failure: spectrometer_elixir_tests:format_function_line_integer_arity_test/0 expected " test_fun/1 (all since: v0.5.0)\n" but got list output
[error] 99-99: Test failure: spectrometer_elixir_tests:format_function_line_all_arities_test/0 expected " test_fun/* (esp32 since: v0.5.0)\n" but got list output
[error] 106-106: Test failure: spectrometer_elixir_tests:format_function_line_arities_list_test/0 expected " test_fun/1/2/3 (stm32 since: v0.6.0)\n" but got list output
🪛 GitHub Actions: Build and Test / 1_build-and-test (29, 3.27.0).txt
[error] 92-92: spectrometer_elixir_tests: format_function_line_integer_arity_test failed: expected output string for "test_fun/1" but got an iolist/list of integers
[error] 99-99: spectrometer_elixir_tests: format_function_line_all_arities_test failed: expected output string for "test_fun/*" but got an iolist/list of integers
[error] 106-106: spectrometer_elixir_tests: format_function_line_arities_list_test failed: expected output string for "test_fun/1/2/3" but got an iolist/list of integers
🪛 GitHub Actions: Build and Test / 2_build-and-test (27).txt
[error] 92-92: spectrometer_elixir_tests:format_function_line_integer_arity_test/0: expected " test_fun/1 (all since: v0.5.0)\n" but got output as list/char tokens.
[error] 99-99: spectrometer_elixir_tests:format_function_line_all_arities_test/0: expected " test_fun/* (esp32 since: v0.5.0)\n" but got output as list/char tokens.
[error] 106-106: spectrometer_elixir_tests:format_function_line_arities_list_test/0: expected " test_fun/1/2/3 (stm32 since: v0.6.0)\n" but got output as list/char tokens.
🪛 GitHub Actions: Build and Test / 3_build-and-test (master).txt
[error] 92-92: EUnit failure: format_function_line_integer_arity_test. Expected " test_fun/1 (all since: v0.5.0)\n", got list output
[error] 99-99: EUnit failure: format_function_line_all_arities_test. Expected " test_fun/* (esp32 since: v0.5.0)\n", got list output
[error] 106-106: EUnit failure: format_function_line_arities_list_test. Expected " test_fun/1/2/3 (stm32 since: v0.6.0)\n", got list output
🪛 GitHub Actions: Build and Test / build-and-test (27)
[error] 92-92: Test failure: format_function_line_integer_arity_test expected string " test_fun/1 (all since: v0.5.0)\n" but got a list with incorrect tokenization (output contained [32,32,"test_fun",...])
[error] 99-99: Test failure: format_function_line_all_arities_test expected " test_fun/* (esp32 since: v0.5.0)\n" but got tokenized list output ("esp32" and "v0.5.0" fragments present)
[error] 106-106: Test failure: format_function_line_arities_list_test expected " test_fun/1/2/3 (stm32 since: v0.6.0)\n" but got tokenized list output ("1/2/3", "stm32", "v0.6.0" present)
🪛 GitHub Actions: Build and Test / build-and-test (28)
[error] 92-92: Test failure: format_function_line_integer_arity_test. Expected " test_fun/1 (all since: v0.5.0)\n" but got a different representation (charlist-like output)
[error] 99-99: Test failure: format_function_line_all_arities_test. Expected " test_fun/* (esp32 since: v0.5.0)\n" but got a different representation (charlist-like output)
[error] 106-106: Test failure: format_function_line_arities_list_test. Expected " test_fun/1/2/3 (stm32 since: v0.6.0)\n" but got a different representation (charlist-like output)
🪛 GitHub Actions: Build and Test / build-and-test (29, 3.27.0)
[error] 92-92: Test failure: spectrometer_elixir_tests:format_function_line_integer_arity_test/0. Expected " test_fun/1 (all since: v0.5.0)\n" but got a charlist representation ("test_fun", "1", "all", "v0.5.0").
[error] 99-99: Test failure: spectrometer_elixir_tests:format_function_line_all_arities_test/0. Expected " test_fun/* (esp32 since: v0.5.0)\n" but got a charlist representation ("test_fun", "*", "esp32", "v0.5.0").
[error] 106-106: Test failure: spectrometer_elixir_tests:format_function_line_arities_list_test/0. Expected " test_fun/1/2/3 (stm32 since: v0.6.0)\n" but got a charlist representation ("test_fun", "1/2/3", "stm32", "v0.6.0").
🪛 GitHub Actions: Build and Test / build-and-test (master)
[error] 92-92: spectrometer_elixir_tests: format_function_line_integer_arity_test failed: expected " test_fun/1 (all since: v0.5.0)\n" got different output
[error] 99-99: spectrometer_elixir_tests: format_function_line_all_arities_test failed: expected " test_fun/* (esp32 since: v0.5.0)\n" got different output
[error] 106-106: spectrometer_elixir_tests: format_function_line_arities_list_test failed: expected " test_fun/1/2/3 (stm32 since: v0.6.0)\n" got different output
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/spectrometer_elixir_tests.erl` around lines 87 - 106, Tests call
spectrometer_atomvm:format_function_line_string/4 which returns iodata;
normalize Output with iolist_to_binary/1 (e.g. OutputBin =
iolist_to_binary(spectrometer_atomvm:format_function_line_string(...))) and
compare against a binary expected (<<" test_fun/1 (all since: v0.5.0)\n">>,
etc.) in format_function_line_integer_arity_test,
format_function_line_all_arities_test and format_function_line_arities_list_test
so assertions don't fail across CI matrices.
| count_arity_test_() -> | ||
| {"counts function arity from argument string", fun() -> | ||
| ?assertEqual(0, spectrometer_updater:count_arity("")), | ||
| ?assertEqual(1, spectrometer_updater:count_arity("x")), | ||
| ?assertEqual(2, spectrometer_updater:count_arity("x, y")), | ||
| ?assertEqual(3, spectrometer_updater:count_arity("x, y, z")), | ||
| ?assertEqual(2, spectrometer_updater:count_arity(" x , y ")) | ||
| end}. |
There was a problem hiding this comment.
count_arity/1 currently fails for spaced argument lists and is red in CI.
The new test is valid for Elixir-style signatures ("x, y"), but implementation returns 1. Please update count_arity/1 to split on commas and trim tokens before counting.
🧰 Tools
🪛 GitHub Actions: Build and Test / 0_build-and-test (28).txt
[error] 976-976: Test failure: spectrometer_updater_tests:-count_arity_test_/0-fun-5-/0 counts function arity from argument string. expected: 2, got: 1
🪛 GitHub Actions: Build and Test / 1_build-and-test (29, 3.27.0).txt
[error] 976-976: spectrometer_updater_tests: -count_arity_test failed: expected 2 from count_arity("x, y"), got 1
🪛 GitHub Actions: Build and Test / 2_build-and-test (27).txt
[error] 976-976: spectrometer_updater_tests:-count_arity_test_/0-fun-5-/0: count_arity("x, y") expected 2, got 1.
🪛 GitHub Actions: Build and Test / 3_build-and-test (master).txt
[error] 976-976: EUnit failure: counts function arity from argument string. ?assertEqual(2, spectrometer_updater:count_arity("x, y")) failed (expected 2, got 1)
🪛 GitHub Actions: Build and Test / build-and-test (27)
[error] 976-976: Test failure: count_arity("x, y") expected 2, got 1
🪛 GitHub Actions: Build and Test / build-and-test (28)
[error] 976-976: Test failure: counts function arity from argument string. ?assertEqual(2, spectrometer_updater:count_arity("x, y")) expected 2, got 1
🪛 GitHub Actions: Build and Test / build-and-test (29, 3.27.0)
[error] 976-976: Test failure: spectrometer_updater_tests:-count_arity_test_/0 counts function arity from argument string. Expected 2, got 1 for count_arity("x, y").
🪛 GitHub Actions: Build and Test / build-and-test (master)
[error] 976-976: spectrometer_updater_tests: count_arity test failed: expected 2 got 1 for count_arity("x, y")
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/spectrometer_updater_tests.erl` around lines 972 - 979, The test fails
because count_arity/1 in module spectrometer_updater treats the whole argument
string as one token when spaces are present; change count_arity/1 to return 0
for the empty string, otherwise split the input on commas, trim whitespace from
each token, ignore empty tokens and return the count of non-empty tokens (so
inputs like "x, y" and " x , y " yield 2). Locate and update the
spectrometer_updater:count_arity/1 implementation referenced by the failing test
in spectrometer_updater_tests to use comma-based splitting and token trimming
before counting.
Fix bug in install.in that can lead to broken installs. Fix bug in `rebar3 as prod tar` where the escript may be omitted. Fix bug when getting temp dir from environment. Signed-off-by: Winford <winford@object.stream>
Signed-off-by: Winford <winford@object.stream>
Add support for Elixir MFA in update, support, and query commands. Signed-off-by: Winford <winford@object.stream>
27e2dba to
a362298
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
src/spectrometer_updater.erl (2)
1278-1287:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse Erlang character literals in the comma scanner.
Charhere is an integer codepoint from a charlist, but thecasearms are matching one-character strings. None of the delimiter branches will ever fire, so multi-argument Elixir defs are recorded with the wrong arity.🛠️ Proposed fix
count_top_level_commas([Char | Rest], Depth, Count) -> case Char of - "(" -> count_top_level_commas(Rest, Depth + 1, Count); - "[" -> count_top_level_commas(Rest, Depth + 1, Count); - "{" -> count_top_level_commas(Rest, Depth + 1, Count); - ")" -> count_top_level_commas(Rest, Depth - 1, Count); - "]" -> count_top_level_commas(Rest, Depth - 1, Count); - "}" -> count_top_level_commas(Rest, Depth - 1, Count); - "," when Depth == 0 -> count_top_level_commas(Rest, Depth, Count + 1); + $( -> count_top_level_commas(Rest, Depth + 1, Count); + $[ -> count_top_level_commas(Rest, Depth + 1, Count); + ${ -> count_top_level_commas(Rest, Depth + 1, Count); + $) -> count_top_level_commas(Rest, Depth - 1, Count); + $] -> count_top_level_commas(Rest, Depth - 1, Count); + $} -> count_top_level_commas(Rest, Depth - 1, Count); + $, when Depth == 0 -> count_top_level_commas(Rest, Depth, Count + 1); _ -> count_top_level_commas(Rest, Depth, Count) end;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/spectrometer_updater.erl` around lines 1278 - 1287, The case in count_top_level_commas is matching one-character strings but Char is an integer codepoint; change all delimiter and comma patterns in count_top_level_commas to Erlang character literals (e.g., use $\(, $[, ${, $), $], $}, $,) so the branches (parentheses/brackets/braces and top-level comma) actually match; update the guard for the comma branch to use Depth == 0 as-is and ensure the function still recurses on Rest for all branches (keeping the same call sites: count_top_level_commas(Rest, Depth + 1, Count), count_top_level_commas(Rest, Depth - 1, Count), count_top_level_commas(Rest, Depth, Count + 1), and default).
1183-1223:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftModule scope still collapses on any standalone
end.
find_elixir_module_def/1returnsend_defmodulefor every bareend, so function/case/ifendings still popModuleStacktoo. The new{unknown, [...]}entry checked intopriv/supported_functions.datais the downstream symptom: module context is still being lost while scanning.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/spectrometer_updater.erl` around lines 1183 - 1223, find_elixir_module_def/1 currently returns end_defmodule for any standalone "end", which causes ModuleStack to pop module entries incorrectly (producing {unknown,...} in priv/supported_functions.data); change the function so a plain "end" returns a generic end marker (e.g. {end_block} or just end) instead of end_defmodule, and only return end_defmodule when you can match it to a previously opened defmodule/defimpl (or include the matching opener name in the return so the caller can decide). Update the caller logic that manages ModuleStack to pop module entries only when the returned token explicitly matches a module/impl opener from find_elixir_module_def/1, leaving other ends to close inner constructs (functions/cases) without affecting module context.test/spectrometer_updater_tests.erl (1)
957-957:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix EUnit teardown callbacks and align
count_arity/1test expectations.
- In
test/spectrometer_updater_tests.erlat lines 957 and 1179, the EUnit cleanup callback returnsfun() -> spectrometer_utils:purge_dir(Dir) end(nested fun) instead of executingspectrometer_utils:purge_dir(Dir), so teardown won’t run.- In
test/spectrometer_updater_tests.erl,count_arity_test_expects arity1for"x, y"/"x, y, z"(lines 976-977), butsrc/spectrometer_updater.erl:count_arity/1returnstop_level_commas + 1, which yields2and3respectively.Suggested fix
- fun(Dir) -> fun() -> spectrometer_utils:purge_dir(Dir) end end, fun( + fun(Dir) -> spectrometer_utils:purge_dir(Dir) end, fun( Dir ) -> ... - fun(Dir) -> fun() -> spectrometer_utils:purge_dir(Dir) end end, fun( + fun(Dir) -> spectrometer_utils:purge_dir(Dir) end, fun( Dir ) ->🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/spectrometer_updater_tests.erl` at line 957, The teardown callback currently returns a nested fun instead of executing the purge, and the tests assert wrong arities; change the cleanup callback from returning fun() -> spectrometer_utils:purge_dir(Dir) end (the extra nesting) to a callback that actually calls purge (i.e., fun(Dir) -> spectrometer_utils:purge_dir(Dir) end or return fun() -> spectrometer_utils:purge_dir(Dir) end directly from the setup as EUnit expects), and update the expectations in count_arity_test_ to match spectrometer_updater:count_arity/1 (expect 2 for "x, y" and 3 for "x, y, z"). Reference spectrometer_utils:purge_dir, count_arity/1, and count_arity_test_ when making these edits.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/spectrometer_reporter_tests.erl`:
- Around line 171-178: The test currently asserts that Content contains
<<"header">>, which is not a real CSV header and causes false failures; update
the assertion to check for the actual CSV header token used by the reporter
(instead of <<"header">>)—for example call the reporter's header provider (e.g.
spectrometer_reporter:csv_header() or the module/function that generates the CSV
header) and assert that Content contains that returned binary (using
binary:match(Content, ExpectedHeader) =/= nomatch) so the test validates the
real header string rather than the literal <<"header">>.
---
Duplicate comments:
In `@src/spectrometer_updater.erl`:
- Around line 1278-1287: The case in count_top_level_commas is matching
one-character strings but Char is an integer codepoint; change all delimiter and
comma patterns in count_top_level_commas to Erlang character literals (e.g., use
$\(, $[, ${, $), $], $}, $,) so the branches (parentheses/brackets/braces and
top-level comma) actually match; update the guard for the comma branch to use
Depth == 0 as-is and ensure the function still recurses on Rest for all branches
(keeping the same call sites: count_top_level_commas(Rest, Depth + 1, Count),
count_top_level_commas(Rest, Depth - 1, Count), count_top_level_commas(Rest,
Depth, Count + 1), and default).
- Around line 1183-1223: find_elixir_module_def/1 currently returns
end_defmodule for any standalone "end", which causes ModuleStack to pop module
entries incorrectly (producing {unknown,...} in priv/supported_functions.data);
change the function so a plain "end" returns a generic end marker (e.g.
{end_block} or just end) instead of end_defmodule, and only return end_defmodule
when you can match it to a previously opened defmodule/defimpl (or include the
matching opener name in the return so the caller can decide). Update the caller
logic that manages ModuleStack to pop module entries only when the returned
token explicitly matches a module/impl opener from find_elixir_module_def/1,
leaving other ends to close inner constructs (functions/cases) without affecting
module context.
In `@test/spectrometer_updater_tests.erl`:
- Line 957: The teardown callback currently returns a nested fun instead of
executing the purge, and the tests assert wrong arities; change the cleanup
callback from returning fun() -> spectrometer_utils:purge_dir(Dir) end (the
extra nesting) to a callback that actually calls purge (i.e., fun(Dir) ->
spectrometer_utils:purge_dir(Dir) end or return fun() ->
spectrometer_utils:purge_dir(Dir) end directly from the setup as EUnit expects),
and update the expectations in count_arity_test_ to match
spectrometer_updater:count_arity/1 (expect 2 for "x, y" and 3 for "x, y, z").
Reference spectrometer_utils:purge_dir, count_arity/1, and count_arity_test_
when making these edits.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 47603002-1b8c-4594-8257-cae0ef8739dd
📒 Files selected for processing (16)
README.mdpriv/supported_functions.datarebar.config.scriptrelease/install.insrc/atomvm_spectrometer.erlsrc/spectrometer_atomvm.erlsrc/spectrometer_help.erlsrc/spectrometer_updater.erlsrc/spectrometer_utils.erltest/atomvm_spectrometer_tests.erltest/spectrometer_elixir_tests.erltest/spectrometer_otp_tests.erltest/spectrometer_reporter_tests.erltest/spectrometer_scanner_tests.erltest/spectrometer_updater_tests.erltest/spectrometer_utils_tests.erl
✅ Files skipped from review due to trivial changes (2)
- src/spectrometer_help.erl
- README.md
| ?assert( | ||
| binary:match(Content, <<"lists,map,2">>) =/= nomatch | ||
| ), | ||
| ?assert( | ||
| binary:match(Content, <<"io,format,2">>) =/= nomatch | ||
| ) | ||
| ), | ||
| ?assert(binary:match(Content, <<"header">>) =/= nomatch) | ||
| end) |
There was a problem hiding this comment.
Incorrect CSV header assertion literal.
Line 177 checks for <<"header">>, which is not a real CSV header token and can fail even when output is correct.
Suggested fix
- ?assert(binary:match(Content, <<"header">>) =/= nomatch)
+ ?assert(
+ binary:match(Content, <<"module,function,arity">>) =/= nomatch
+ )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/spectrometer_reporter_tests.erl` around lines 171 - 178, The test
currently asserts that Content contains <<"header">>, which is not a real CSV
header and causes false failures; update the assertion to check for the actual
CSV header token used by the reporter (instead of <<"header">>)—for example call
the reporter's header provider (e.g. spectrometer_reporter:csv_header() or the
module/function that generates the CSV header) and assert that Content contains
that returned binary (using binary:match(Content, ExpectedHeader) =/= nomatch)
so the test validates the real header string rather than the literal
<<"header">>.
Add support for Elixir MFA in update, support, and query commands.
Expand overall test coverage.