Preliminary Elixir support#5
Open
UncleGrumpy wants to merge 4 commits into
Open
Conversation
📝 WalkthroughWalkthroughAdds Elixir support: Elixir-aware query parsing, module-name normalization utilities, recursive .ex scanning and public def extraction, CLI flags to filter output by language, bundled Elixir function data, help/README updates, installer/build tweaks, and broad EUnit test coverage. ChangesElixir Support for AtomVM Spectrometer
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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
UncleGrumpy
force-pushed
the
elixir-support
branch
from
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
UncleGrumpy
force-pushed
the
elixir-support
branch
from
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
Comment on lines
+87
to
+106
| 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.
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>
UncleGrumpy
force-pushed
the
elixir-support
branch
from
27e2dba to
a362298
Compare
Owner
Author
|
@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
UncleGrumpy
force-pushed
the
elixir-support
branch
from
a362298 to
eba9494
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/atomvm_spectrometer.erl (1)
417-417:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMissing-query error text should mention dot syntax too.
The parser now accepts
Module.Function[/Arity], but this message still documents only colon syntax.Suggested fix
-parse_query_args([], _) -> - {error, "No function specified. Usage: query Module:Function[/Arity]"}; +parse_query_args([], _) -> + {error, + "No function specified. Usage: query Module:Function[/Arity] or Module.Function[/Arity]"};🤖 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/atomvm_spectrometer.erl` at line 417, Update the error tuple that currently returns {error, "No function specified. Usage: query Module:Function[/Arity]"} to mention the accepted dot syntax as well; change the message to include both "Module:Function" and "Module.Function" (for example: "Usage: query Module:Function or Module.Function[/Arity]") so the parser's supported syntax is reflected; locate and modify the exact error string literal in the clause that returns this {error, ...} tuple.
♻️ Duplicate comments (2)
test/spectrometer_otp_tests.erl (1)
258-260:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winCleanup callback is a no-op due to nested
funreturn.This setup cleanup returns a function instead of executing cleanup, so the test leaves temp state behind.
Suggested fix
- fun(TempDir) -> - fun() -> filelib:ensure_path(filename:join(TempDir, "noop")) end - end, + fun(TempDir) -> + spectrometer_utils:purge_dir(TempDir) + 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 `@test/spectrometer_otp_tests.erl` around lines 258 - 260, The cleanup callback currently returns a nested fun (fun(TempDir) -> fun() -> filelib:ensure_path(...) end end) so it never runs cleanup; change the callback so it executes ensure_path directly for the TempDir (e.g., replace the outer anonymous fun to call filelib:ensure_path/1 for filename:join(TempDir, "noop") or invoke the inner fun immediately) so the cleanup runs instead of returning a function.src/spectrometer_updater.erl (1)
1109-1141:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftModule scope isn’t actually unwound, so nested modules can mis-attribute later exports.
ModuleStackis pushed but never popped onend_block, so after a nesteddefmoduleends, subsequentdefs can stay bound to the inner module.Possible fix direction
-find_elixir_exports([Line | Rest], ModuleStack, CurrentModule, Acc) -> +find_elixir_exports([Line | Rest], ModuleStack, CurrentModule, Acc) -> case find_elixir_module_def(Line) of {defmodule, ModName} -> ModAtom = spectrometer_utils:atom_from_string("Elixir." ++ ModName), find_elixir_exports(Rest, [ModAtom | ModuleStack], ModAtom, Acc); ... - {end_block} -> - find_elixir_exports(Rest, ModuleStack, CurrentModule, Acc); + {end_block} -> + case ModuleStack of + [_Top | Tail] -> + NextCurrent = + case Tail of + [Parent | _] -> Parent; + [] -> undefined + end, + find_elixir_exports(Rest, Tail, NextCurrent, Acc); + [] -> + find_elixir_exports(Rest, [], CurrentModule, Acc) + end;Also applies to: 1165-1205
🤖 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 1109 - 1141, The function find_elixir_exports is pushing module atoms onto ModuleStack for defmodule/defimpl but never pops them, so nested modules leak their scope; modify the {end_block} branch in find_elixir_exports to detect when an end closes a module/defimpl by comparing CurrentModule to the head of ModuleStack and, if so, pop ModuleStack and set CurrentModule to the new head (or undefined/nil when stack empty), otherwise leave ModuleStack/CurrentModule unchanged for non-module ends; apply the same pop logic to the corresponding end handling used around lines 1165-1205 to ensure ModuleStack and CurrentModule are unwound properly.
🤖 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_updater.erl`:
- Around line 1227-1242: The function extract_function_from_line/1 currently
only matches "def name(args)" and misses zero-arity forms like "def foo do";
update the re:run pattern in extract_function_from_line/1 to also accept
optional parentheses and the "do" block form (e.g. allow either
"\\s*\\(([^)]*)\\)" or "\\s+do\\b") and capture an empty Args string (or a
distinct marker) for zero-arity matches so the function still returns {ok,
FunName, Args} for both parenthesized and "def name do" forms while preserving
existing behavior for names ending with ? or ! and ignoring defp.
In `@test/spectrometer_elixir_tests.erl`:
- Around line 15-32: The test parse_query_8_formats_test currently only lists 4
format strings and also mistakenly overwrites the Expected variable instead of
asserting the result; update the test so FormatsWithArity and FormatsNoArity
together contain all 8 expected input variants (add the missing 4 format
strings) and inside the lists:foreach replace the body so it calls
spectrometer_atomvm:parse_query_string(Format) and compares/asserts the return
equals the Expected tuple (do not rebind Expected). Use the function name
spectrometer_atomvm:parse_query_string and the test name
parse_query_8_formats_test to locate where to add the four missing formats and
the proper assertion.
In `@test/spectrometer_utils_tests.erl`:
- Around line 408-425: The test find_first_file_found_test_ uses a hardcoded
"/tmp/spectrometer_utils_test_marker" which can collide across runs and
platforms; change it to create a unique temporary marker path (e.g., derive a
temp dir from os:getenv("TMPDIR") or use filename:join with a generated unique
suffix using make_ref() or erlang:unique_integer()) and assign that to
MarkerFile, then continue to write the file, call
spectrometer_utils:find_first_file([... MarkerFile ...]) and delete it in the
existing try...after block so the test is portable and collision-free.
- Around line 319-321: The after-block restoring TEMPDIR currently uses
os:putenv("TEMPDIR", "") when OldTempdir is false, which leaves an empty-valued
env var; change the false branch to call os:unsetenv("TEMPDIR") instead so
TEMPDIR is truly removed; update the case handling around OldTempdir in the
after block (the branches referencing OldTempdir, os:putenv and TEMPDIR) to call
os:unsetenv("TEMPDIR") when OldTempdir == false and keep os:putenv("TEMPDIR",
OldTempdir) for the non-false branch.
---
Outside diff comments:
In `@src/atomvm_spectrometer.erl`:
- Line 417: Update the error tuple that currently returns {error, "No function
specified. Usage: query Module:Function[/Arity]"} to mention the accepted dot
syntax as well; change the message to include both "Module:Function" and
"Module.Function" (for example: "Usage: query Module:Function or
Module.Function[/Arity]") so the parser's supported syntax is reflected; locate
and modify the exact error string literal in the clause that returns this
{error, ...} tuple.
---
Duplicate comments:
In `@src/spectrometer_updater.erl`:
- Around line 1109-1141: The function find_elixir_exports is pushing module
atoms onto ModuleStack for defmodule/defimpl but never pops them, so nested
modules leak their scope; modify the {end_block} branch in find_elixir_exports
to detect when an end closes a module/defimpl by comparing CurrentModule to the
head of ModuleStack and, if so, pop ModuleStack and set CurrentModule to the new
head (or undefined/nil when stack empty), otherwise leave
ModuleStack/CurrentModule unchanged for non-module ends; apply the same pop
logic to the corresponding end handling used around lines 1165-1205 to ensure
ModuleStack and CurrentModule are unwound properly.
In `@test/spectrometer_otp_tests.erl`:
- Around line 258-260: The cleanup callback currently returns a nested fun
(fun(TempDir) -> fun() -> filelib:ensure_path(...) end end) so it never runs
cleanup; change the callback so it executes ensure_path directly for the TempDir
(e.g., replace the outer anonymous fun to call filelib:ensure_path/1 for
filename:join(TempDir, "noop") or invoke the inner fun immediately) so the
cleanup runs instead of returning a function.
🪄 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: 25b7423d-448e-4732-b005-a44db91446f3
📒 Files selected for processing (15)
README.mdpriv/supported_functions.datarebar.config.scriptsrc/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
- priv/supported_functions.data
- src/spectrometer_help.erl
UncleGrumpy
force-pushed
the
elixir-support
branch
from
eba9494 to
a33150c
Compare
|
Note Unit test generation is a public access feature. Expect some limitations and changes as we gather feedback and continue to improve it. Generating unit tests... This may take up to 20 minutes. |
1 similar comment
|
Note Unit test generation is a public access feature. Expect some limitations and changes as we gather feedback and continue to improve it. Generating unit tests... This may take up to 20 minutes. |
|
✅ Unit tests committed locally. Commit: |
|
Note Unit test generation is a public access feature. Expect some limitations and changes as we gather feedback and continue to improve it. Generating unit tests... This may take up to 20 minutes. |
|
✅ Created PR with unit tests: #6 |
|
✅ Created PR with unit tests: #7 |
UncleGrumpy
force-pushed
the
elixir-support
branch
from
90c7c02 to
3a069a1
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
test/spectrometer_elixir_tests.erl (1)
1-163:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRun
erlfmton this file before merge.
Check Formattingreports this file as non-compliant, so CI remains red until it is formatted.🤖 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 1 - 163, The file spectrometer_elixir_tests is failing CI due to formatting; run the Erlang formatter (erlfmt) on this module and commit the reformatted output so parse_query_8_formats_test, normalize_gpio_test, normalize_elixir_gpio_test, normalize_lists_test, normalize_with_flag_test, is_elixir_module_name_elxir_prefix_test, is_elixir_module_name_lowercase_test, and other test functions match the project's erlfmt rules; ensure consistent indentation, spacing, and line breaks (especially around lists, function clauses, and comments) and re-run the formatter until Check Formatting passes before merging.test/spectrometer_utils_tests.erl (1)
1-656:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRun
erlfmton this file before merge.This file is currently flagged by the formatting check, which will block green CI.
🤖 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_utils_tests.erl` around lines 1 - 656, The file spectrometer_utils_tests.erl (module spectrometer_utils_tests) fails the formatter check; run erlfmt over the file to fix spacing/formatting issues and commit the formatted result. Specifically, run erlfmt (or your editor/CI formatter) on spectrometer_utils_tests.erl so normalize_github_url_test_(), make_temp_dir_* tests, purge_dir_* tests, and the other test functions are consistently formatted, then add and push the resulting changes.test/spectrometer_updater_tests.erl (1)
1-1506:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRun
erlfmton this file before merge.
Check Formattingreports style violations here, so the formatting job will keep failing.🤖 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 1 - 1506, The test file spectrometer_updater_tests has formatting/style violations; run the Erlang formatter (erlfmt) on this file (module spectrometer_updater_tests) and commit the reformatted file so the Check Formatting CI step passes; ensure you format the entire file (not just hunks) and re-run the formatter locally (or via your editor/CI) before pushing the change.src/spectrometer_updater.erl (1)
349-373:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard
parse_release_branch_version/1against non-numericrelease-*names.Line 350 always sends
release-*branches toparse_release_branch_version/1, but Line 369/371 callbinary_to_integer/1without validation. Inputs likerelease-mainorrelease-nextwill throwbadargduring merge/sort.✅ Suggested hardening
branch_sort_key(<<"release-", Version/binary>>) -> - {2, parse_release_branch_version(Version)}; + case parse_release_branch_version(Version) of + {ok, V} -> {2, V}; + error -> {1, <<"release-", Version/binary>>} + end; parse_release_branch_version(Version) -> Parts = binary:split(Version, <<".">>, [global]), case Parts of - [Major, Minor | _] -> - {binary_to_integer(Major), binary_to_integer(Minor)}; - [Major] -> - {binary_to_integer(Major), 0}; + [Major, Minor | _] when is_digit_binary(Major), is_digit_binary(Minor) -> + {ok, {binary_to_integer(Major), binary_to_integer(Minor)}}; + [Major] when is_digit_binary(Major) -> + {ok, {binary_to_integer(Major), 0}}; _ -> - {0, 0} + error 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 349 - 373, The parse_release_branch_version/1 function currently calls binary_to_integer/1 on parts that may be non-numeric (e.g., "main" or "next") and can crash; update parse_release_branch_version to validate Major and Minor using the existing is_digit_binary/1 predicate before calling binary_to_integer, and when validation fails return a safe fallback (e.g., {0,0}) so branch_sort_key/1 won't raise on branches like "release-main"; ensure all pattern branches ([Major,Minor|_], [Major], _) perform digit checks where appropriate.
♻️ Duplicate comments (1)
src/spectrometer_utils.erl (1)
436-449:⚠️ Potential issue | 🟠 Major | ⚡ Quick winElixir-mode normalization still misses lowercase shorthand.
When
ElixirFlag=true, lowercase inputs (e.g.json) are left unchanged, so Elixir shorthand can fail to match entries like'Elixir.json'.Suggested fix
normalize_module_name(Str, true) when is_list(Str) -> case Str of "Elixir." ++ _ -> list_to_atom(Str); - [C | _] when C >= $A, C =< $Z -> - % Capitalized module name - treat as Elixir module - list_to_atom("Elixir." ++ Str); _ -> - % Lowercase (Erlang module) or other - keep as-is - list_to_atom(Str) + list_to_atom("Elixir." ++ Str) 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_utils.erl` around lines 436 - 449, normalize_module_name/2 currently leaves lowercase strings (e.g. "json") unchanged when ElixirFlag = true, so shorthand lookups fail; update the function (normalize_module_name) so that in the ElixirFlag=true branch the fallback case checks for lowercase identifiers (e.g. first char between $a and $z or entire string lowercase) and returns list_to_atom("Elixir." ++ Str) for those inputs, while preserving list_to_atom(Str) for true Erlang module names or cases that should remain unchanged.
🧹 Nitpick comments (1)
test/spectrometer_reporter_tests.erl (1)
56-62: ⚡ Quick winAssert the non-OTP function is removed, not just that one OTP function exists.
This test currently passes even if
{my_app,my_func,2}is still present. Add explicit negative assertions on both buckets.Proposed test tightening
Supp = maps:get(supported, Report), Unsupp = maps:get(unsupported, Report), ?assert( lists:keymember({lists, map, 2}, 1, Supp) orelse lists:keymember({lists, map, 2}, 1, Unsupp) - ). + ), + ?assertNot(lists:keymember({my_app, my_func, 2}, 1, Supp)), + ?assertNot(lists:keymember({my_app, my_func, 2}, 1, Unsupp)).🤖 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 56 - 62, The test only checks that an OTP function ({lists,map,2}) appears in either Supp or Unsupp but doesn't assert that the non-OTP function ({my_app,my_func,2}) was removed; update the assertions around Report (maps:get(supported, Report) and maps:get(unsupported, Report)) to also explicitly assert that {my_app,my_func,2} is NOT present in both Supp and Unsupp (use the same lists:keymember pattern or equivalent negative checks) so the test fails if the non-OTP entry remains while keeping the existing positive check for the OTP function.
🤖 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 319-323: The code is passing ElixirFlag=true to
spectrometer_utils:normalize_module_name for colon-form queries which causes
Elixir-style normalization; change the flag to false so Module:Function[/Arity]
uses non-Elixir normalization. Locate the calls to
spectrometer_utils:normalize_module_name(ModStr, true) in
spectrometer_atomvm.erl (the block that returns {ok, ... ,
spectrometer_utils:atom_from_string(FunStr), ...} and the similar calls around
the region mentioned) and update the second argument from true to false so
module names are normalized using the non-Elixir path.
- Around line 314-331: The parser allows empty module or function tokens (e.g.,
when ModStr or FunStr are empty) before conversion which leads to malformed
inputs getting through; in the pattern matching branch that handles [ModStr,
Rest] and the subsequent [FunStr, ArityStr] / [FunStr] cases, add explicit
non-empty checks (guards) for ModStr and FunStr before calling
spectrometer_utils:normalize_module_name/2 and
spectrometer_utils:atom_from_string/1; specifically, ensure ModStr /= "" and
FunStr /= "" (or equivalent string:length/1 > 0 guards) are validated and return
{error, "Empty module or function"} (or similar) when empty, and apply the same
guard fixes to the analogous code around the other block referenced (lines
~341-364) so empty tokens are rejected prior to normalization/atom conversion.
In `@src/spectrometer_updater.erl`:
- Around line 1114-1228: find_elixir_module_def/1 is too permissive and is
treating macro forms like unquote/name as module names; tighten the regexes in
find_elixir_module_def (used in the top-level case that drives module stack
updates in find_elixir_exports) to only match actual "defmodule ... do" and
"defimpl ... do" blocks (require the trailing "do" or opening parenthesis/brace
context and anchor whitespace properly) and reject forms with "unquote" or
non-identifier tokens; update the three patterns (defmodule, defimpl with for:,
defimpl fallback) to include the "do" delimiter (or require a word boundary and
not a following dot/unquote) so only real module/impl declarations produce
{defmodule, ModName} / {defimpl, ...} and leave other lines returning error.
In `@test/spectrometer_updater_tests.erl`:
- Around line 1331-1361: The tests fail because the module name shape in the Acc
map produced by spectrometer_updater:scan_atomvm_repo doesn't match the test
fixtures (fixtures use defmodule Elixir.MyElixirMod while Acc contains a
different-normalized name); update spectrometer_updater:scan_atomvm_repo so when
constructing keys for functions it normalizes module names consistently (e.g.,
ensure module_name string either always includes the "Elixir." prefix or is
always stripped) before inserting into Acc, or alternatively update the test
fixture (my_elixir_mod.ex) to match the codepath normalization; locate the key
construction code that produces entries like {'Elixir.MyElixirMod',
exported_func, 2} and change it to prepend "Elixir." if missing (or remove the
prefix consistently) so the maps:is_key checks in the tests pass.
---
Outside diff comments:
In `@src/spectrometer_updater.erl`:
- Around line 349-373: The parse_release_branch_version/1 function currently
calls binary_to_integer/1 on parts that may be non-numeric (e.g., "main" or
"next") and can crash; update parse_release_branch_version to validate Major and
Minor using the existing is_digit_binary/1 predicate before calling
binary_to_integer, and when validation fails return a safe fallback (e.g.,
{0,0}) so branch_sort_key/1 won't raise on branches like "release-main"; ensure
all pattern branches ([Major,Minor|_], [Major], _) perform digit checks where
appropriate.
In `@test/spectrometer_elixir_tests.erl`:
- Around line 1-163: The file spectrometer_elixir_tests is failing CI due to
formatting; run the Erlang formatter (erlfmt) on this module and commit the
reformatted output so parse_query_8_formats_test, normalize_gpio_test,
normalize_elixir_gpio_test, normalize_lists_test, normalize_with_flag_test,
is_elixir_module_name_elxir_prefix_test, is_elixir_module_name_lowercase_test,
and other test functions match the project's erlfmt rules; ensure consistent
indentation, spacing, and line breaks (especially around lists, function
clauses, and comments) and re-run the formatter until Check Formatting passes
before merging.
In `@test/spectrometer_updater_tests.erl`:
- Around line 1-1506: The test file spectrometer_updater_tests has
formatting/style violations; run the Erlang formatter (erlfmt) on this file
(module spectrometer_updater_tests) and commit the reformatted file so the Check
Formatting CI step passes; ensure you format the entire file (not just hunks)
and re-run the formatter locally (or via your editor/CI) before pushing the
change.
In `@test/spectrometer_utils_tests.erl`:
- Around line 1-656: The file spectrometer_utils_tests.erl (module
spectrometer_utils_tests) fails the formatter check; run erlfmt over the file to
fix spacing/formatting issues and commit the formatted result. Specifically, run
erlfmt (or your editor/CI formatter) on spectrometer_utils_tests.erl so
normalize_github_url_test_(), make_temp_dir_* tests, purge_dir_* tests, and the
other test functions are consistently formatted, then add and push the resulting
changes.
---
Duplicate comments:
In `@src/spectrometer_utils.erl`:
- Around line 436-449: normalize_module_name/2 currently leaves lowercase
strings (e.g. "json") unchanged when ElixirFlag = true, so shorthand lookups
fail; update the function (normalize_module_name) so that in the ElixirFlag=true
branch the fallback case checks for lowercase identifiers (e.g. first char
between $a and $z or entire string lowercase) and returns list_to_atom("Elixir."
++ Str) for those inputs, while preserving list_to_atom(Str) for true Erlang
module names or cases that should remain unchanged.
---
Nitpick comments:
In `@test/spectrometer_reporter_tests.erl`:
- Around line 56-62: The test only checks that an OTP function ({lists,map,2})
appears in either Supp or Unsupp but doesn't assert that the non-OTP function
({my_app,my_func,2}) was removed; update the assertions around Report
(maps:get(supported, Report) and maps:get(unsupported, Report)) to also
explicitly assert that {my_app,my_func,2} is NOT present in both Supp and Unsupp
(use the same lists:keymember pattern or equivalent negative checks) so the test
fails if the non-OTP entry remains while keeping the existing positive check for
the OTP function.
🪄 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: f8c1db5a-6e07-430c-a1e5-409d8a981051
📒 Files selected for processing (15)
README.mdpriv/supported_functions.datarebar.config.scriptsrc/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 (1)
- src/spectrometer_help.erl
Streamline the escript and reduce function call overhead. Signed-off-by: Winford <winford@object.stream>
UncleGrumpy
force-pushed
the
elixir-support
branch
from
3a069a1 to
2b2fd54
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
test/spectrometer_utils_tests.erl (1)
410-427:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAvoid the fixed
/tmpfallback in this test.This still hardcodes
/tmpwhenTMPDIRis unset, so the test is not portable and can diverge from the module's own temp-dir resolution. Usemake_temp_dir/1orsystem_temp_dir/0here instead.♻️ Proposed fix
- TempDir = os:getenv("TMPDIR"), - UniqueSuffix = erlang:unique_integer([positive]), - MarkerFile = - case TempDir of - false -> - filename:join( - "/tmp", - "spectrometer_utils_test_marker_" ++ - integer_to_list(UniqueSuffix) - ); - _ -> - filename:join( - TempDir, - "spectrometer_utils_test_marker_" ++ - integer_to_list(UniqueSuffix) - ) - end, - file:write_file(MarkerFile, ""), + TempDir = spectrometer_utils:make_temp_dir("find_first_file_"), + MarkerFile = filename:join(TempDir, "marker"), + ok = file:write_file(MarkerFile, ""), try Result = spectrometer_utils:find_first_file([ "/nonexistent/file1", MarkerFile, "/nonexistent/file2" ]), ?assertEqual(MarkerFile, Result) after - file:delete(MarkerFile) + file:delete(MarkerFile), + spectrometer_utils:purge_dir(TempDir) 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 `@test/spectrometer_utils_tests.erl` around lines 410 - 427, The test currently falls back to the hardcoded "/tmp" when TMPDIR is unset (see TempDir and MarkerFile), making it non-portable; change the fallback to use the project's temp utilities by calling system_temp_dir()/0 to obtain the system temp directory or preferably use make_temp_dir/1 to create a unique temp directory and then build the MarkerFile inside it, ensuring you replace the "/tmp" literal with those helpers and update any cleanup to remove the created directory/file.
🧹 Nitpick comments (1)
src/spectrometer_utils.erl (1)
436-438: ⚡ Quick winAlign the
normalize_module_name/2spec with the exported API.The implementation accepts atoms here, and the new tests rely on that, but the spec only allows
string(). That will produce misleading Dialyzer results for valid callers.♻️ Proposed fix
--spec normalize_module_name(string(), boolean()) -> atom(). +-spec normalize_module_name(string() | atom(), boolean()) -> atom(). normalize_module_name(Atom, ElixirFlag) when is_atom(Atom) -> normalize_module_name(atom_to_list(Atom), ElixirFlag);🤖 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_utils.erl` around lines 436 - 438, The spec for normalize_module_name/2 is too narrow: update the -spec to reflect that the function accepts atoms as well as strings (lists), e.g. change it to -spec normalize_module_name(string() | atom(), boolean()) -> atom() so Dialyzer matches the implementation which handles atoms (see normalize_module_name/2 and the clause calling atom_to_list/1).
🤖 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_updater.erl.patched`:
- Around line 1220-1245: The bug is that find_elixir_exports only pops
ModuleStack when the raw line matches "end" ++ _, so indented ends (closing
nested defmodule/defimpl) never pop and inner modules remain current; fix by
tracking indentation for module entries and popping based on indentation: change
ModuleStack entries to tuples like {ModuleName, Indent} when pushing a
module/impl, compute the current line's leading-space indentation for any line
matching /^\s*end\b/ and when an end is seen pop while the top entry's Indent >=
end Indent (updating CurrentModule from the new top or undefined), and update
all push/pop sites in find_elixir_exports to use the tuple form.
- Around line 163-190: The code currently assumes file:consult(OutputFile)
returns {ok, [Data]} and will crash on other {ok, ...} shapes; modify the
ExistingDB case in update/1 so that the file:consult(OutputFile) clause handles
other successful parses explicitly (e.g., add a clause matching {ok, _Other}
that logs a warning about a malformed/corrupted cached file referencing
OutputFile and then returns a safe fallback like #{} or a controlled {error,
malformed_cache} as appropriate), leaving existing behavior for {error, enoent}
and {error, Reason} unchanged; reference the file:consult(OutputFile) match, the
ExistingDB binding, and build_db_from_list when implementing the new branch.
In `@test/spectrometer_otp_tests.erl`:
- Around line 258-260: The teardown for
modules_list_handles_missing_cache_dir_test_ currently recreates the temp path
using filelib:ensure_path/1 which leaks temp directories; replace the cleanup
fun(TempDir) -> filelib:ensure_path(filename:join(TempDir, "noop")) end with a
call that removes the temp directory and its contents (e.g.
filelib:delete_dir(TempDir) or an equivalent recursive delete) so the temp dir
is removed after the test instead of being recreated.
---
Duplicate comments:
In `@test/spectrometer_utils_tests.erl`:
- Around line 410-427: The test currently falls back to the hardcoded "/tmp"
when TMPDIR is unset (see TempDir and MarkerFile), making it non-portable;
change the fallback to use the project's temp utilities by calling
system_temp_dir()/0 to obtain the system temp directory or preferably use
make_temp_dir/1 to create a unique temp directory and then build the MarkerFile
inside it, ensuring you replace the "/tmp" literal with those helpers and update
any cleanup to remove the created directory/file.
---
Nitpick comments:
In `@src/spectrometer_utils.erl`:
- Around line 436-438: The spec for normalize_module_name/2 is too narrow:
update the -spec to reflect that the function accepts atoms as well as strings
(lists), e.g. change it to -spec normalize_module_name(string() | atom(),
boolean()) -> atom() so Dialyzer matches the implementation which handles atoms
(see normalize_module_name/2 and the clause calling atom_to_list/1).
🪄 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: 3c4cd3b3-9d91-43c3-bd77-d34319f15e8c
📒 Files selected for processing (16)
README.mdpriv/supported_functions.datarebar.config.scriptsrc/atomvm_spectrometer.erlsrc/spectrometer_atomvm.erlsrc/spectrometer_help.erlsrc/spectrometer_updater.erlsrc/spectrometer_updater.erl.patchedsrc/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
- priv/supported_functions.data
Comment on lines
+163
to
+190
| ExistingDB = | ||
| case file:consult(OutputFile) of | ||
| {ok, [Data]} when is_list(Data) -> | ||
| io:format("Loading existing data set from ~s\n", [OutputFile]), | ||
| build_db_from_list(Data); | ||
| {error, enoent} -> | ||
| % If no user cache exists, try to load from bundled data for initial values | ||
| Datafile = spectrometer_utils:bundled_data_path(), | ||
| case file:consult(Datafile) of | ||
| {ok, [Data]} when is_list(Data) -> | ||
| io:format( | ||
| "Loading bundled data set from ~s\n", [Datafile] | ||
| ), | ||
| build_db_from_list(Data); | ||
| {ok, [_]} -> | ||
| % Bundled file exists but has invalid structure | ||
| #{}; | ||
| {error, enoent} -> | ||
| io:format( | ||
| "No existing data found, starting with empty data set\n" | ||
| ), | ||
| #{}; | ||
| {error, Reason} -> | ||
| {error, Reason} | ||
| end; | ||
| {error, Reason} -> | ||
| {error, Reason} | ||
| end, |
There was a problem hiding this comment.
Handle malformed existing output files explicitly.
This file:consult(OutputFile) branch only accepts {ok, [Data]} when Data is a list. Any other successful parse result crashes with case_clause, so a corrupted user cache aborts update/1 instead of falling back or returning a normal error.
🛠️ Minimal fix
ExistingDB =
case file:consult(OutputFile) of
{ok, [Data]} when is_list(Data) ->
io:format("Loading existing data set from ~s\n", [OutputFile]),
build_db_from_list(Data);
+ {ok, _Invalid} ->
+ io:format(
+ "Ignoring invalid data set in ~s, starting from fallback data\n",
+ [OutputFile]
+ ),
+ #{};
{error, enoent} ->
% If no user cache exists, try to load from bundled data for initial values
Datafile = spectrometer_utils:bundled_data_path(),🤖 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.patched` around lines 163 - 190, The code
currently assumes file:consult(OutputFile) returns {ok, [Data]} and will crash
on other {ok, ...} shapes; modify the ExistingDB case in update/1 so that the
file:consult(OutputFile) clause handles other successful parses explicitly
(e.g., add a clause matching {ok, _Other} that logs a warning about a
malformed/corrupted cached file referencing OutputFile and then returns a safe
fallback like #{} or a controlled {error, malformed_cache} as appropriate),
leaving existing behavior for {error, enoent} and {error, Reason} unchanged;
reference the file:consult(OutputFile) match, the ExistingDB binding, and
build_db_from_list when implementing the new branch.
2b2fd54 to
eb7919c
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/spectrometer_updater.erl (1)
1231-1255:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftModule stack pop logic misclassifies indented module-closing
endLine 1234 currently treats only column-0
endas module close. In nested Elixir modules, the closingendis commonly indented, so the stack is not popped and later defs can be attributed to the wrong module.Suggested direction
- {end_block} -> - % Check if this end is at column 0 (no indentation) to detect - % module/defimpl closing vs function/clause closing - case Line of - "end" ++ _ -> - % No indentation - this closes a module/impl, pop the stack + {end_block} -> + % Pop module scope based on tracked module block depth/indentation, + % not "column 0" heuristic. + case should_pop_module_scope(Line, ModuleStack) of + true -> case ModuleStack of [CurrentModule | RestStack] -> ... - _ -> - % Indented end - closes function/clause, not module + false -> find_elixir_exports(Rest, ModuleStack, CurrentModule, Acc) end;A minimal robust fix is to track module block indentation/depth when pushing module contexts and pop on matching
endregardless of absolute column.🤖 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 1231 - 1255, The current logic in find_elixir_exports relies on detecting "end" only at column 0 (Line matching "end" ++ _) which misses indented module/defimpl closings; change the module stack to track each pushed module's indentation (e.g., store {ModuleName, Indent} or a depth marker in ModuleStack when you push a module/defimpl) and on any line that is an "end" (regardless of leading spaces) pop the stack when that end matches the most recent module/defimpl context; update find_elixir_exports to use ModuleStack entries with indentation/depth and to pop the top entry on matching end, and also update the code paths that push module contexts so they push the indentation/depth alongside the module name (references: function find_elixir_exports, ModuleStack, CurrentModule).
🧹 Nitpick comments (1)
test/spectrometer_utils_tests.erl (1)
293-303: 💤 Low valueRedundant test case.
run_git_command_empty_env_test_is identical torun_git_command_happy_path_test_—both call["--version"], []. Consider testing a distinct scenario (e.g., with a custom env var like[{"GIT_PAGER", "cat"}]) or removing the duplicate.🤖 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_utils_tests.erl` around lines 293 - 303, The test run_git_command_empty_env_test_ is a duplicate of run_git_command_happy_path_test_ (both call spectrometer_utils:run_git_command(["--version"], [])); either remove this redundant test or change it to exercise a distinct scenario—e.g., call spectrometer_utils:run_git_command(["--version"], [{"GIT_PAGER","cat"}]) to verify custom env propagation or another relevant env case—so the test suite covers different behaviors instead of repeating the same assertion.
🤖 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/atomvm_spectrometer.erl`:
- Around line 417-418: Update the usage string returned in the error tuple
{error, "No function specified. Usage: query Module:Function or
Module.Function[/Arity]"} to mention the /Arity form for the colon syntax as
well (e.g. "Usage: query Module:Function/Arity or Module.Function[/Arity]") so
the message correctly documents both Module:Function/Arity and
Module.Function[/Arity]; locate and modify the string literal in
atomvm_spectrometer.erl where that tuple is constructed and return the corrected
message.
In `@test/spectrometer_otp_tests.erl`:
- Around line 96-101: The fixture sets the global app env via
application:set_env(spectrometer, cache_dir, TempDir) but teardown only purges
the TempDir, leaving spectrometer cache_dir mutated; modify each fixture (the
ones around TempDir, and similar blocks at lines noted) to capture the previous
value with application:get_env(spectrometer, cache_dir) before setting the
TempDir, then in the teardown fun restore the original value (or unset it if it
was undefined) after calling spectrometer_utils:purge_dir(TempDir) so that
spectrometer cache_dir is returned to its prior state and tests do not leak
global env between fixtures.
---
Duplicate comments:
In `@src/spectrometer_updater.erl`:
- Around line 1231-1255: The current logic in find_elixir_exports relies on
detecting "end" only at column 0 (Line matching "end" ++ _) which misses
indented module/defimpl closings; change the module stack to track each pushed
module's indentation (e.g., store {ModuleName, Indent} or a depth marker in
ModuleStack when you push a module/defimpl) and on any line that is an "end"
(regardless of leading spaces) pop the stack when that end matches the most
recent module/defimpl context; update find_elixir_exports to use ModuleStack
entries with indentation/depth and to pop the top entry on matching end, and
also update the code paths that push module contexts so they push the
indentation/depth alongside the module name (references: function
find_elixir_exports, ModuleStack, CurrentModule).
---
Nitpick comments:
In `@test/spectrometer_utils_tests.erl`:
- Around line 293-303: The test run_git_command_empty_env_test_ is a duplicate
of run_git_command_happy_path_test_ (both call
spectrometer_utils:run_git_command(["--version"], [])); either remove this
redundant test or change it to exercise a distinct scenario—e.g., call
spectrometer_utils:run_git_command(["--version"], [{"GIT_PAGER","cat"}]) to
verify custom env propagation or another relevant env case—so the test suite
covers different behaviors instead of repeating the same assertion.
🪄 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: 72bd5d3a-7bba-49ec-8fbf-ad4017392691
📒 Files selected for processing (15)
README.mdpriv/supported_functions.datarebar.config.scriptsrc/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
- test/atomvm_spectrometer_tests.erl
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>
UncleGrumpy
force-pushed
the
elixir-support
branch
from
eb7919c to
db9dc09
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/spectrometer_updater.erl (1)
1290-1333:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRequire real Elixir aliases in
defmodule/defimplmatching.These patterns still accept lowercase/non-alias tokens, and the generated DB already shows the fallout (
Elixir.json,Elixir.name,Elixir.unquote, plus{unknown, ...}). Tighten the captures to uppercase-leading aliases so only real Elixir module keys are emitted.#!/bin/bash # Show the polluted generated entries and the permissive matcher that allows them. rg -n "Elixir\\.(json|name|unquote)|\\{unknown," priv/supported_functions.data awk 'NR>=1290 && NR<=1333 {print NR ":" $0}' src/spectrometer_updater.erlProposed tightening
- "^\\s*defmodule\\s+([A-Za-z_][A-Za-z0-9_.]*)\\s+do\\b", + "^\\s*defmodule\\s+([A-Z][A-Za-z0-9_.]*)\\s+do\\b", @@ - "^\\s*defimpl\\s+([A-Za-z_][A-Za-z0-9_.]*),\\s*for:\\s*([A-Za-z_][A-Za-z0-9_.]*)\\s+do\\b", + "^\\s*defimpl\\s+([A-Z][A-Za-z0-9_.]*),\\s*for:\\s*([A-Z][A-Za-z0-9_.]*)\\s+do\\b", @@ - "^\\s*defimpl\\s+([A-Za-z_][A-Za-z0-9_.]*)\\s+do\\b", + "^\\s*defimpl\\s+([A-Z][A-Za-z0-9_.]*)\\s+do\\b",🤖 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 1290 - 1333, The current find_elixir_module_def function accepts lower-case or non-alias tokens; update the regex captures for defmodule and defimpl (both the primary name and the "for" target) so they require an uppercase-leading alias (e.g. change the identifier class to require first character [A-Z] instead of [A-Za-z_], and keep the rest as [A-Za-z0-9_.]*), and apply the same tightened rule to the fallback defimpl capture (ImplName) so only real Elixir module keys are emitted; modify the three regexes inside find_elixir_module_def that produce ModName, ProtocolName/TargetName, and ImplName accordingly.
🤖 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 478-481: The branch that dispatches when #{module := Mod} calls
print_supported(Mod, Filter) but the two-argument variant ignores Filter, so
module-based reporting bypasses the language filter; update the implementation
of print_supported/2 (the clause handling a Mod and a Filter) to apply the same
filtering logic used by print_supported/1 (or the single-arg path), i.e., ensure
the Filter (erl/ex) is checked when building the list of supported items for Mod
and only those matching the Filter are printed; also mirror this change for the
similar case around the other occurrence noted (lines referenced near 497-498)
so both module-path branches honor the Filter.
In `@src/spectrometer_utils.erl`:
- Around line 430-456: normalize_module_name/1,2 currently converts arbitrary
user-controlled strings into atoms via atom_from_string/1 (which falls back to
list_to_atom/1), creating a permanent atom DoS risk; change
normalize_module_name and atom_from_string so they do NOT call list_to_atom/1 on
unknown strings—use list_to_existing_atom/1 and handle the miss (return an
{error,invalid_module} or a safe non-atom representation) or validate/whitelist
the module name against a bounded set before converting; update callers in
atomvm_spectrometer and spectrometer_atomvm to handle the error/safe result
accordingly so no unbounded atoms are created.
---
Duplicate comments:
In `@src/spectrometer_updater.erl`:
- Around line 1290-1333: The current find_elixir_module_def function accepts
lower-case or non-alias tokens; update the regex captures for defmodule and
defimpl (both the primary name and the "for" target) so they require an
uppercase-leading alias (e.g. change the identifier class to require first
character [A-Z] instead of [A-Za-z_], and keep the rest as [A-Za-z0-9_.]*), and
apply the same tightened rule to the fallback defimpl capture (ImplName) so only
real Elixir module keys are emitted; modify the three regexes inside
find_elixir_module_def that produce ModName, ProtocolName/TargetName, and
ImplName accordingly.
🪄 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: b3ddb45d-6d4a-4a8b-91c3-2939ee772c2e
📒 Files selected for processing (15)
README.mdpriv/supported_functions.datarebar.config.scriptsrc/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
Comment on lines
478
to
+481
| #{module := Mod} -> | ||
| print_supported(Mod); | ||
| print_supported(Mod, Filter); | ||
| #{} -> | ||
| print_supported() | ||
| print_supported(Filter) |
There was a problem hiding this comment.
--module path currently bypasses language filter behavior.
When module is present, report_supported/1 calls print_supported/2, but print_supported(Mod, _Filter) ignores the filter. supported --module ... --erl/--ex therefore does not honor filtering.
Suggested minimal fix
report_supported(Opts) ->
@@
case Opts of
#{module := Mod} ->
- print_supported(Mod, Filter);
+ case filter_modules_by_type([Mod], Filter) of
+ [Mod] -> print_supported(Mod, Filter);
+ [] ->
+ io:format(
+ standard_error,
+ "Module ~ts excluded by selected filter\n",
+ [atom_to_list(Mod)]
+ ),
+ {error, unsupported}
+ end;
#{} ->
print_supported(Filter)
end.Also applies to: 497-498
🤖 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 478 - 481, The branch that
dispatches when #{module := Mod} calls print_supported(Mod, Filter) but the
two-argument variant ignores Filter, so module-based reporting bypasses the
language filter; update the implementation of print_supported/2 (the clause
handling a Mod and a Filter) to apply the same filtering logic used by
print_supported/1 (or the single-arg path), i.e., ensure the Filter (erl/ex) is
checked when building the list of supported items for Mod and only those
matching the Filter are printed; also mirror this change for the similar case
around the other occurrence noted (lines referenced near 497-498) so both
module-path branches honor the Filter.
Comment on lines
+430
to
+456
| -spec normalize_module_name(string() | atom()) -> atom(). | ||
| normalize_module_name(Atom) when is_atom(Atom) -> | ||
| normalize_module_name(atom_to_list(Atom)); | ||
| normalize_module_name(Str) when is_list(Str) -> | ||
| normalize_module_name(Str, false). | ||
|
|
||
| -spec normalize_module_name(string() | atom(), boolean()) -> atom(). | ||
| normalize_module_name(Atom, ElixirFlag) when is_atom(Atom) -> | ||
| normalize_module_name(atom_to_list(Atom), ElixirFlag); | ||
| normalize_module_name(Str, true) when is_list(Str) -> | ||
| case Str of | ||
| "Elixir." ++ _ -> | ||
| atom_from_string(Str); | ||
| [C | _] when C >= $A, C =< $Z -> | ||
| % Capitalized module name - treat as Elixir module | ||
| atom_from_string("Elixir." ++ Str); | ||
| _ -> | ||
| % Lowercase (Erlang module) or other - keep as-is | ||
| atom_from_string(Str) | ||
| end; | ||
| normalize_module_name(Str, false) when is_list(Str) -> | ||
| case Str of | ||
| "Elixir." ++ _ -> | ||
| atom_from_string(Str); | ||
| _ -> | ||
| atom_from_string(Str) | ||
| end. |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the new helpers are used from user-input parsing paths and still create atoms on misses.
rg -n "normalize_module_name\(" src test
awk 'NR>=41 && NR<=51 {print NR ":" $0} NR>=430 && NR<=456 {print NR ":" $0}' src/spectrometer_utils.erlRepository: UncleGrumpy/atomvm_spectrometer
Length of output: 4235
Avoid converting user-controlled module/query strings into atoms (permanent atom DoS risk)
src/spectrometer_utils.erl’s atom_from_string/1 uses list_to_existing_atom/1 but falls back to list_to_atom/1 on misses, and normalize_module_name/1,2 funnel all string inputs through it (still returning atom()). Those helpers are invoked from user-facing parsing paths in src/atomvm_spectrometer.erl (--module/-m) and src/spectrometer_atomvm.erl (module parsing in queries), so unknown inputs can create unbounded permanent atoms.
Update the normalization to avoid list_to_atom/1 for user input (e.g., only allow existing atoms via list_to_existing_atom/1 and handle misses, or resolve/whitelist module names against a bounded set before converting).
🤖 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_utils.erl` around lines 430 - 456, normalize_module_name/1,2
currently converts arbitrary user-controlled strings into atoms via
atom_from_string/1 (which falls back to list_to_atom/1), creating a permanent
atom DoS risk; change normalize_module_name and atom_from_string so they do NOT
call list_to_atom/1 on unknown strings—use list_to_existing_atom/1 and handle
the miss (return an {error,invalid_module} or a safe non-atom representation) or
validate/whitelist the module name against a bounded set before converting;
update callers in atomvm_spectrometer and spectrometer_atomvm to handle the
error/safe result accordingly so no unbounded atoms are created.
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
1 participant
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.
Add support for Elixir MFA in update, support, and query commands.
Expand overall test coverage.