Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull request overview
This PR introduces an auto-discovery system for Amber language keywords (via a new AutoKeyword derive + inventory registry) and adds a CLI subcommand to generate a grammar.ebnf file from a partially hardcoded EBNF template plus build-time-generated keyword/builtin sections.
Changes:
- Add
amber_meta::AutoKeywordderive macro and a runtime keyword registry (modules::keywords) backed byinventory. - Introduce
utils::grammar_ebnf::generate_grammar_ebnf()and aGrammarEbnfCLI subcommand to emitgrammar.ebnf. - Annotate many syntax modules (statements/builtins/operators) with
#[derive(AutoKeyword)]and addinventorydependencies.
Reviewed changes
Copilot reviewed 60 out of 62 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| src/utils/mod.rs | Exposes the new grammar_ebnf utility module. |
| src/utils/grammar_ebnf.rs | Adds EBNF template + generator that injects discovered keywords/builtins. |
| src/tests/testing.rs | Removes trailing whitespace line. |
| src/modules/variable/mod.rs | Registers several statement keywords via AutoKeyword. |
| src/modules/variable/init.rs | Registers let/const keywords via AutoKeyword. |
| src/modules/test.rs | Registers test keyword via AutoKeyword. |
| src/modules/prelude.rs | Re-exports AutoKeyword for convenient use across modules. |
| src/modules/mod.rs | Adds the new modules::keywords module. |
| src/modules/main.rs | Registers main keyword via AutoKeyword. |
| src/modules/loops/while_loop.rs | Registers while keyword via AutoKeyword. |
| src/modules/loops/iter_loop.rs | Registers for keyword via AutoKeyword. |
| src/modules/loops/infinite_loop.rs | Registers loop keyword via AutoKeyword. |
| src/modules/loops/continue_stmt.rs | Registers continue keyword via AutoKeyword. |
| src/modules/loops/break_stmt.rs | Registers break keyword via AutoKeyword. |
| src/modules/keywords.rs | Introduces KeywordKind, KeywordRegistration, and iter_keywords() using inventory. |
| src/modules/imports/import.rs | Registers import/pub keywords via AutoKeyword. |
| src/modules/function/ret.rs | Registers return keyword via AutoKeyword. |
| src/modules/function/fail.rs | Registers fail keyword via AutoKeyword. |
| src/modules/function/declaration.rs | Registers ref/fun keywords via AutoKeyword. |
| src/modules/expression/unop/not.rs | Registers not keyword via AutoKeyword. |
| src/modules/expression/literal/status.rs | Registers status keyword via AutoKeyword. |
| src/modules/expression/binop/add.rs | Registers a binary-op keyword entry for the add operator module. |
| src/modules/expression/binop/and.rs | Registers and as a binary-op keyword. |
| src/modules/expression/binop/div.rs | Registers a binary-op keyword entry for the div operator module. |
| src/modules/expression/binop/eq.rs | Registers a binary-op keyword entry for the eq operator module. |
| src/modules/expression/binop/ge.rs | Registers a binary-op keyword entry for the ge operator module. |
| src/modules/expression/binop/gt.rs | Registers a binary-op keyword entry for the gt operator module. |
| src/modules/expression/binop/le.rs | Registers a binary-op keyword entry for the le operator module. |
| src/modules/expression/binop/lt.rs | Registers a binary-op keyword entry for the lt operator module. |
| src/modules/expression/binop/modulo.rs | Registers a binary-op keyword entry for the modulo operator module. |
| src/modules/expression/binop/mul.rs | Registers a binary-op keyword entry for the mul operator module. |
| src/modules/expression/binop/neq.rs | Registers a binary-op keyword entry for the neq operator module. |
| src/modules/expression/binop/or.rs | Registers or as a binary-op keyword. |
| src/modules/expression/binop/range.rs | Registers a binary-op keyword entry for the range operator module. |
| src/modules/expression/binop/sub.rs | Registers a binary-op keyword entry for the sub operator module. |
| src/modules/condition/ifcond.rs | Registers if via AutoKeyword. |
| src/modules/condition/ifchain.rs | Registers if via AutoKeyword and reshuffles DocumentationModule impl placement. |
| src/modules/condition/failure_handler.rs | Registers failed/succeeded/exited keywords via AutoKeyword. |
| src/modules/command/modifier.rs | Registers command modifier keywords (sudo, silent, etc.) via AutoKeyword. |
| src/modules/builtin/wait.rs | Registers await builtin via AutoKeyword. |
| src/modules/builtin/touch.rs | Registers touch builtin via AutoKeyword. |
| src/modules/builtin/sleep.rs | Registers sleep builtin via AutoKeyword. |
| src/modules/builtin/rm.rs | Registers rm builtin via AutoKeyword. |
| src/modules/builtin/pwd.rs | Registers pwd builtin via AutoKeyword. |
| src/modules/builtin/pid.rs | Registers pid builtin via AutoKeyword. |
| src/modules/builtin/nameof.rs | Registers nameof builtin via AutoKeyword. |
| src/modules/builtin/mv.rs | Registers mv builtin via AutoKeyword. |
| src/modules/builtin/ls.rs | Registers ls builtin via AutoKeyword. |
| src/modules/builtin/lines.rs | Registers lines builtin via AutoKeyword. |
| src/modules/builtin/len.rs | Registers len builtin via AutoKeyword. |
| src/modules/builtin/exit.rs | Registers exit builtin via AutoKeyword. |
| src/modules/builtin/echo.rs | Registers echo builtin via AutoKeyword. |
| src/modules/builtin/disown.rs | Registers disown builtin via AutoKeyword. |
| src/modules/builtin/cp.rs | Registers cp builtin via AutoKeyword. |
| src/modules/builtin/clear.rs | Registers clear builtin via AutoKeyword. |
| src/modules/builtin/cd.rs | Registers cd builtin via AutoKeyword. |
| src/main.rs | Adds a GrammarEbnf CLI subcommand that writes grammar.ebnf. |
| meta/src/lib.rs | Extends proc-macro crate: refactors Context macros and adds AutoKeyword derive (inventory submit). |
| meta/src/dispatch.rs | Marks dispatch generator hidden in docs. |
| meta/Cargo.toml | Adds inventory dependency to proc-macro crate. |
| Cargo.toml | Adds inventory dependency to main crate. |
| Cargo.lock | Locks inventory and its transitive deps. |
| .gitignore | Ignores .sisyphus. |
Comments suppressed due to low confidence (1)
src/modules/expression/binop/add.rs:38
- These
BinOpimplementations register keywords like"add", but the actual parsed operator token is"+"(parse_operator()callstoken(meta, "+")). This will put non-existent user-facing keywords into the keyword registry (and potentially into generated artifacts if filtering changes). Either avoidAutoKeywordon symbolic operators, or register the real operator lexeme in a way the downstream generator can handle (likely separate fromKEYWORD_*).
#[derive(Debug, Clone, AutoKeyword)]
#[keyword = "add"]
#[kind = "binary_op"]
pub struct Add {
left: Box<Expr>,
right: Box<Expr>,
kind: Type,
}
impl Typed for Add {
fn get_type(&self) -> Type {
self.kind.clone()
}
}
impl BinOp for Add {
fn set_left(&mut self, left: Expr) {
*self.left = left;
}
fn set_right(&mut self, right: Expr) {
*self.right = right;
}
fn parse_operator(&mut self, meta: &mut ParserMetadata) -> SyntaxResult {
token(meta, "+")?;
Ok(())
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Collect unique keywords using HashSet to deduplicate | ||
| let mut keyword_defs = String::new(); | ||
| let mut keyword_list: Vec<_> = keywords.into_iter().collect(); | ||
| // Sort for deterministic output | ||
| keyword_list.sort(); | ||
| for kw in keyword_list { | ||
| keyword_defs.push_str(&format!("KEYWORD_{} = '{}' ;\n", kw.to_uppercase(), kw)); | ||
| } | ||
| keyword_defs.push('\n'); | ||
|
|
||
| // Generate individual builtin rules for statement builtins | ||
| let mut builtin_stmt_rules = String::new(); | ||
| for kw in &builtin_stmt_keywords { | ||
| let kw_upper = kw.to_uppercase(); | ||
| let builtin_name = format!("builtin_{}", kw); | ||
| match kw.as_ref() { | ||
| "clear" | "pid" => { | ||
| builtin_stmt_rules.push_str(&format!("{} = KEYWORD_{} ;\n", builtin_name, kw_upper)) | ||
| } | ||
| _ => builtin_stmt_rules.push_str(&format!( | ||
| "{} = KEYWORD_{}, expression ;\n", | ||
| builtin_name, kw_upper | ||
| )), | ||
| } | ||
| } | ||
| builtin_stmt_rules.push('\n'); | ||
|
|
||
| // Generate individual builtin rules for expression builtins | ||
| let mut builtin_expr_rules = String::new(); | ||
| for kw in &builtin_expr_keywords { | ||
| let kw_upper = kw.to_uppercase(); | ||
| let builtin_name = format!("builtin_{}", kw); | ||
| match kw.as_ref() { | ||
| "pid" => { | ||
| builtin_expr_rules.push_str(&format!("{} = KEYWORD_{} ;\n", builtin_name, kw_upper)) | ||
| } | ||
| _ => builtin_expr_rules.push_str(&format!( | ||
| "{} = KEYWORD_{}, expression ;\n", | ||
| builtin_name, kw_upper | ||
| )), | ||
| } | ||
| } | ||
| builtin_expr_rules.push('\n'); | ||
|
|
||
| // Generate builtins_statement rule | ||
| let builtins_stmt_rule = if builtin_stmt_keywords.is_empty() { | ||
| String::new() | ||
| } else { | ||
| let alternatives: Vec<String> = builtin_stmt_keywords | ||
| .iter() | ||
| .map(|kw| format!("builtin_{}", kw)) | ||
| .collect(); | ||
| format!("builtins_statement = {} ;\n\n", alternatives.join(" | ")) | ||
| }; | ||
|
|
||
| // Generate builtins_expression rule | ||
| let builtins_expr_rule = if builtin_expr_keywords.is_empty() { | ||
| String::new() | ||
| } else { | ||
| let alternatives: Vec<String> = builtin_expr_keywords | ||
| .iter() |
There was a problem hiding this comment.
The generated builtin rules assume a uniform KEYWORD_X, expression shape (and special-case clear/pid), but several builtins have different concrete syntax in the parser: e.g. cp(...) takes 2–3 comma-separated args and requires a failure handler; rm(...) takes 1–3 args; pwd()/pid() require empty parentheses; ls(...) has 0–3 args + failure handler. As written, generate_grammar_ebnf() will generate rules that don't match the language. Consider encoding per-builtin signatures (args/parentheses/handler) in the registry (extra derive attributes) or generate these rules directly from the parsing definitions instead of hardcoding KEYWORD_X, expression.
src/main.rs
Outdated
| CommandKind::GrammarEbnf => { | ||
| let output = grammar_ebnf::generate_grammar_ebnf(); | ||
| let output_path = PathBuf::from("grammar.ebnf"); | ||
| std::fs::write(&output_path, output).expect("Failed to write grammar.ebnf"); |
There was a problem hiding this comment.
GrammarEbnf uses std::fs::write(...).expect(...), which will panic instead of returning an error through main()’s Result. Prefer propagating the I/O error with ? (or mapping it into the existing error type) so the CLI exits cleanly with a message and non-zero status.
| std::fs::write(&output_path, output).expect("Failed to write grammar.ebnf"); | |
| std::fs::write(&output_path, output)?; |
| /// Parse the `#[kind = "..."]` attribute | ||
| fn parse_kind_attribute(attrs: &[Attribute]) -> Option<&'static str> { | ||
| for attr in attrs { | ||
| if attr.path().is_ident("kind") { | ||
| let meta = &attr.meta; | ||
| if let syn::Meta::NameValue(name_value) = meta { | ||
| if let syn::Expr::Lit(syn::ExprLit { | ||
| lit: syn::Lit::Str(lit_str), | ||
| .. | ||
| }) = &name_value.value | ||
| { | ||
| return Some(lit_str.value().leak()); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| None | ||
| } | ||
|
|
||
| /// Parse the `#[keyword = "..."]` attribute | ||
| fn parse_keyword_attribute(attrs: &[Attribute]) -> Option<&'static str> { | ||
| for attr in attrs { | ||
| if attr.path().is_ident("keyword") { | ||
| // Parse the attribute value | ||
| let meta = &attr.meta; | ||
| if let syn::Meta::NameValue(name_value) = meta { | ||
| if let syn::Expr::Lit(syn::ExprLit { | ||
| lit: syn::Lit::Str(lit_str), | ||
| .. | ||
| }) = &name_value.value | ||
| { | ||
| return Some(lit_str.value().leak()); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| None | ||
| } |
There was a problem hiding this comment.
parse_kind_attribute() / parse_keyword_attribute() convert syn::LitStr to &'static str via String::leak(). Even though this runs in the proc-macro process, leaking per derive invocation is avoidable and can bloat the compiler process on large crates. Prefer returning an owned String (or LitStr) and carrying it through codegen without leaking.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
@Ph0enixKM I think that requires a real review especially for the I see also that Copilot report that the grammar.ebnf as it is today, is not following really the standard. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/modules/function/fail.rs (1)
12-18:⚠️ Potential issue | 🟡 MinorMake
is_mainprivate.
This field is only accessed internally within theFailimplementation and should not be part of the public API. Remove thepubvisibility modifier entirely.♻️ Suggested visibility fix
pub struct Fail { pub expr: Expr, pub code: String, - pub is_main: bool, + is_main: bool, }
🤖 Fix all issues with AI agents
In `@meta/src/lib.rs`:
- Line 10: After parsing the input as a DeriveInput in the context_manager and
context_helper macros, add a compile-time guard that checks that input.data is
syn::Data::Struct and returns a syn::Error compiled into a TokenStream if not;
update both macros in meta/src/lib.rs (the functions that call
parse_macro_input::<DeriveInput>) to emit Error::new_spanned(...) ->
to_compile_error().into() when the data is not a struct so
ManagerVisitor/HelperVisitor won’t silently produce empty impls for enums or
unions.
In `@src/main.rs`:
- Around line 163-211: Remove the duplicate main() (the first definition) and
keep the second main that calls .with_env_vars(); then add a
CommandKind::GrammarEbnf arm into that retained main which mirrors the removed
one by calling grammar_ebnf::generate_grammar_ebnf(), writing the result to
PathBuf::from("grammar.ebnf") with std::fs::write (with the same error
handling/expect message), and returning 0 as the exit code; ensure the other
command arms (e.g., CommandKind::Build, CommandKind::Docs,
CompilerOptions::from_args, execute_output) remain unchanged and the program
still calls std::process::exit(exit_code) at the end.
In `@src/modules/expression/literal/status.rs`:
- Around line 10-12: The Status literal is incorrectly annotated as a statement:
remove the #[kind = "stmt"] attribute from the Status declaration so it matches
other literals (Bool, Integer, Null, Number, Text, Array) and remains an
expression that implements Typed (returns Type::Int) and translates to
VarExprFragment; if a kind is required by the macro, change it to the
expression-appropriate kind instead of "stmt".
In `@src/modules/keywords.rs`:
- Around line 5-9: Clippy warns about the blank line after the outer attribute
#[allow(dead_code)]; remove the empty line so the doc comment (the /// The kind
of keyword ...) immediately follows the attribute, or if the documentation is
meant for the module convert the outer attribute into an inner one
(#![allow(dead_code)]) and keep the module-level doc comments as-is; update the
code around #[allow(dead_code)] and the subsequent doc comment accordingly.
In `@src/utils/grammar_ebnf.rs`:
- Around line 201-231: The generator in grammar_ebnf.rs is emitting a
one-argument rule for all builtins (loop over builtin_stmt_keywords /
builtin_expr_keywords) which misrepresents multi-arg builtins like Mv::parse and
produces builtin_mv = KEYWORD_MV, expression ; — fix by introducing per-builtin
signature metadata (e.g., a map from keyword to
signature/arity/parentheses/optional modifiers) and update the loop that builds
builtin_name rules to consult that metadata; for "mv" emit a rule matching its
real signature (include optional modifiers and two expressions/parenthesized
expressions, e.g. builtin_mv = KEYWORD_MV, modifiers?, expression, expression ;
or the project’s grammar-equivalent), and extend the metadata for other
multi-arg builtins so generated rules reflect correct arity and optional
parentheses.
🧹 Nitpick comments (5)
meta/src/lib.rs (1)
237-254: Avoid leaking strings during macro expansion.
lit_str.value().leak()at lines 248 and 268 permanently allocates in the compiler process. ReturningStringis sufficient and works correctly with thequote!macro, which properly interpolates string values to&'static strliterals in generated code.♻️ Refactor to return `String` instead of leaking
-fn parse_kind_attribute(attrs: &[Attribute]) -> Option<&'static str> { +fn parse_kind_attribute(attrs: &[Attribute]) -> Option<String> { for attr in attrs { if attr.path().is_ident("kind") { let meta = &attr.meta; if let syn::Meta::NameValue(name_value) = meta { if let syn::Expr::Lit(syn::ExprLit { lit: syn::Lit::Str(lit_str), .. }) = &name_value.value { - return Some(lit_str.value().leak()); + return Some(lit_str.value()); } } } } None } -fn parse_keyword_attribute(attrs: &[Attribute]) -> Option<&'static str> { +fn parse_keyword_attribute(attrs: &[Attribute]) -> Option<String> { for attr in attrs { if attr.path().is_ident("keyword") { let meta = &attr.meta; if let syn::Meta::NameValue(name_value) = meta { if let syn::Expr::Lit(syn::ExprLit { lit: syn::Lit::Str(lit_str), .. }) = &name_value.value { - return Some(lit_str.value().leak()); + return Some(lit_str.value()); } } } } None }Also applies to: lines 256-274
src/modules/builtin/sleep.rs (1)
9-11: Minor formatting inconsistency: trailing space in attribute.Line 11 has a trailing space before the closing bracket (
#[kind = "builtin_stmt" ]), while other files in this PR don't have this inconsistency. This is a nitpick but worth keeping formatting consistent across the codebase.Note: Unlike other files in this PR (e.g.,
status.rs,and.rs), this file doesn't explicitly importamber_meta::AutoKeywordand relies on the prelude re-export. Both approaches work, but consider being consistent across the codebase.src/modules/builtin/ls.rs (1)
11-13: Minor formatting: trailing space in attribute.Same minor inconsistency as in
sleep.rs- trailing space before closing bracket on line 13 (#[kind = "builtin_expr" ]). The keyword and kind values are correct and match the implementation.src/modules/condition/ifchain.rs (1)
15-18: Consider keepingcond_blocks/false_blockencapsulated.Making these fields
pubwidens the mutation surface; if external access is required, accessors or a focused API can preserve invariants.src/utils/grammar_ebnf.rs (1)
180-189: Sort/dedup builtin keyword lists for stable grammar output.
keyword_defsis deterministic, but builtin lists preserve iteration order. Sorting + deduping avoids duplicate rule definitions and makes the generated grammar stable across builds.Suggested change
- let builtin_stmt_keywords: Vec<&str> = iter_keywords() + let mut builtin_stmt_keywords: Vec<&str> = iter_keywords() .filter(|r| r.kind == KeywordKind::BuiltinStmt) .map(|r| r.keyword) .collect(); + builtin_stmt_keywords.sort(); + builtin_stmt_keywords.dedup(); - let builtin_expr_keywords: Vec<&str> = iter_keywords() + let mut builtin_expr_keywords: Vec<&str> = iter_keywords() .filter(|r| r.kind == KeywordKind::BuiltinExpr) .map(|r| r.keyword) .collect(); + builtin_expr_keywords.sort(); + builtin_expr_keywords.dedup();
| use proc_macro::TokenStream; | ||
| use syn::visit::Visit; | ||
| use syn::{parse_macro_input, DeriveInput, ItemStruct}; | ||
| use syn::{parse_macro_input, Attribute, DeriveInput}; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's read the meta/src/lib.rs file to see the current implementation
cat -n meta/src/lib.rsRepository: amber-lang/amber
Length of output: 10775
🏁 Script executed:
# Check the ManagerVisitor implementation
cat -n meta/src/manager.rs | head -100Repository: amber-lang/amber
Length of output: 4392
🏁 Script executed:
# Continue reading manager.rs to see visit_data implementation
cat -n meta/src/manager.rs | tail -20Repository: amber-lang/amber
Length of output: 849
🏁 Script executed:
# Check if there's a visit_data method in manager.rs
grep -n "visit_data" meta/src/manager.rsRepository: amber-lang/amber
Length of output: 42
🏁 Script executed:
# Check helper.rs to see HelperVisitor implementation
cat -n meta/src/helper.rsRepository: amber-lang/amber
Length of output: 1746
🏁 Script executed:
# Search for actual usages of #[derive(ContextManager)] and #[derive(ContextHelper)]
rg -n "#\[derive.*ContextManager\]" -A 1
rg -n "#\[derive.*ContextHelper\]" -A 1Repository: amber-lang/amber
Length of output: 42
🏁 Script executed:
# Try broader search for ContextManager and ContextHelper derives
rg "ContextManager|ContextHelper" --type rust -B 3 -A 1Repository: amber-lang/amber
Length of output: 4931
🏁 Script executed:
# Check if visit_data is defined in syn or is custom
grep -rn "visit_data" meta/src/Repository: amber-lang/amber
Length of output: 160
🏁 Script executed:
# Check the syn crate to understand visit_data - let's look at what it does
# First, let's see if there's a Cargo.toml in meta to check syn version
cat meta/Cargo.toml | grep -A 5 "\[dependencies\]"Repository: amber-lang/amber
Length of output: 186
🏁 Script executed:
# Let's write a quick test to understand how syn's Visit trait behaves with enums
python3 << 'EOF'
# Check syn documentation for Visit trait behavior
# The Visit trait has default implementations that dispatch based on the data kind
# For enums, visit_data calls visit_enum_data which iterates over variants
# Since the visitors only implement visit_field, enums would be silently processed with empty output
# Let's verify that no struct guard exists by checking the exact flow
print("Verification of concern:")
print("1. context_manager parses as DeriveInput (lines 87-94)")
print("2. No explicit guard checking data is Struct")
print("3. Calls visitor.visit_data(&data)")
print("4. ManagerVisitor only implements visit_field")
print("5. For enums, visit_data would dispatch to visit_enum_data")
print("6. This would visit variants instead of fields")
print("7. Since no visit_variant is implemented, nothing happens")
print("8. Macro silently generates empty impl with no error")
print("\nAll current usages are on structs, so problem is latent")
EOFRepository: amber-lang/amber
Length of output: 539
🏁 Script executed:
# Let's verify the exact behavior by checking if there's any safety check we missed
grep -n "matches!" meta/src/lib.rsRepository: amber-lang/amber
Length of output: 42
Add explicit struct-only guards to context_manager and context_helper.
Both macros currently parse as DeriveInput without checking that the input is actually a struct. If applied to an enum or union, the visitors would iterate over variants or union fields respectively, and since ManagerVisitor and HelperVisitor only implement visit_field, the macros would silently generate empty implementations with no error. Add a compile-time check to ensure these macros only work on structs.
Suggested guard to preserve the prior behavior
pub fn context_manager(input: TokenStream) -> TokenStream {
let input = parse_macro_input!(input as DeriveInput);
let syn::DeriveInput { data, ident, .. } = input;
+ if !matches!(data, syn::Data::Struct(_)) {
+ return syn::Error::new_spanned(
+ &ident,
+ "ContextManager can only be derived for structs",
+ )
+ .to_compile_error()
+ .into();
+ }
let mut visitor = ManagerVisitor::new(&ident);
visitor.visit_data(&data);Also applies to: lines 100-106 (context_helper)
🤖 Prompt for AI Agents
In `@meta/src/lib.rs` at line 10, After parsing the input as a DeriveInput in the
context_manager and context_helper macros, add a compile-time guard that checks
that input.data is syn::Data::Struct and returns a syn::Error compiled into a
TokenStream if not; update both macros in meta/src/lib.rs (the functions that
call parse_macro_input::<DeriveInput>) to emit Error::new_spanned(...) ->
to_compile_error().into() when the data is not a struct so
ManagerVisitor/HelperVisitor won’t silently produce empty impls for enums or
unions.
| #[derive(Debug, Clone, AutoKeyword)] | ||
| #[keyword = "status"] | ||
| #[kind = "stmt"] |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/modules/expression/literal/status.rsRepository: amber-lang/amber
Length of output: 2027
🏁 Script executed:
# Find all kind declarations in the codebase to understand patterns
rg -n '#\[kind\s*=' --type rust -B2 | head -100Repository: amber-lang/amber
Length of output: 4613
🏁 Script executed:
# Search for Pwd and other builtin expressions
rg -n '#\[kind' --type rust src/modules/expression/literal/ src/modules/expression/binop/ | head -50Repository: amber-lang/amber
Length of output: 973
🏁 Script executed:
# Let's look for Pwd specifically
find . -name "pwd.rs" -type fRepository: amber-lang/amber
Length of output: 88
🏁 Script executed:
# More broadly search for builtin_expr or expr kind values
rg -n '#\[kind\s*=\s*".*expr' --type rustRepository: amber-lang/amber
Length of output: 399
🏁 Script executed:
# Check if there are any other literals in expression/literal/ to see their kind
ls -la src/modules/expression/literal/Repository: amber-lang/amber
Length of output: 634
🏁 Script executed:
# Look at the structure of other files in expression/literal
for file in src/modules/expression/literal/*.rs; do echo "=== $file ==="; grep -E '#\[kind|impl.*Typed' "$file"; doneRepository: amber-lang/amber
Length of output: 621
🏁 Script executed:
# Check the actual content of a few other literal files to see if any have kind attributes
cat -n src/modules/expression/literal/bool.rsRepository: amber-lang/amber
Length of output: 1598
🏁 Script executed:
cat -n src/modules/expression/literal/integer.rs | head -20Repository: amber-lang/amber
Length of output: 638
Remove or correct the kind attribute for Status.
Status is an expression literal that implements Typed (returns Type::Int) and translates to a VarExprFragment. Marking it as kind = "stmt" is semantically incorrect—statements define control flow or declarations, not expressions that return values.
All other literals in src/modules/expression/literal/ (Bool, Integer, Null, Number, Text, Array) omit the kind attribute entirely and follow the same pattern. Status should either match this convention by removing the attribute, or if a kind is required, be classified as an expression type rather than a statement.
🤖 Prompt for AI Agents
In `@src/modules/expression/literal/status.rs` around lines 10 - 12, The Status
literal is incorrectly annotated as a statement: remove the #[kind = "stmt"]
attribute from the Status declaration so it matches other literals (Bool,
Integer, Null, Number, Text, Array) and remains an expression that implements
Typed (returns Type::Int) and translates to VarExprFragment; if a kind is
required by the macro, change it to the expression-appropriate kind instead of
"stmt".
| #[allow(dead_code)] | ||
|
|
||
| /// The kind of keyword (statement, builtin, or binary operation) | ||
| /// Also supports dynamic string-based kinds like "binary_op", "builtin_stmt", "builtin_expr", etc. | ||
| #[derive(Debug, Clone, Copy, PartialEq, Eq)] |
There was a problem hiding this comment.
Remove the empty line after the outer attribute (Clippy).
Clippy flags “empty line after outer attribute.” Place the doc comment immediately after the attribute (or convert to an inner attribute if it’s meant for the module).
Suggested fix
-#[allow(dead_code)]
-
-/// The kind of keyword (statement, builtin, or binary operation)
+#[allow(dead_code)]
+/// The kind of keyword (statement, builtin, or binary operation)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #[allow(dead_code)] | |
| /// The kind of keyword (statement, builtin, or binary operation) | |
| /// Also supports dynamic string-based kinds like "binary_op", "builtin_stmt", "builtin_expr", etc. | |
| #[derive(Debug, Clone, Copy, PartialEq, Eq)] | |
| #[allow(dead_code)] | |
| /// The kind of keyword (statement, builtin, or binary operation) | |
| /// Also supports dynamic string-based kinds like "binary_op", "builtin_stmt", "builtin_expr", etc. | |
| #[derive(Debug, Clone, Copy, PartialEq, Eq)] |
🧰 Tools
🪛 GitHub Check: Clippy tests
[failure] 5-5:
empty line after outer attribute
🤖 Prompt for AI Agents
In `@src/modules/keywords.rs` around lines 5 - 9, Clippy warns about the blank
line after the outer attribute #[allow(dead_code)]; remove the empty line so the
doc comment (the /// The kind of keyword ...) immediately follows the attribute,
or if the documentation is meant for the module convert the outer attribute into
an inner one (#![allow(dead_code)]) and keep the module-level doc comments
as-is; update the code around #[allow(dead_code)] and the subsequent doc comment
accordingly.
| // Generate individual builtin rules for statement builtins | ||
| let mut builtin_stmt_rules = String::new(); | ||
| for kw in &builtin_stmt_keywords { | ||
| let kw_upper = kw.to_uppercase(); | ||
| let builtin_name = format!("builtin_{}", kw); | ||
| match kw.as_ref() { | ||
| "clear" | "pid" => { | ||
| builtin_stmt_rules.push_str(&format!("{} = KEYWORD_{} ;\n", builtin_name, kw_upper)) | ||
| } | ||
| _ => builtin_stmt_rules.push_str(&format!( | ||
| "{} = KEYWORD_{}, expression ;\n", | ||
| builtin_name, kw_upper | ||
| )), | ||
| } | ||
| } | ||
| builtin_stmt_rules.push('\n'); | ||
|
|
||
| // Generate individual builtin rules for expression builtins | ||
| let mut builtin_expr_rules = String::new(); | ||
| for kw in &builtin_expr_keywords { | ||
| let kw_upper = kw.to_uppercase(); | ||
| let builtin_name = format!("builtin_{}", kw); | ||
| match kw.as_ref() { | ||
| "pid" => { | ||
| builtin_expr_rules.push_str(&format!("{} = KEYWORD_{} ;\n", builtin_name, kw_upper)) | ||
| } | ||
| _ => builtin_expr_rules.push_str(&format!( | ||
| "{} = KEYWORD_{}, expression ;\n", | ||
| builtin_name, kw_upper | ||
| )), | ||
| } |
There was a problem hiding this comment.
Generated builtin rules don’t match mv’s real signature (modifiers + 2 args).
Mv::parse accepts command modifiers and two expressions (optionally parenthesized), but the generator emits builtin_mv = KEYWORD_MV, expression ;. The produced grammar will reject valid syntax and misdocument mv (and likely other multi‑arg builtins). Consider encoding per‑builtin signatures (or metadata) so rules reflect real arity/parentheses and optional modifiers.
Example fix pattern (extend for other builtins as needed)
- _ => builtin_stmt_rules.push_str(&format!(
- "{} = KEYWORD_{}, expression ;\n",
- builtin_name, kw_upper
- )),
+ "mv" => builtin_stmt_rules.push_str(&format!(
+ "{} = command_modifier, KEYWORD_{}, ( '(', expression, ',', expression, ')' | expression, expression ) ;\n",
+ builtin_name, kw_upper
+ )),
+ _ => builtin_stmt_rules.push_str(&format!(
+ "{} = command_modifier, KEYWORD_{}, expression ;\n",
+ builtin_name, kw_upper
+ )),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Generate individual builtin rules for statement builtins | |
| let mut builtin_stmt_rules = String::new(); | |
| for kw in &builtin_stmt_keywords { | |
| let kw_upper = kw.to_uppercase(); | |
| let builtin_name = format!("builtin_{}", kw); | |
| match kw.as_ref() { | |
| "clear" | "pid" => { | |
| builtin_stmt_rules.push_str(&format!("{} = KEYWORD_{} ;\n", builtin_name, kw_upper)) | |
| } | |
| _ => builtin_stmt_rules.push_str(&format!( | |
| "{} = KEYWORD_{}, expression ;\n", | |
| builtin_name, kw_upper | |
| )), | |
| } | |
| } | |
| builtin_stmt_rules.push('\n'); | |
| // Generate individual builtin rules for expression builtins | |
| let mut builtin_expr_rules = String::new(); | |
| for kw in &builtin_expr_keywords { | |
| let kw_upper = kw.to_uppercase(); | |
| let builtin_name = format!("builtin_{}", kw); | |
| match kw.as_ref() { | |
| "pid" => { | |
| builtin_expr_rules.push_str(&format!("{} = KEYWORD_{} ;\n", builtin_name, kw_upper)) | |
| } | |
| _ => builtin_expr_rules.push_str(&format!( | |
| "{} = KEYWORD_{}, expression ;\n", | |
| builtin_name, kw_upper | |
| )), | |
| } | |
| // Generate individual builtin rules for statement builtins | |
| let mut builtin_stmt_rules = String::new(); | |
| for kw in &builtin_stmt_keywords { | |
| let kw_upper = kw.to_uppercase(); | |
| let builtin_name = format!("builtin_{}", kw); | |
| match kw.as_ref() { | |
| "clear" | "pid" => { | |
| builtin_stmt_rules.push_str(&format!("{} = KEYWORD_{} ;\n", builtin_name, kw_upper)) | |
| } | |
| "mv" => builtin_stmt_rules.push_str(&format!( | |
| "{} = command_modifier, KEYWORD_{}, ( '(', expression, ',', expression, ')' | expression, expression ) ;\n", | |
| builtin_name, kw_upper | |
| )), | |
| _ => builtin_stmt_rules.push_str(&format!( | |
| "{} = command_modifier, KEYWORD_{}, expression ;\n", | |
| builtin_name, kw_upper | |
| )), | |
| } | |
| } | |
| builtin_stmt_rules.push('\n'); | |
| // Generate individual builtin rules for expression builtins | |
| let mut builtin_expr_rules = String::new(); | |
| for kw in &builtin_expr_keywords { | |
| let kw_upper = kw.to_uppercase(); | |
| let builtin_name = format!("builtin_{}", kw); | |
| match kw.as_ref() { | |
| "pid" => { | |
| builtin_expr_rules.push_str(&format!("{} = KEYWORD_{} ;\n", builtin_name, kw_upper)) | |
| } | |
| _ => builtin_expr_rules.push_str(&format!( | |
| "{} = KEYWORD_{}, expression ;\n", | |
| builtin_name, kw_upper | |
| )), | |
| } |
🤖 Prompt for AI Agents
In `@src/utils/grammar_ebnf.rs` around lines 201 - 231, The generator in
grammar_ebnf.rs is emitting a one-argument rule for all builtins (loop over
builtin_stmt_keywords / builtin_expr_keywords) which misrepresents multi-arg
builtins like Mv::parse and produces builtin_mv = KEYWORD_MV, expression ; — fix
by introducing per-builtin signature metadata (e.g., a map from keyword to
signature/arity/parentheses/optional modifiers) and update the loop that builds
builtin_name rules to consult that metadata; for "mv" emit a rule matching its
real signature (include optional modifiers and two expressions/parenthesized
expressions, e.g. builtin_mv = KEYWORD_MV, modifiers?, expression, expression ;
or the project’s grammar-equivalent), and extend the metadata for other
multi-arg builtins so generated rules reflect correct arity and optional
parentheses.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
So I did a lot of changes because my plan was to create something that fill the file automatically using macros.
So part of the file is hardcoded and another part is updated automatically.
It requires a careful review because I changed a lot of files.
Summary by CodeRabbit
Release Notes
New Features
Chores