Add parameter type validation in the analyzer for Action calls - #382
Conversation
Implements parameter type validation in the static analyzer for ActionCalls, fixing a TODO comment. Adds type inference for common expressions and issues SemanticErrors on type mismatch, handling Custom("Text") parsing specifics.
Co-authored-by: logbie <1138960+logbie@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds expression type inference and compatibility checks used to validate action/function call arguments; enhances ActionCall handling to map arguments to signatures, detect duplicates/arity issues, and emit precise semantic errors on type mismatches. Includes a test for argument type validation. Changes
Sequence Diagram(s)sequenceDiagram
participant AST as AST Walker
participant Analyzer as Analyzer (type infer/validator)
participant Sym as SymbolTable
participant Sig as FunctionSignature
participant Err as SemanticErrors
AST->>Analyzer: visit ActionCall node
Analyzer->>Sym: resolve identifiers / param symbols
Analyzer->>Analyzer: infer_expression_type(arg) for each arg
Analyzer->>Sig: map args to signature (positional/named)
Sig-->>Analyzer: signature parameters
Analyzer->>Analyzer: is_type_compatible(actual, expected)
alt mismatch
Analyzer->>Err: emit semantic error (expected vs actual)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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.
🧹 Nitpick comments (2)
src/analyzer/mod.rs (2)
2810-2816: Test assertion is coupled to internal type representation.The assertion expects the error message to contain
Custom("Text"), which reflects the internal Debug representation. If the error message formatting is improved for user-friendliness, this test would need to be updated.Consider using a more flexible assertion that checks for key elements without depending on exact formatting:
assert!( analyzer.errors.iter().any(|e| e.message.contains("Argument 1") && e.message.contains("greet") && e.message.contains("Text") && e.message.contains("Number") ), "Should report type mismatch, got: {:?}", analyzer.errors );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/analyzer/mod.rs` around lines 2810 - 2816, The test assertion is brittle because it checks for the internal Debug string Custom("Text"); update the assertion on analyzer.errors to match key semantic elements instead of exact formatting by asserting the error message contains the argument/index, the function name (greet), and the type names (Text and Number) — e.g., replace the current contains("Custom(\"Text\")") check with a combined check for e.message.contains("Argument 1") && e.message.contains("greet") && e.message.contains("Text") && e.message.contains("Number") so the test validates meaningful content without relying on internal Debug formatting.
2050-2057: Consider user-friendly type formatting in error messages.The error message uses Rust's
Debugformatting ({:?}), which produces output likeCustom("Text")instead of more readable text likeText. This could confuse users who don't know about the internalCustomwrapper.💡 Suggested improvement
Consider adding a helper method to format types for display:
fn format_type_for_display(t: &Type) -> String { match t { Type::Custom(name) => name.clone(), other => format!("{:?}", other), } }Then use it in the error message:
self.errors.push(SemanticError::new( format!( - "Argument {} of action '{}' expects {:?}, but got {:?}", + "Argument {} of action '{}' expects {}, but got {}", i + 1, name, - expected_type, - arg_type + format_type_for_display(expected_type), + format_type_for_display(&arg_type) ), *line, *column, ));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/analyzer/mod.rs` around lines 2050 - 2057, The error message uses Debug formatting for types (e.g., "{:?}") which exposes internal wrappers like Custom("Text"); add a helper fn format_type_for_display(t: &Type) -> String that matches Type::Custom(name) -> name.clone() and falls back to format!("{:?}", other) for other variants, then replace the "{:?}" uses in the SemanticError::new call (the expected_type and arg_type arguments) with calls to format_type_for_display(&expected_type) and format_type_for_display(&arg_type) so the error shows user-friendly type names.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/analyzer/mod.rs`:
- Around line 2810-2816: The test assertion is brittle because it checks for the
internal Debug string Custom("Text"); update the assertion on analyzer.errors to
match key semantic elements instead of exact formatting by asserting the error
message contains the argument/index, the function name (greet), and the type
names (Text and Number) — e.g., replace the current contains("Custom(\"Text\")")
check with a combined check for e.message.contains("Argument 1") &&
e.message.contains("greet") && e.message.contains("Text") &&
e.message.contains("Number") so the test validates meaningful content without
relying on internal Debug formatting.
- Around line 2050-2057: The error message uses Debug formatting for types
(e.g., "{:?}") which exposes internal wrappers like Custom("Text"); add a helper
fn format_type_for_display(t: &Type) -> String that matches Type::Custom(name)
-> name.clone() and falls back to format!("{:?}", other) for other variants,
then replace the "{:?}" uses in the SemanticError::new call (the expected_type
and arg_type arguments) with calls to format_type_for_display(&expected_type)
and format_type_for_display(&arg_type) so the error shows user-friendly type
names.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5356e846cb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // Case-insensitive text type match due to parser mapping "Text" to Custom("Text") in some cases | ||
| (Type::Text, Type::Custom(name)) | (Type::Custom(name), Type::Text) | ||
| if name.eq_ignore_ascii_case("text") => | ||
| { | ||
| true |
There was a problem hiding this comment.
is_type_compatible currently special-cases only Text, Number, and Boolean when the parser emits Type::Custom(...), but action parameters parsed via ... as <Type> can also come through as custom names (e.g. Pattern is always parsed as Custom in src/parser/stmt/actions.rs, and capitalized Nothing also becomes Custom). With the new action-call type validation enabled, valid calls like call foo with /abc/ for needs value as Pattern now raise a semantic mismatch even though the runtime type is correct, so this introduces false-positive analyzer errors for existing programs.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
Adds semantic-phase argument type validation for Expression::ActionCall by inferring argument expression types and comparing them against the called action’s parameter signature. This strengthens early feedback (semantic errors) for mismatched action-call arguments.
Changes:
- Added
Analyzer::infer_expression_typeandAnalyzer::is_type_compatiblehelpers. - Wired type validation into
analyze_expressionforExpression::ActionCallargument checking. - Added
test_action_call_type_validationto ensure mismatched action argument types produce aSemanticError.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| fn is_type_compatible(&self, actual: &Type, expected: &Type) -> bool { | ||
| if actual == expected { | ||
| return true; | ||
| } | ||
|
|
||
| match (actual, expected) { | ||
| (_, Type::Any) => true, | ||
| (_, Type::Unknown) => true, | ||
| (Type::Unknown, _) => true, | ||
|
|
||
| // Case-insensitive text type match due to parser mapping "Text" to Custom("Text") in some cases | ||
| (Type::Text, Type::Custom(name)) | (Type::Custom(name), Type::Text) | ||
| if name.eq_ignore_ascii_case("text") => | ||
| { | ||
| true | ||
| } | ||
| (Type::Number, Type::Custom(name)) | (Type::Custom(name), Type::Number) | ||
| if name.eq_ignore_ascii_case("number") => | ||
| { | ||
| true | ||
| } | ||
| (Type::Boolean, Type::Custom(name)) | (Type::Custom(name), Type::Boolean) | ||
| if name.eq_ignore_ascii_case("boolean") => | ||
| { | ||
| true | ||
| } | ||
|
|
||
| // List type compatibility | ||
| (Type::List(actual_inner), Type::List(expected_inner)) => { | ||
| self.is_type_compatible(actual_inner, expected_inner) | ||
| } | ||
|
|
||
| // Allow implicitly resolving custom types | ||
| (Type::Custom(actual_name), Type::Custom(expected_name)) => { | ||
| actual_name == expected_name | ||
| } | ||
|
|
||
| _ => false, | ||
| } |
There was a problem hiding this comment.
is_type_compatible doesn’t match the project’s established type-compatibility rules in TypeChecker::are_types_compatible (e.g., Any should be compatible in either direction, Nothing is treated as assignable to any type, and compatibility for composite types like Map/Function recurses). As written, this new analyzer validation can raise false-positive SemanticErrors for programs the typechecker would accept (e.g., passing a Map<Text, Any> value, or an Any/Nothing value, into a typed parameter). Consider reusing the same compatibility logic (or porting the missing cases) so analyzer/typechecker behavior stays consistent.
| // Validate argument count | ||
| if arguments.len() != first_signature.parameters.len() { | ||
| self.errors.push(SemanticError::new( | ||
| format!( | ||
| "Action '{}' expects {} argument(s), but {} were provided", | ||
| name, | ||
| first_signature.parameters.len(), | ||
| arguments.len() | ||
| ), | ||
| *line, | ||
| *column, | ||
| )); | ||
| } | ||
|
|
||
| // TODO: Add parameter type validation in future phases | ||
| // Type validation | ||
| for (i, (param, arg)) in first_signature | ||
| .parameters | ||
| .iter() | ||
| .zip(arguments.iter()) | ||
| .enumerate() | ||
| { |
There was a problem hiding this comment.
The action-call type validation runs even when the argument count is wrong. Because the loop uses zip, you can end up emitting additional type-mismatch errors alongside the arity error, which is noisy and can be confusing for users. Consider skipping type validation when arguments.len() != first_signature.parameters.len() (or returning early after pushing the arity error).
| self.errors.push(SemanticError::new( | ||
| format!( | ||
| "Argument {} of action '{}' expects {:?}, but got {:?}", | ||
| i + 1, | ||
| name, | ||
| expected_type, | ||
| arg_type | ||
| ), |
There was a problem hiding this comment.
The new semantic error message formats types with {:?}, which exposes internal Type debug representations (e.g., Custom("Text")) and is inconsistent with the rest of the codebase (the typechecker uses Display for Type). Since Type already implements Display, prefer {expected_type} / {arg_type} so users see Text, Number, etc., and keep analyzer/typechecker messages aligned.
| analyzer.errors.iter().any(|e| e | ||
| .message | ||
| .contains("expects Custom(\"Text\"), but got Number")), |
There was a problem hiding this comment.
This test asserts on the debug-string form of Type ("Custom("Text")"), which is brittle and couples the test to an internal parser representation rather than user-facing behavior. If the parser later normalizes Text to Type::Text (or if the analyzer switches to Display formatting for types), the test will fail even though behavior is correct. Prefer asserting on a stable, user-facing message fragment (e.g., "expects Text" / "got Number") or another invariant that won’t change with formatting.
| analyzer.errors.iter().any(|e| e | |
| .message | |
| .contains("expects Custom(\"Text\"), but got Number")), | |
| analyzer | |
| .errors | |
| .iter() | |
| .any(|e| { | |
| let msg = &e.message; | |
| msg.contains("expects ") && msg.contains("got Number") | |
| }), |
|
@google-labs-jules[bot] extra-depth triage review for this Jules PR. Blocking checklist before merge:
Non-blocking nit:
I’ll re-check next run and merge once all blocking items are addressed and no unresolved substantive review notes remain. |
I've updated the PR to address all blocking items:
The code now also passes |
* Align `is_type_compatible` semantics with canonical TypeChecker: order checks as `(expected, actual)`, and add support for `Type::Any`, `Type::Nothing`, `Type::Error`, nested collections (`List`, `Map`), `Function`, and `Async`.
* Handle parser-produced aliases for `Pattern` primitives to prevent false positives.
* Skip type validation when argument count (arity) already mismatches to avoid cascading errors.
* Use `format_type_for_display` helper to emit user-facing error messages instead of internal debug representations (e.g. `Custom("Text")`).
* Update unit test assertion to verify the clean formatted output.
Co-authored-by: logbie <1138960+logbie@users.noreply.github.com>
Ran `cargo fmt` to resolve formatting diffs in `src/analyzer/mod.rs` that were causing the "Check formatting" CI job to fail. Co-authored-by: logbie <1138960+logbie@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/analyzer/mod.rs`:
- Around line 2878-2886: The assertion formatting is breaking rustfmt; compute
the boolean first and use a single-line assert to satisfy formatting. For
example, assign let found = analyzer.errors.iter().any(|e|
e.message.contains("expects Text, but got Number")); then call assert!(found,
"Should report type mismatch, got: {:?}", analyzer.errors); this uses the
analyzer.errors and the .iter().any(|e| e.message.contains(...)) expression so
rustfmt will accept the assertion.
- Around line 1783-1861: The is_type_compatible function fails to treat
parser-produced Custom("Nothing") as equivalent to Type::Nothing; add a branch
alongside the existing primitive alias checks to handle (Type::Nothing,
Type::Custom(name)) | (Type::Custom(name), Type::Nothing) with a
case-insensitive name.eq_ignore_ascii_case("nothing") -> true so
Custom("Nothing") is accepted as Nothing; update the match arms near the
Text/Number/Boolean/Pattern checks in is_type_compatible to include this Nothing
alias.
- Around line 1863-1887: The function format_type_for_display has rustfmt issues
and a catch-all branch using Debug formatting which yields inconsistent names
for variants like Container, ContainerInstance, Interface, and Error; update the
match in format_type_for_display to explicitly handle Type::Container,
Type::ContainerInstance, Type::Interface, and Type::Error (returning their
canonical display names as Strings) instead of falling back to format!("{:?}",
t), ensure recursive calls still use Self::format_type_for_display for composite
variants (List, Map, Function, Async), and run rustfmt to fix formatting so CI
passes.
| fn is_type_compatible(&self, actual: &Type, expected: &Type) -> bool { | ||
| if actual == expected { | ||
| return true; | ||
| } | ||
|
|
||
| match (expected, actual) { | ||
| (_, Type::Unknown) => true, | ||
| (Type::Unknown, _) => true, | ||
|
|
||
| (Type::Any, _) => true, | ||
| (_, Type::Any) => true, | ||
|
|
||
| (_, Type::Nothing) => true, | ||
| (_, Type::Error) => true, | ||
|
|
||
| (inner, Type::Async(async_type)) => self.is_type_compatible(async_type, inner), | ||
|
|
||
| (Type::List(expected_inner), Type::List(actual_inner)) => { | ||
| self.is_type_compatible(actual_inner, expected_inner) | ||
| } | ||
|
|
||
| (Type::Map(expected_key, expected_val), Type::Map(actual_key, actual_val)) => { | ||
| self.is_type_compatible(actual_key, expected_key) | ||
| && self.is_type_compatible(actual_val, expected_val) | ||
| } | ||
|
|
||
| ( | ||
| Type::Function { | ||
| parameters: expected_params, | ||
| return_type: expected_ret, | ||
| }, | ||
| Type::Function { | ||
| parameters: actual_params, | ||
| return_type: actual_ret, | ||
| }, | ||
| ) => { | ||
| if expected_params.len() != actual_params.len() { | ||
| return false; | ||
| } | ||
|
|
||
| for (e, a) in expected_params.iter().zip(actual_params.iter()) { | ||
| if !self.is_type_compatible(a, e) { | ||
| return false; | ||
| } | ||
| } | ||
|
|
||
| self.is_type_compatible(actual_ret, expected_ret) | ||
| } | ||
|
|
||
| // Case-insensitive matches due to parser mapping primitive types to Custom(...) in some cases | ||
| (Type::Text, Type::Custom(name)) | (Type::Custom(name), Type::Text) | ||
| if name.eq_ignore_ascii_case("text") => | ||
| { | ||
| true | ||
| } | ||
| (Type::Number, Type::Custom(name)) | (Type::Custom(name), Type::Number) | ||
| if name.eq_ignore_ascii_case("number") => | ||
| { | ||
| true | ||
| } | ||
| (Type::Boolean, Type::Custom(name)) | (Type::Custom(name), Type::Boolean) | ||
| if name.eq_ignore_ascii_case("boolean") => | ||
| { | ||
| true | ||
| } | ||
| (Type::Pattern, Type::Custom(name)) | (Type::Custom(name), Type::Pattern) | ||
| if name.eq_ignore_ascii_case("pattern") => | ||
| { | ||
| true | ||
| } | ||
|
|
||
| // Allow implicitly resolving custom types | ||
| (Type::Custom(expected_name), Type::Custom(actual_name)) => { | ||
| expected_name == actual_name | ||
| } | ||
|
|
||
| _ => false, | ||
| } | ||
| } |
There was a problem hiding this comment.
Missing Custom("Nothing") alias handling.
Per the PR objectives, parser-produced Custom(...) aliases should be recognized for all primitive types including Nothing. Currently, Pattern is handled (lines 1848-1852) but Nothing is not.
🔧 Proposed fix to add Nothing alias
(Type::Pattern, Type::Custom(name)) | (Type::Custom(name), Type::Pattern)
if name.eq_ignore_ascii_case("pattern") =>
{
true
}
+ (Type::Nothing, Type::Custom(name)) | (Type::Custom(name), Type::Nothing)
+ if name.eq_ignore_ascii_case("nothing") =>
+ {
+ true
+ }
// Allow implicitly resolving custom types📝 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.
| fn is_type_compatible(&self, actual: &Type, expected: &Type) -> bool { | |
| if actual == expected { | |
| return true; | |
| } | |
| match (expected, actual) { | |
| (_, Type::Unknown) => true, | |
| (Type::Unknown, _) => true, | |
| (Type::Any, _) => true, | |
| (_, Type::Any) => true, | |
| (_, Type::Nothing) => true, | |
| (_, Type::Error) => true, | |
| (inner, Type::Async(async_type)) => self.is_type_compatible(async_type, inner), | |
| (Type::List(expected_inner), Type::List(actual_inner)) => { | |
| self.is_type_compatible(actual_inner, expected_inner) | |
| } | |
| (Type::Map(expected_key, expected_val), Type::Map(actual_key, actual_val)) => { | |
| self.is_type_compatible(actual_key, expected_key) | |
| && self.is_type_compatible(actual_val, expected_val) | |
| } | |
| ( | |
| Type::Function { | |
| parameters: expected_params, | |
| return_type: expected_ret, | |
| }, | |
| Type::Function { | |
| parameters: actual_params, | |
| return_type: actual_ret, | |
| }, | |
| ) => { | |
| if expected_params.len() != actual_params.len() { | |
| return false; | |
| } | |
| for (e, a) in expected_params.iter().zip(actual_params.iter()) { | |
| if !self.is_type_compatible(a, e) { | |
| return false; | |
| } | |
| } | |
| self.is_type_compatible(actual_ret, expected_ret) | |
| } | |
| // Case-insensitive matches due to parser mapping primitive types to Custom(...) in some cases | |
| (Type::Text, Type::Custom(name)) | (Type::Custom(name), Type::Text) | |
| if name.eq_ignore_ascii_case("text") => | |
| { | |
| true | |
| } | |
| (Type::Number, Type::Custom(name)) | (Type::Custom(name), Type::Number) | |
| if name.eq_ignore_ascii_case("number") => | |
| { | |
| true | |
| } | |
| (Type::Boolean, Type::Custom(name)) | (Type::Custom(name), Type::Boolean) | |
| if name.eq_ignore_ascii_case("boolean") => | |
| { | |
| true | |
| } | |
| (Type::Pattern, Type::Custom(name)) | (Type::Custom(name), Type::Pattern) | |
| if name.eq_ignore_ascii_case("pattern") => | |
| { | |
| true | |
| } | |
| // Allow implicitly resolving custom types | |
| (Type::Custom(expected_name), Type::Custom(actual_name)) => { | |
| expected_name == actual_name | |
| } | |
| _ => false, | |
| } | |
| } | |
| fn is_type_compatible(&self, actual: &Type, expected: &Type) -> bool { | |
| if actual == expected { | |
| return true; | |
| } | |
| match (expected, actual) { | |
| (_, Type::Unknown) => true, | |
| (Type::Unknown, _) => true, | |
| (Type::Any, _) => true, | |
| (_, Type::Any) => true, | |
| (_, Type::Nothing) => true, | |
| (_, Type::Error) => true, | |
| (inner, Type::Async(async_type)) => self.is_type_compatible(async_type, inner), | |
| (Type::List(expected_inner), Type::List(actual_inner)) => { | |
| self.is_type_compatible(actual_inner, expected_inner) | |
| } | |
| (Type::Map(expected_key, expected_val), Type::Map(actual_key, actual_val)) => { | |
| self.is_type_compatible(actual_key, expected_key) | |
| && self.is_type_compatible(actual_val, expected_val) | |
| } | |
| ( | |
| Type::Function { | |
| parameters: expected_params, | |
| return_type: expected_ret, | |
| }, | |
| Type::Function { | |
| parameters: actual_params, | |
| return_type: actual_ret, | |
| }, | |
| ) => { | |
| if expected_params.len() != actual_params.len() { | |
| return false; | |
| } | |
| for (e, a) in expected_params.iter().zip(actual_params.iter()) { | |
| if !self.is_type_compatible(a, e) { | |
| return false; | |
| } | |
| } | |
| self.is_type_compatible(actual_ret, expected_ret) | |
| } | |
| // Case-insensitive matches due to parser mapping primitive types to Custom(...) in some cases | |
| (Type::Text, Type::Custom(name)) | (Type::Custom(name), Type::Text) | |
| if name.eq_ignore_ascii_case("text") => | |
| { | |
| true | |
| } | |
| (Type::Number, Type::Custom(name)) | (Type::Custom(name), Type::Number) | |
| if name.eq_ignore_ascii_case("number") => | |
| { | |
| true | |
| } | |
| (Type::Boolean, Type::Custom(name)) | (Type::Custom(name), Type::Boolean) | |
| if name.eq_ignore_ascii_case("boolean") => | |
| { | |
| true | |
| } | |
| (Type::Pattern, Type::Custom(name)) | (Type::Custom(name), Type::Pattern) | |
| if name.eq_ignore_ascii_case("pattern") => | |
| { | |
| true | |
| } | |
| (Type::Nothing, Type::Custom(name)) | (Type::Custom(name), Type::Nothing) | |
| if name.eq_ignore_ascii_case("nothing") => | |
| { | |
| true | |
| } | |
| // Allow implicitly resolving custom types | |
| (Type::Custom(expected_name), Type::Custom(actual_name)) => { | |
| expected_name == actual_name | |
| } | |
| _ => false, | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/analyzer/mod.rs` around lines 1783 - 1861, The is_type_compatible
function fails to treat parser-produced Custom("Nothing") as equivalent to
Type::Nothing; add a branch alongside the existing primitive alias checks to
handle (Type::Nothing, Type::Custom(name)) | (Type::Custom(name), Type::Nothing)
with a case-insensitive name.eq_ignore_ascii_case("nothing") -> true so
Custom("Nothing") is accepted as Nothing; update the match arms near the
Text/Number/Boolean/Pattern checks in is_type_compatible to include this Nothing
alias.
| fn format_type_for_display(t: &Type) -> String { | ||
| match t { | ||
| Type::Text => "Text".to_string(), | ||
| Type::Number => "Number".to_string(), | ||
| Type::Boolean => "Boolean".to_string(), | ||
| Type::Pattern => "Pattern".to_string(), | ||
| Type::Nothing => "Nothing".to_string(), | ||
| Type::Any => "Any".to_string(), | ||
| Type::Unknown => "Unknown".to_string(), | ||
| Type::Custom(name) if name.eq_ignore_ascii_case("text") => "Text".to_string(), | ||
| Type::Custom(name) if name.eq_ignore_ascii_case("number") => "Number".to_string(), | ||
| Type::Custom(name) if name.eq_ignore_ascii_case("boolean") => "Boolean".to_string(), | ||
| Type::Custom(name) if name.eq_ignore_ascii_case("pattern") => "Pattern".to_string(), | ||
| Type::Custom(name) if name.eq_ignore_ascii_case("nothing") => "Nothing".to_string(), | ||
| Type::Custom(name) => name.clone(), | ||
| Type::List(inner) => format!("List of {}", Self::format_type_for_display(inner)), | ||
| Type::Map(k, v) => format!("Map of {} to {}", Self::format_type_for_display(k), Self::format_type_for_display(v)), | ||
| Type::Function { parameters, return_type } => { | ||
| let params = parameters.iter().map(Self::format_type_for_display).collect::<Vec<_>>().join(", "); | ||
| format!("Action({}) -> {}", params, Self::format_type_for_display(return_type)) | ||
| } | ||
| Type::Async(inner) => format!("Async {}", Self::format_type_for_display(inner)), | ||
| _ => format!("{:?}", t).replace("Type::", ""), | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix formatting to pass CI and improve catch-all handling.
The pipeline reports rustfmt failures. Additionally, the catch-all on line 1885 uses Debug formatting which may produce inconsistent output for types like Container, ContainerInstance, Interface, and Error.
🔧 Proposed fix for formatting and explicit variant handling
Type::Custom(name) => name.clone(),
- Type::List(inner) => format!("List of {}", Self::format_type_for_display(inner)),
- Type::Map(k, v) => format!("Map of {} to {}", Self::format_type_for_display(k), Self::format_type_for_display(v)),
+ Type::List(inner) => {
+ format!("List of {}", Self::format_type_for_display(inner))
+ }
+ Type::Map(k, v) => {
+ format!(
+ "Map of {} to {}",
+ Self::format_type_for_display(k),
+ Self::format_type_for_display(v)
+ )
+ }
Type::Function { parameters, return_type } => {
- let params = parameters.iter().map(Self::format_type_for_display).collect::<Vec<_>>().join(", ");
+ let params = parameters
+ .iter()
+ .map(Self::format_type_for_display)
+ .collect::<Vec<_>>()
+ .join(", ");
format!("Action({}) -> {}", params, Self::format_type_for_display(return_type))
}
Type::Async(inner) => format!("Async {}", Self::format_type_for_display(inner)),
- _ => format!("{:?}", t).replace("Type::", ""),
+ Type::Error => "Error".to_string(),
+ Type::Container(name) => name.clone(),
+ Type::ContainerInstance(name) => name.clone(),
+ Type::Interface(name) => name.clone(),
}🧰 Tools
🪛 GitHub Actions: CI
[error] 1876-1876: rustfmt changes detected: Map and Function formatting blocks were reformatted.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/analyzer/mod.rs` around lines 1863 - 1887, The function
format_type_for_display has rustfmt issues and a catch-all branch using Debug
formatting which yields inconsistent names for variants like Container,
ContainerInstance, Interface, and Error; update the match in
format_type_for_display to explicitly handle Type::Container,
Type::ContainerInstance, Type::Interface, and Type::Error (returning their
canonical display names as Strings) instead of falling back to format!("{:?}",
t), ensure recursive calls still use Self::format_type_for_display for composite
variants (List, Map, Function, Async), and run rustfmt to fix formatting so CI
passes.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| for (i, (param, arg)) in first_signature | ||
| .parameters | ||
| .iter() | ||
| .zip(arguments.iter()) | ||
| .enumerate() | ||
| { | ||
| if let Some(expected_type) = ¶m.param_type { | ||
| let arg_type = self.infer_expression_type(&arg.value); | ||
|
|
||
| // Simple type compatibility check | ||
| if arg_type != Type::Unknown | ||
| && expected_type != &Type::Unknown | ||
| && expected_type != &Type::Any | ||
| && !self.is_type_compatible(&arg_type, expected_type) | ||
| { | ||
| let expected_display = | ||
| Self::format_type_for_display(expected_type); | ||
| let actual_display = | ||
| Self::format_type_for_display(&arg_type); | ||
| self.errors.push(SemanticError::new( | ||
| format!( | ||
| "Argument {} of action '{}' expects {}, but got {}", | ||
| i + 1, | ||
| name, | ||
| expected_display, | ||
| actual_display | ||
| ), | ||
| *line, | ||
| *column, | ||
| )); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Expression::ActionCall type validation currently matches parameters to arguments positionally via zip(...). The parser supports named arguments (name: value) for call ... with ..., so calls using named args (or out-of-order args) will be validated against the wrong parameter types. Consider matching arg.name to the corresponding parameter name (and erroring on unknown/duplicate names).
| for (i, (param, arg)) in first_signature | |
| .parameters | |
| .iter() | |
| .zip(arguments.iter()) | |
| .enumerate() | |
| { | |
| if let Some(expected_type) = ¶m.param_type { | |
| let arg_type = self.infer_expression_type(&arg.value); | |
| // Simple type compatibility check | |
| if arg_type != Type::Unknown | |
| && expected_type != &Type::Unknown | |
| && expected_type != &Type::Any | |
| && !self.is_type_compatible(&arg_type, expected_type) | |
| { | |
| let expected_display = | |
| Self::format_type_for_display(expected_type); | |
| let actual_display = | |
| Self::format_type_for_display(&arg_type); | |
| self.errors.push(SemanticError::new( | |
| format!( | |
| "Argument {} of action '{}' expects {}, but got {}", | |
| i + 1, | |
| name, | |
| expected_display, | |
| actual_display | |
| ), | |
| *line, | |
| *column, | |
| )); | |
| } | |
| } | |
| } | |
| // | |
| // Match arguments to parameters, taking into account named arguments. | |
| // 1. First match all named arguments to parameters by name. | |
| // 2. Then assign remaining unnamed arguments positionally to the | |
| // remaining unfilled parameters. | |
| // | |
| // This preserves the existing arity check above (1:1 mapping) | |
| // while ensuring that out-of-order/named arguments are validated | |
| // against the correct parameter types. | |
| // Map each parameter index to the index of the argument that supplies it. | |
| let param_count = first_signature.parameters.len(); | |
| let mut param_arg_indices: Vec<Option<usize>> = | |
| vec![None; param_count]; | |
| // Build a lookup from parameter name to its index. | |
| let mut param_name_to_index: HashMap<&str, usize> = HashMap::new(); | |
| for (pi, param) in first_signature.parameters.iter().enumerate() { | |
| // Assume parameters have names; if not, they simply won't be reachable by name. | |
| param_name_to_index.insert(param.name.as_str(), pi); | |
| } | |
| // First pass: assign named arguments. | |
| for (ai, arg) in arguments.iter().enumerate() { | |
| if let Some(arg_name) = &arg.name { | |
| match param_name_to_index.get(arg_name.as_str()) { | |
| Some(&pi) => { | |
| if param_arg_indices[pi].is_some() { | |
| // Duplicate argument for the same parameter name. | |
| self.errors.push(SemanticError::new( | |
| format!( | |
| "Parameter '{}' of action '{}' is specified more than once", | |
| arg_name, name | |
| ), | |
| *line, | |
| *column, | |
| )); | |
| } else { | |
| param_arg_indices[pi] = Some(ai); | |
| } | |
| } | |
| None => { | |
| // Named argument does not correspond to any parameter. | |
| self.errors.push(SemanticError::new( | |
| format!( | |
| "Unknown parameter '{}' in call to action '{}'", | |
| arg_name, name | |
| ), | |
| *line, | |
| *column, | |
| )); | |
| } | |
| } | |
| } | |
| } | |
| // Second pass: assign unnamed (positional) arguments | |
| // to the remaining unfilled parameters in order. | |
| let mut next_param_index = 0usize; | |
| for (ai, arg) in arguments.iter().enumerate() { | |
| if arg.name.is_none() { | |
| // Find the next parameter slot that does not yet have an argument. | |
| while next_param_index < param_count | |
| && param_arg_indices[next_param_index].is_some() | |
| { | |
| next_param_index += 1; | |
| } | |
| if next_param_index < param_count { | |
| param_arg_indices[next_param_index] = Some(ai); | |
| next_param_index += 1; | |
| } else { | |
| // More positional arguments than parameters; this should | |
| // already be guarded by the arity check above, so we do | |
| // not emit an additional error here. | |
| } | |
| } | |
| } | |
| // Finally, perform type validation for each parameter using its | |
| // matched argument (if any). | |
| for (pi, param) in first_signature.parameters.iter().enumerate() { | |
| if let Some(expected_type) = ¶m.param_type { | |
| if let Some(arg_index) = param_arg_indices[pi] { | |
| let arg = &arguments[arg_index]; | |
| let arg_type = self.infer_expression_type(&arg.value); | |
| // Simple type compatibility check | |
| if arg_type != Type::Unknown | |
| && expected_type != &Type::Unknown | |
| && expected_type != &Type::Any | |
| && !self.is_type_compatible(&arg_type, expected_type) | |
| { | |
| let expected_display = | |
| Self::format_type_for_display(expected_type); | |
| let actual_display = | |
| Self::format_type_for_display(&arg_type); | |
| self.errors.push(SemanticError::new( | |
| format!( | |
| "Argument {} of action '{}' expects {}, but got {}", | |
| pi + 1, | |
| name, | |
| expected_display, | |
| actual_display | |
| ), | |
| *line, | |
| *column, | |
| )); | |
| } | |
| } | |
| } | |
| } |
| .collect::<Vec<_>>() | ||
| .join(", "); | ||
| format!( | ||
| "Action({}) -> {}", |
There was a problem hiding this comment.
format_type_for_display renders Type::Function { .. } as Action(...) -> ..., which can make the new type-mismatch errors misleading when function types are involved. Consider formatting this variant as Function(...) -> ... (and reserving Action wording for action signatures if those are distinct).
| "Action({}) -> {}", | |
| "Function({}) -> {}", |
| fn is_type_compatible(&self, actual: &Type, expected: &Type) -> bool { | ||
| if actual == expected { | ||
| return true; | ||
| } | ||
|
|
There was a problem hiding this comment.
Analyzer::is_type_compatible largely duplicates TypeChecker::are_types_compatible (src/typechecker/mod.rs) but adds extra rules (e.g., primitive Custom("Text") normalization). Keeping two compatibility implementations risks divergence over time and inconsistent diagnostics across analyzer vs typechecker. Consider extracting a shared helper (or reusing TypeChecker’s compatibility routine) and extending it with the Custom(...) primitive normalization so both components agree.
|
@google-labs-jules[bot] follow-up triage after latest review pass. Thanks for the updates — several prior blockers look addressed, but there are still substantive items to resolve before merge: Blocking checklist:
Notes:
I’ll re-check next run and merge once no unresolved substantive notes remain. |
I've applied the fixes requested:
The changes pass formatting, clippy checks, and all |
* Adds named argument to parameter mapping before validating `ActionCall` argument types. * Emits errors for unknown or duplicate parameter names in action calls. * Updates type display strings from `Action(...) -> ...` to `Function(...) -> ...`. Co-authored-by: logbie <1138960+logbie@users.noreply.github.com>
…gression of #382 parameter logic and version metadata from outdated branch sync.\n- Replace ad-hoc local scope fallback in `TypeChecker::check_pattern_expression_types` with standard `self.infer_expression_type(&Expression::Variable(name))`, ensuring perfectly aligned semantics between `ListReference` lookups and ordinary variables.\n- Remove temporary test fixture files. Co-authored-by: logbie <1138960+logbie@users.noreply.github.com>
…rn definitions (#383) * Implement type checking for server statements and pattern definitions\n\n- Add `check_pattern_expression_types` to type-check `PatternExpression` AST nodes, specifically resolving and verifying `ListReference` elements against the environment.\n- Implement type checking for `WaitForRequestStatement`, `StopAcceptingConnectionsStatement`, and `CloseServerStatement`, enforcing that `server` expressions are `Text` and `timeout` expressions are `Number`.\n- Add unit tests for both web server statements and pattern definitions to prevent regressions and verify correct behavior. Co-authored-by: logbie <1138960+logbie@users.noreply.github.com> * Fix cargo fmt errors in tests\n\nRun cargo fmt to ensure `src/typechecker/mod.rs` passes formatting checks. Co-authored-by: logbie <1138960+logbie@users.noreply.github.com> * Address PR comments on typechecker implementation\n\n- Fix lexical scoping for pattern list references to support action scope.\n- Enforce pattern list element-type constraints (`List<Text>`, `Unknown`, or `Any`).\n- Refactor `WaitForRequestStatement`, `StopAcceptingConnectionsStatement`, and `CloseServerStatement` to use `check_server_expression_type` helper. Co-authored-by: logbie <1138960+logbie@users.noreply.github.com> * Fix pattern list-reference lexical scoping and add constraint tests\n\n- Move pattern list-reference existence check from `TypeChecker` to `Analyzer` so local scope variables inside actions are resolved before `Analyzer` scopes are popped.\n- Ensure `TypeChecker` no longer flags valid local list refs as undefined due to popped `Analyzer` scopes, relying on `Analyzer` for semantic validation.\n- Add explicit tests ensuring `List<Text>` pattern references pass while `List<Number>` references fail. Co-authored-by: logbie <1138960+logbie@users.noreply.github.com> * Align list-reference scoping with standard variable rules\n\n- Fix regression of #382 parameter logic and version metadata from outdated branch sync.\n- Replace ad-hoc local scope fallback in `TypeChecker::check_pattern_expression_types` with standard `self.infer_expression_type(&Expression::Variable(name))`, ensuring perfectly aligned semantics between `ListReference` lookups and ordinary variables.\n- Remove temporary test fixture files. Co-authored-by: logbie <1138960+logbie@users.noreply.github.com> * Enforce strict type constraints in type checker logic\n\n- Remove `Type::Unknown` and `Type::Error` fallbacks from `TypeChecker`'s pattern list references, `server`, and `timeout` expressions to ensure strict constraint enforcement for unresolved identifiers.\n- Remove temporary exploratory test scripts. Co-authored-by: logbie <1138960+logbie@users.noreply.github.com> --------- Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com>
Added
infer_expression_typetoAnalyzerand wired it into theExpression::ActionCallevaluation insideanalyze_expressionto statically validate argument types against function parameter signatures. Also accounts for parsed types being uppercaseCustom("Text")instead ofType::Textnatively due toparsermodule mechanics.A new test
test_action_call_type_validationensures mismatched types correctly issue aSemanticError.PR created automatically by Jules for task 11000740612994387411 started by @logbie
Summary by CodeRabbit