Warn about arguments the callee never declared

CLAUDE.md

@@ -424,7 +424,29 @@ grep for `ponytail:`.
   give sibling statements in the same block correct assignment visibility (see the header comment
   before reworking this; it's not an arbitrary choice).
 - `include/openscad_cpp_evaluator/eval_error.hpp`, `src/eval_error.cpp` — `EvalError` and the
-  exact ERROR/TRACE string formatting real OpenSCAD produces.
+  exact ERROR/TRACE string formatting real OpenSCAD produces. Also the **unexpected-argument
+  warnings** (`warnUnexpectedNamedArg`/`warnTooManyPositionalArgs`/`warnUnexpectedArgs`), the port
+  of real OpenSCAD's `Parameters.cc` `parse_without_defaults`: `variable X not specified as
+  parameter` for a named argument the callee doesn't declare, and `Too many unnamed arguments
+  supplied` once per call for positional arguments past the last parameter. `$`-prefixed names
+  other than `$children` are exempt (`isConfigVariable`, mirroring
+  `ContextFrame::is_config_variable` — a `$`-name is a dynamic-scope override, never a parameter).
+  The check has to be repeated at **every** argument-binding path, since there are five and none of
+  them share a choke point: `bindArgs` (interpreter, `user_calls.cpp`),
+  `buildBoundArgs`/`bindAstArgsIntoFrame` (bytecode VM, `bytecode_vm.cpp`), `evalModularCall`
+  (builtin modules, `csg_resolve.cpp`) and the VM's own `Op::PushBuiltinWrap`/`Op::PushCsgWrap`
+  handlers — the transform/color/hull/minkowski/render/extrude/projection/offset/roof/CSG family
+  bypasses `evalModularCall` entirely once compiled, which is exactly the gap
+  `tests/test_unexpected_args.cpp` runs every case twice (VM off and on) to catch. Builtin
+  parameter names live in `builtinParamNames` (`registry.cpp`): each list is the union of real
+  OpenSCAD's own `Parameters::parse` declaration and any extra name this port reads via `getArg`,
+  so the port never warns about an argument it goes on to honour — **add to it whenever a builtin
+  gains a parameter**, or that parameter starts warning. Builtin *functions* deliberately have no
+  entries beyond `textmetrics`/`fontmetrics`: upstream reads their arguments positionally without
+  `Parameters::parse`, so `sin(bogus=30)` warns about nothing there (verified against 2022.08.22)
+  and warning here would be a divergence. Not ported: the reference's `argument X supplied more
+  than once` / `argument X overrides positional argument`, separate conditions this port's simpler
+  positional-matching rule doesn't track.
 - `include/openscad_cpp_evaluator/evaluator.hpp`, `src/expr_eval.cpp`, `src/stmt_eval.cpp`,
   `src/user_calls.cpp` — the `Evaluator` class. Expression/statement dispatch is a `switch` on
   `NodeKind` (matches `openscad_cpp_parser`'s own dispatch convention, not a visitor).

include/openscad_cpp_evaluator/dispatch.hpp

@@ -54,6 +54,25 @@ using GenerateFn = std::vector<ColoredBody> (*)(Evaluator&, const CSGParams&, co
 const std::unordered_map<std::string_view, ResolveFn>& resolveDispatch();
 const std::unordered_map<std::string_view, GenerateFn>& generateDispatch();
 
+// Every parameter name a builtin accepts, for the unexpected-argument
+// warning (see eval_error.hpp). Returns nullptr for a name with no entry,
+// which suppresses the check rather than warning about everything.
+//
+// Each list is the UNION of real OpenSCAD's own Parameters::parse
+// declaration for that builtin (2022.08.22 -- the authority on what warns
+// upstream) and any extra name this port itself reads via getArg, so this
+// port never warns about an argument it goes on to honour.
+const std::vector<std::string>* builtinParamNames(const std::string& name);
+
+// builtinParamNames + warnUnexpectedArgs for one builtin module call. Must
+// be invoked from EVERY path a builtin call can take: evalModularCall
+// (interpreter, and Op::NativeStatement) plus the bytecode VM's own
+// Op::PushBuiltinWrap/Op::PushCsgWrap handlers, which bypass
+// evalModularCall entirely for the transform/color/hull/extrude/CSG family.
+// Silently does nothing for a node that isn't a ModularCall (a `#`/`%`/`!`
+// modifier shares BuiltinWrapSite but carries no arguments).
+void warnUnexpectedBuiltinArgs(Evaluator& ev, const oscad::ASTNode& callNode);
+
 // Concatenates every top-level node's already-generated `.bodies` (does NOT
 // recurse into `.children` -- a parent's `.bodies` is already the fully
 // combined result of its children). Mirrors the reference's

include/openscad_cpp_evaluator/eval_error.hpp

@@ -2,12 +2,15 @@
 
 #include "openscad_cpp_parser/position.hpp"
 
+#include <memory>
 #include <stdexcept>
 #include <string>
 #include <vector>
 
 namespace oscad {
 class ASTNode;
+class Argument;
+class ParameterDeclaration;
 } // namespace oscad
 
 namespace oscadeval {
@@ -129,4 +132,36 @@ std::string formatError(const std::string& msg, const oscad::Position* nodePosit
 std::string formatWarning(const std::string& msg, const oscad::Position* nodePosition,
                            const std::vector<CallStackFrame>& callStack);
 
+// -- Unexpected-argument warnings -----------------------------------------
+//
+// Real OpenSCAD (Parameters.cc's parse_without_defaults) warns for a named
+// argument the callee doesn't declare, and once per call for positional
+// arguments past the last parameter. Both are silently dropped otherwise,
+// which makes a typo'd argument name invisible -- the call still runs, the
+// parameter just keeps its default. Wording is verbatim from the reference
+// (2022.08.22, which quotes neither name nor message).
+//
+// NOT covered here (separate reference warnings, separate conditions):
+// "argument X supplied more than once" and "argument X overrides positional
+// argument".
+class Evaluator;
+
+// ContextFrame::is_config_variable: a $-prefixed name other than $children
+// is a dynamic-scope override, never a declared parameter, so it is exempt.
+bool isConfigVariable(const std::string& name);
+
+// Linear scan -- ponytail: parameter lists are single-digit in practice, so
+// a per-call set/map would cost more than it saves.
+bool declaresParam(const std::vector<std::unique_ptr<oscad::ParameterDeclaration>>& params, const std::string& name);
+
+void warnUnexpectedNamedArg(Evaluator& ev, const std::string& name, const oscad::Position* pos);
+void warnTooManyPositionalArgs(Evaluator& ev, const oscad::Position* pos);
+
+// Both warnings at once for a callee whose parameter names are a fixed list
+// rather than a declaration -- i.e. a builtin (see builtinParamNames,
+// dispatch.hpp). Reports against each argument's own position, which is the
+// call's own line in every case that isn't a multi-line argument list.
+void warnUnexpectedArgs(Evaluator& ev, const std::vector<std::string>& params,
+                         const std::vector<std::unique_ptr<oscad::Argument>>& arguments);
+
 } // namespace oscadeval

pyproject.toml

@@ -4,7 +4,7 @@ build-backend = "scikit_build_core.build"
 
 [project]
 name = "openscad_cpp_evaluator"
-version = "0.29.3"
+version = "0.30.0"
 description = "C++ OpenSCAD evaluator with Python bindings"
 readme = "README.md"
 requires-python = ">=3.12"

src/builtins/function_builtins.cpp

@@ -1,5 +1,6 @@
 #include "openscad_cpp_evaluator/function_builtins.hpp"
 
+#include "openscad_cpp_evaluator/dispatch.hpp"
 #include "openscad_cpp_evaluator/evaluator.hpp"
 #include "openscad_cpp_evaluator/segments.hpp"
 #include "openscad_cpp_evaluator/text_metrics.hpp"
@@ -509,6 +510,18 @@ Value evalBuiltinFunction(Evaluator& ev, const std::string& name, const CallArgs
     // elsewhere before reaching here) -- mirrors the old chain's fallthrough.
     if (idIt == ids.end()) return Value{};
 
+    // Only textmetrics/fontmetrics have an entry -- every other builtin
+    // function reads its arguments positionally upstream and warns about
+    // nothing. See builtinParamNames (registry.cpp).
+    if (const std::vector<std::string>* declared = builtinParamNames(name)) {
+        for (const auto& [argName, _] : args.named) {
+            if (!isConfigVariable(argName) &&
+                std::find(declared->begin(), declared->end(), argName) == declared->end()) {
+                warnUnexpectedNamedArg(ev, argName, &node.position());
+            }
+        }
+    }
+
     if (const auto arityIt = scalarNumericArity().find(name); arityIt != scalarNumericArity().end()) {
         const std::vector<Value> positional = allPositional(args);
         if (positional.size() < arityIt->second) return Value{};

src/builtins/registry.cpp

@@ -1,6 +1,7 @@
 #include "builtins.hpp"
 
 #include "openscad_cpp_evaluator/dispatch.hpp"
+#include "openscad_cpp_evaluator/eval_error.hpp"
 
 namespace oscadeval {
 
@@ -78,4 +79,64 @@ const std::unordered_map<std::string_view, GenerateFn>& generateDispatch() {
     return table;
 }
 
+const std::vector<std::string>* builtinParamNames(const std::string& name) {
+    // Deliberately absent: the builtin *functions* other than the three
+    // below. Real OpenSCAD's builtin functions read their arguments
+    // positionally without going through Parameters::parse, so `sin(bogus=30)`
+    // warns about nothing upstream -- verified against 2022.08.22 -- and
+    // warning here would be a divergence, not a fix.
+    static const std::unordered_map<std::string, std::vector<std::string>> table = {
+        // -- builtin modules ---------------------------------------------
+        {"cube", {"size", "center"}},
+        {"sphere", {"r", "d"}},
+        {"cylinder", {"h", "r1", "r2", "center", "r", "d", "d1", "d2"}},
+        {"polyhedron", {"points", "faces", "convexity", "triangles"}},
+        {"square", {"size", "center"}},
+        {"circle", {"r", "d"}},
+        {"polygon", {"points", "paths", "convexity"}},
+        {"translate", {"v"}},
+        {"rotate", {"a", "v"}},
+        {"scale", {"v"}},
+        {"mirror", {"v"}},
+        {"multmatrix", {"m"}},
+        {"resize", {"newsize", "auto", "convexity"}},
+        {"color", {"c", "alpha"}},
+        {"union", {}},
+        {"difference", {}},
+        {"intersection", {}},
+        {"hull", {}},
+        {"minkowski", {"convexity"}},
+        {"children", {"index"}},
+        {"render", {"convexity"}},
+        // "repair" is this port's own addition, not an upstream parameter.
+        {"import",
+          {"file", "layer", "convexity", "origin", "scale", "width", "height", "filename", "layername", "center", "dpi",
+           "id", "repair"}},
+        {"linear_extrude", {"height", "v", "scale", "center", "twist", "slices", "segments", "convexity"}},
+        {"rotate_extrude", {"angle", "start", "convexity"}},
+        {"projection", {"cut", "convexity"}},
+        {"roof", {"method", "convexity"}},
+        {"offset", {"r", "delta", "chamfer"}},
+        {"surface", {"file", "center", "convexity", "invert"}},
+        {"text", {"text", "size", "font", "direction", "language", "script", "halign", "valign", "spacing"}},
+        // breakpoint() is this port's own debugger extension, no upstream
+        // equivalent to mirror.
+        {"breakpoint", {"condition"}},
+
+        // -- the three builtin FUNCTIONS that do use Parameters::parse ----
+        {"textmetrics", {"text", "size", "font", "direction", "language", "script", "halign", "valign", "spacing"}},
+        {"fontmetrics", {"size", "font"}},
+    };
+    auto it = table.find(name);
+    return it == table.end() ? nullptr : &it->second;
+}
+
+void warnUnexpectedBuiltinArgs(Evaluator& ev, const oscad::ASTNode& callNode) {
+    if (callNode.kind() != oscad::NodeKind::ModularCall) return;
+    const auto& call = static_cast<const oscad::ModularCall&>(callNode);
+    if (const std::vector<std::string>* declared = builtinParamNames(call.name->name)) {
+        warnUnexpectedArgs(ev, *declared, call.arguments);
+    }
+}
+
 } // namespace oscadeval

src/bytecode_vm.cpp

@@ -40,18 +40,25 @@ CallArgs buildCallArgs(const CompiledChunk::CallSite& site, std::vector<Value>&
 // LIST is `paramNames` (in declared order -- a FunctionDeclaration's
 // parameters or a FunctionLiteral's). Shared by CallFn/CallFnTail (a
 // site.decl callee) and CallDynamic/CallDynamicTail (a closure callee).
-BoundArgs buildBoundArgs(const CompiledChunk::CallSite& site, std::vector<Value>& args, size_t argCount,
+BoundArgs buildBoundArgs(Evaluator& ev, const CompiledChunk::CallSite& site, std::vector<Value>& args, size_t argCount,
                           const std::vector<std::unique_ptr<oscad::ParameterDeclaration>>& paramNames) {
     BoundArgs bound;
     bound.reserve(argCount);
     size_t positionalIdx = 0;
     const size_t nparams = paramNames.size();
+    const oscad::Position* pos = site.callNode ? &site.callNode->position() : nullptr;
     for (size_t i = 0; i < argCount; ++i) {
         if (site.argNames[i]) {
-            bound.set(*site.argNames[i], std::move(args[i]));
+            const std::string& name = *site.argNames[i];
+            if (!isConfigVariable(name) && !declaresParam(paramNames, name)) {
+                warnUnexpectedNamedArg(ev, name, pos);
+            }
+            bound.set(name, std::move(args[i]));
         } else {
             if (positionalIdx < nparams) {
                 bound.set(paramNames[positionalIdx]->name->name, std::move(args[i]));
+            } else if (positionalIdx == nparams) {
+                warnTooManyPositionalArgs(ev, pos);
             }
             ++positionalIdx;
         }
@@ -126,6 +133,9 @@ void bindAstArgsIntoFrame(Evaluator& ev, const CompiledChunk& chunk,
             if (!matched && !name.empty() && name[0] == '$') {
                 ctx.dyn->set(name, v);
             }
+            if (!matched && !isConfigVariable(name)) {
+                warnUnexpectedNamedArg(ev, name, &argPtr->position());
+            }
         } else {
             auto& a = static_cast<const oscad::PositionalArgument&>(*argPtr);
             Value v = ev.evalExpr(*a.expr, callerCtx);
@@ -137,6 +147,8 @@ void bindAstArgsIntoFrame(Evaluator& ev, const CompiledChunk& chunk,
                     frame.slots[static_cast<size_t>(p.slot)] = std::move(v);
                 }
                 frame.bound[positionalIdx] = true;
+            } else if (positionalIdx == nparams) {
+                warnTooManyPositionalArgs(ev, &argPtr->position());
             }
             ++positionalIdx;
         }
@@ -770,7 +782,7 @@ Value driveVm(Evaluator& ev, size_t floor) {
                         f.stack.push_back(evalBuiltinFunction(ev, site.calleeName, callArgs, *site.callNode));
                         ++f.pc;
                     } else {
-                        BoundArgs bound = buildBoundArgs(site, args, argCount, site.decl->parameters);
+                        BoundArgs bound = buildBoundArgs(ev, site, args, argCount, site.decl->parameters);
                         const CompiledChunk* calleeChunk = ev.useBytecodeVm() ? ev.lookupOrCompileChunk(*site.decl) : nullptr;
                         if (calleeChunk) {
                             const oscad::Scope* fnScope = site.decl->scope() ? site.decl->scope() : ctx.scope;
@@ -801,7 +813,7 @@ Value driveVm(Evaluator& ev, size_t floor) {
                     if (const auto* closurePtr = std::get_if<ClosurePtr>(&callee); closurePtr && *closurePtr) {
                         const Closure& closure = **closurePtr;
                         const oscad::FunctionLiteral& funcNode = *closure.node;
-                        BoundArgs bound = buildBoundArgs(site, args, argCount, funcNode.parameters);
+                        BoundArgs bound = buildBoundArgs(ev, site, args, argCount, funcNode.parameters);
                         const CompiledChunk* calleeChunk = ev.useBytecodeVm() ? ev.lookupCompiledLiteralChunk(funcNode) : nullptr;
                         if (calleeChunk) {
                             const oscad::Scope* fnScope = funcNode.scope() ? funcNode.scope() : ctx.scope;
@@ -827,7 +839,7 @@ Value driveVm(Evaluator& ev, size_t floor) {
                         args[argCount - 1 - i] = std::move(f.stack.back());
                         f.stack.pop_back();
                     }
-                    BoundArgs bound = buildBoundArgs(site, args, argCount, site.decl->parameters);
+                    BoundArgs bound = buildBoundArgs(ev, site, args, argCount, site.decl->parameters);
                     // Tail-hop-in-place requires f.hopEligible -- a frame
                     // that's call-boundary-free (statement expression,
                     // assignment block, parameter default) has no
@@ -894,7 +906,7 @@ Value driveVm(Evaluator& ev, size_t floor) {
                     if (const auto* closurePtr = std::get_if<ClosurePtr>(&callee); closurePtr && *closurePtr) {
                         const Closure& closure = **closurePtr;
                         const oscad::FunctionLiteral& funcNode = *closure.node;
-                        BoundArgs bound = buildBoundArgs(site, args, argCount, funcNode.parameters);
+                        BoundArgs bound = buildBoundArgs(ev, site, args, argCount, funcNode.parameters);
                         const bool thisFrameBracketed = f.hopEligible;
                         std::optional<EvalContext> hopCtx = thisFrameBracketed
                                                                  ? ev.isolatedCallCtxFor(funcNode, ctx, capturedLetTrail(closure))
@@ -1055,6 +1067,7 @@ Value driveVm(Evaluator& ev, size_t floor) {
                     // uncacheable/ManifoldCache taint tracking for a
                     // rands() call embedded in the wrapper's own args).
                     const std::uint64_t randsBefore = ev.randsCallCount();
+                    warnUnexpectedBuiltinArgs(ev, *site.node);
                     CSGParams params;
                     CallArgs deferredArgs; // Roof only -- see PendingBuiltinWrap's own doc comment
                     switch (site.kind) {
@@ -1183,6 +1196,7 @@ Value driveVm(Evaluator& ev, size_t floor) {
                     // rands-in-args taint reasoning as Op::PushBuiltinWrap.
                     const std::uint64_t randsBefore = ev.randsCallCount();
                     if (site.hasArgs) {
+                        warnUnexpectedBuiltinArgs(ev, *site.node);
                         // union/difference/intersection take no positional
                         // arguments in real OpenSCAD -- `args` is discarded,
                         // exactly mirroring resolveCsg's own `(void)args;`

src/csg_resolve.cpp

@@ -94,6 +94,7 @@ void Evaluator::evalModularCall(const oscad::ModularCall& node, EvalContext& ctx
     CSGParams params;
     try {
         if (hasResolveFn) {
+            warnUnexpectedBuiltinArgs(*this, node);
             params = it->second(*this, node, ctx);
         } else if (!isBuiltin) {
             evalUserModule(static_cast<const oscad::ModuleDeclaration&>(*userModuleDecl), node, ctx);