Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -232,6 +232,8 @@ lazy_static! {
E129, Error, include_str!("./error_codes/E129.md"), // Direct interface reference call
E130, Error, include_str!("./error_codes/E130.md"), // Array size exceeds supported limit
E117, Error, include_str!("./error_codes/E117.md"), // Non-constant array boundary
E131, Error, include_str!("./error_codes/E131.md"), // Positional argument collides with later named argument
E132, Warning, include_str!("./error_codes/E132.md"), // Mixing implicit and explicit call parameters
);
}

Expand Down
25 changes: 25 additions & 0 deletions compiler/plc_diagnostics/src/diagnostics/error_codes/E131.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
# Positional argument collides with later named argument

In a mixed implicit/explicit call, a positional argument would naturally fill the same
parameter that a *later* named argument also targets. The positional would spill into
the next free slot - a silent reinterpretation that is almost never what the caller
intended. This is an error because no legitimate code should depend on that
disambiguation.

```iecst
FUNCTION myfunc : INT
VAR_INPUT
a : INT;
b : INT;
END_VAR
END_FUNCTION

PROGRAM main
VAR x : INT; END_VAR
// `1` sits in position 0 (param `a`), but `a := 10` also targets `a`.
x := myfunc(1, a := 10);
END_PROGRAM
```

Rewrite the call so the intent is unambiguous - either name both arguments or drop the
duplicate name.
40 changes: 40 additions & 0 deletions compiler/plc_diagnostics/src/diagnostics/error_codes/E132.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
# Mixing implicit and explicit call parameters

A call mixes positional (implicit) arguments with named (explicit) arguments.
The compiler accepts this, but the resolution is order-sensitive: named args
first claim their slots, and any remaining positional args fill the leftover
slots left-to-right. This is easy to misread and easy to break by reordering.

Example:

```st
FUNCTION myfunc : INT
VAR_INPUT
a : INT;
b : INT;
c : INT;
END_VAR
END_FUNCTION

PROGRAM main
VAR x : INT; END_VAR
// `b := 20` claims slot 1; positional `1` fills slot 0 (a), `3` fills slot 2 (c).
x := myfunc(b := 20, 1, 3);
END_PROGRAM
```

Prefer one style per call - fully positional or fully named - so a reader can
see the mapping at a glance. If a mix is genuinely the clearest option (e.g. to
override one default in the middle of a long parameter list), name every
argument whose position isn't obvious.

To silence this warning, rewrite the call with a single convention:

```st
x := myfunc(1, 20, 3); // all positional
x := myfunc(a := 1, b := 20, c := 3); // all named
```

See also `E131` — positional arg collides with later named arg — which catches
the more dangerous case where a positional argument's natural slot is also
named, and the resolver silently shifts it to a different parameter.
75 changes: 61 additions & 14 deletions src/codegen/generators/expression_generator.rs
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Idle thought, we can make our life much easier if the call lowerer would solve this:

  • Re-order the paramters,
  • Make everything either formal or not
  • Fill in missing parameters with the default value
    But we need to make sure we still hit the validation.

not saying you should do it now, just thought it might make it easier

Original file line number Diff line number Diff line change
Expand Up @@ -701,27 +701,29 @@ impl<'ink, 'b> ExpressionCodeGenerator<'ink, 'b> {
.get(index)
.is_some_and(|var_index_entry| var_index_entry.get_variable_type().is_output())
}
// else, the parameters are named and may not be in order, so we need to retrieve the information about the parameter from the assignment
// else, the parameters are named (or mixed) and may not be in order
else {
// Now we can fetch the identifier for the assignment statement and extract the name
let variable_name = match assignment_statement.get_stmt() {
match assignment_statement.get_stmt() {
AstStatement::OutputAssignment(assignment) | AstStatement::Assignment(assignment) => {
if let Some(identifier) = assignment.left.get_identifier() {
let variable_name = if let Some(identifier) = assignment.left.get_identifier() {
match identifier.get_stmt() {
AstStatement::Identifier(value) => value,
_ => unreachable!("this is definitely an identifier"),
}
} else {
unreachable!("assignment must have an identifier")
}
};
pou_info
.iter()
.find(|variable_index_entry| variable_index_entry.get_name() == variable_name)
.is_some_and(|var_index_entry| var_index_entry.get_variable_type().is_output())
}
_ => unreachable!("non-implicit statement must be an assignment"),
};

pou_info
.iter()
.find(|variable_index_entry| variable_index_entry.get_name() == variable_name)
.is_some_and(|var_index_entry| var_index_entry.get_variable_type().is_output())
// Reached only in mixed calls: `implicit` tracks the first arg's style,
// so an `Assignment`/`OutputAssignment` first arg makes `implicit = false`
// and any later positional arg lands here. Outputs are written post-call
// only for `=>`, so a bare positional never triggers the copy.
_ => false,
}
};

if assignment_statement.is_output_assignment() || (implicit && is_output) {
Expand Down Expand Up @@ -1097,8 +1099,53 @@ impl<'ink, 'b> ExpressionCodeGenerator<'ink, 'b> {
let mut result = Vec::new();
let mut variadic_parameters = Vec::new();
let mut passed_param_indices = Vec::new();
for (i, argument) in arguments.iter().enumerate() {
let (i, argument, _) = get_implicit_call_parameter(argument, &declared_parameters, i)?;

// Mixed-call resolution: named args claim their slots first, then positional args
// fill the remaining slots in declaration order. **This rule must match
// `TypeAnnotator::annotate_arguments_mixed` in the resolver** — the resolver uses it
// for type hinting, we use it here to compute the LLVM argument index.
//
// Why duplicated? The function-call codegen path threads args through
// `get_implicit_call_parameter`, which was designed for pure-positional / pure-named
// calls and expects the caller to pre-compute the slot. FB/PROGRAM calls use the
// resolver's `Argument` hint directly and don't need this.
let is_mixed = arguments.iter().any(|a| a.is_assignment() || a.is_output_assignment())
&& arguments.iter().any(|a| !a.is_assignment() && !a.is_output_assignment());
let named_positions: Vec<usize> = if is_mixed {
arguments
.iter()
.filter_map(|arg| match arg.get_stmt() {
AstStatement::Assignment(data) | AstStatement::OutputAssignment(data) => {
let name = data.left.get_flat_reference_name()?;
declared_parameters.iter().position(|p| p.get_name().eq_ignore_ascii_case(name))
}
_ => None,
})
.collect()
} else {
vec![]
};
let positional_positions: Vec<usize> =
(0..declared_parameters.len()).filter(|i| !named_positions.contains(i)).collect();
let mut positional_cursor = 0usize;

for (arg_idx, orig_argument) in arguments.iter().enumerate() {
// For mixed calls, positional args are remapped to the next free slot; named args
// and all args in pure calls keep their natural call index.
//
// The `unwrap_or(arg_idx)` fallback fires for variadic overflow: surplus args past
// the declared params carry their call index through to be collected as variadics
// below (where `declared_parameters.get(i) == None` → push to `variadic_parameters`).
let effective_idx =
if is_mixed && !orig_argument.is_assignment() && !orig_argument.is_output_assignment() {
let idx = positional_positions.get(positional_cursor).copied().unwrap_or(arg_idx);
positional_cursor += 1;
idx
} else {
arg_idx
};
let (i, argument, _) =
get_implicit_call_parameter(orig_argument, &declared_parameters, effective_idx)?;
let argument_is_reference_to = self.is_member_reference_to_reference_to(argument);

// parameter_info includes the declaration type and type name
Expand Down
94 changes: 93 additions & 1 deletion src/resolver.rs
Original file line number Diff line number Diff line change
Expand Up @@ -308,7 +308,16 @@ impl TypeAnnotator<'_> {
implementation.map(|it| it.get_type_name().to_string()).unwrap_or(name)
};

let generics = if arguments.iter().any(|arg| arg.is_assignment() | arg.is_output_assignment()) {
// Three variants, not one generalized routine: each has a distinct param-lookup
// strategy that doesn't compose cleanly in a single loop.
// - all-positional: zip params with args in order (no per-arg name lookup)
// - all-named: resolve each arg by name (ignoring declaration order)
// - mixed: named args claim slots first, positional args fill the gaps
let has_named = arguments.iter().any(|arg| arg.is_assignment() || arg.is_output_assignment());
let has_positional = arguments.iter().any(|arg| !arg.is_assignment() && !arg.is_output_assignment());
let generics = if has_named && has_positional {
self.annotate_arguments_mixed(&pou_name, arguments)
} else if has_named {
self.annotate_arguments_named(&pou_name, arguments)
} else {
self.annotate_arguments_positional(&pou_name, operator, arguments)
Expand All @@ -317,6 +326,89 @@ impl TypeAnnotator<'_> {
self.update_generic_call_statement(generics, &pou_name, operator, arguments_node, ctx.to_owned());
}

/// Annotate call arguments when the caller mixes positional and named args,
/// e.g. `foo(1, b := 20)` or `foo(a := 10, 2, c := 30)`.
///
/// The resolution rule matches `generate_function_arguments` in the codegen: named args
/// claim their slots first, then positional args fill the remaining slots left-to-right.
/// **Both sides must stay in sync** — if this rule changes, update the codegen too.
///
/// Invariant: this is only called when the argument list contains AT LEAST one named and
/// one positional arg; otherwise `annotate_arguments_named` / `annotate_arguments_positional`
/// handle the pure cases.
fn annotate_arguments_mixed(
&mut self,
pou_name: &str,
arguments: Vec<&AstNode>,
) -> FxHashMap<String, Vec<String>> {
let mut generics_candidates = FxHashMap::<String, Vec<String>>::default();
let parameters = self.index.get_available_parameters(pou_name);

// Slots that named args have already claimed — positional fill skips these.
let named_positions: FxHashSet<usize> = arguments
.iter()
.filter_map(|arg| {
let var_name = arg.get_assignment_identifier()?;
parameters.iter().position(|p| p.get_name().eq_ignore_ascii_case(var_name))
})
.collect();

// Remaining slots in declaration order — consumed by positional args one by one.
let positional_positions: Vec<usize> =
(0..parameters.len()).filter(|i| !named_positions.contains(i)).collect();
let mut positional_cursor = 0;

for argument in arguments {
if let Some(var_name) = argument.get_assignment_identifier() {
// Named arg (`x := value` or `x => value`): resolve by name, honouring
// inheritance (`find_pou_member_and_depth` walks the supertype chain).
let Some((parameter, depth)) =
TypeAnnotator::find_pou_member_and_depth(self.index, pou_name, var_name)
else {
continue;
};

if let Some((key, candidate)) =
self.get_generic_candidate(parameter.get_type_name(), argument)
{
generics_candidates.entry(key.to_string()).or_default().push(candidate.to_string());
} else {
self.annotate_argument(
parameter.get_qualifier().expect("parameter must have a qualifier"),
argument,
parameter.get_type_name(),
depth,
parameter.get_location_in_parent() as usize,
);
}
} else {
// Positional arg: consume the next free slot. Advance the cursor unconditionally
// so surplus positionals (beyond the declared params) are simply dropped — the
// arity check in the validator (E032) surfaces this as a user-facing error.
let pos = positional_positions.get(positional_cursor).copied();
positional_cursor += 1;
let Some(pos) = pos else { continue };

let Some(parameter) = parameters.get(pos) else { continue };
let type_name = parameter.get_type_name();

if let Some((key, candidate)) = self.get_generic_candidate(type_name, argument) {
generics_candidates.entry(key.to_string()).or_default().push(candidate.to_string());
} else {
self.annotate_argument(
pou_name,
argument,
type_name,
0,
parameter.get_location_in_parent() as usize,
);
}
}
}

generics_candidates
}

fn annotate_arguments_named(
&mut self,
pou_name: &str,
Expand Down
58 changes: 54 additions & 4 deletions src/validation/statement.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1892,12 +1892,12 @@ fn validate_call<T: AnnotationMap>(
}
}

// mixing implicit and explicit arguments is not allowed
// allways compare to the first argument
// mixing implicit and explicit arguments is discouraged
// always compare to the first argument
if arguments_are_implicit != is_implicit {
validator.push_diagnostic(
Diagnostic::new("Cannot mix implicit and explicit call parameters!")
.with_error_code("E031")
Diagnostic::new("Mixing implicit and explicit call parameters")
.with_error_code("E132")
.with_location(*argument),
);
}
Expand All @@ -1917,6 +1917,56 @@ fn validate_call<T: AnnotationMap>(
visit_statement(validator, argument, context);
}

// E131: flag positional args whose natural slot is also named by a LATER arg.
//
// Rule: a positional arg at call-index P collides if any named arg at call-index > P
// targets the slot P would naturally fill (= the number of positional args before it).
//
// Why only "later" named args? Because names that appear *before* a positional are how
// the caller reshuffles the default filling order — `myfunc(b := 20, 1, 3)` is a clear
// use of mixing where positional `1` cleanly takes slot 0 and `3` takes slot 2. That is
// not a collision; the user saw the name first and chose the positional below it.
//
// A name coming *after* the positional is different: the positional reads like it fills
// slot N, but the resolver actually spills it past the named slot. E.g. `myfunc(1, a := 10)`
// silently becomes `a := 10, b := 1`. Almost always a mistake — hence error, not warning.
let mut positional_ordinal = 0usize;
let positionals: Vec<(usize, usize)> = arguments
.iter()
.enumerate()
.filter_map(|(call_idx, arg)| {
if arg.is_assignment() || arg.is_output_assignment() {
return None;
}
let entry = (call_idx, positional_ordinal);
positional_ordinal += 1;
Some(entry)
})
.collect();
for (pos_call_idx, natural_slot) in positionals {
// Overflow past declared params — already flagged by the arity check (E032).
if natural_slot >= parameters.len() {
continue;
}
let colliding_name =
arguments.iter().enumerate().skip(pos_call_idx + 1).find_map(|(named_call_idx, named_arg)| {
let name = named_arg.get_assignment_identifier()?;
let named_slot = parameters.iter().position(|p| p.get_name().eq_ignore_ascii_case(name))?;
(named_slot == natural_slot).then_some(named_call_idx)
});
if let Some(named_call_idx) = colliding_name {
let param_name = parameters[natural_slot].get_name();
validator.push_diagnostic(
Diagnostic::new(format!(
"Positional argument collides with later named argument `{param_name}`"
))
.with_error_code("E131")
.with_location(arguments[pos_call_idx])
.with_secondary_location(arguments[named_call_idx]),
);
}
}

// for PROGRAM/FB we need special inout validation
if pou.is_stateful() || pou.is_method() {
// pou might actually be an action call: in that case,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,17 +2,17 @@
source: src/validation/tests/statement_validation_tests.rs
expression: "&diagnostics"
---
error[E031]: Cannot mix implicit and explicit call parameters!
warning[E132]: Mixing implicit and explicit call parameters
┌─ <internal>:23:34
23 │ foo(output1 => var1, var1, var1); // invalid cannot mix explicit and implicit
│ ^^^^ Cannot mix implicit and explicit call parameters!
│ ^^^^ Mixing implicit and explicit call parameters

error[E031]: Cannot mix implicit and explicit call parameters!
warning[E132]: Mixing implicit and explicit call parameters
┌─ <internal>:23:40
23 │ foo(output1 => var1, var1, var1); // invalid cannot mix explicit and implicit
│ ^^^^ Cannot mix implicit and explicit call parameters!
│ ^^^^ Mixing implicit and explicit call parameters

error[E037]: Invalid assignment: cannot assign 'STRING' to 'DINT'
┌─ <internal>:25:17
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,17 +2,17 @@
source: src/validation/tests/statement_validation_tests.rs
expression: "&diagnostics"
---
error[E031]: Cannot mix implicit and explicit call parameters!
warning[E132]: Mixing implicit and explicit call parameters
┌─ <internal>:23:35
23 │ prog(output1 => var1, var1, var1); // invalid cannot mix explicit and implicit
│ ^^^^ Cannot mix implicit and explicit call parameters!
│ ^^^^ Mixing implicit and explicit call parameters

error[E031]: Cannot mix implicit and explicit call parameters!
warning[E132]: Mixing implicit and explicit call parameters
┌─ <internal>:23:41
23 │ prog(output1 => var1, var1, var1); // invalid cannot mix explicit and implicit
│ ^^^^ Cannot mix implicit and explicit call parameters!
│ ^^^^ Mixing implicit and explicit call parameters

error[E037]: Invalid assignment: cannot assign 'STRING' to 'DINT'
┌─ <internal>:25:18
Expand Down
Loading
Loading