diff --git a/CLAUDE.md b/CLAUDE.md index d24979c..c7a34e5 100644 --- a/CLAUDE.md +++ b/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). diff --git a/include/openscad_cpp_evaluator/dispatch.hpp b/include/openscad_cpp_evaluator/dispatch.hpp index 850d8c4..9ea63e0 100644 --- a/include/openscad_cpp_evaluator/dispatch.hpp +++ b/include/openscad_cpp_evaluator/dispatch.hpp @@ -54,6 +54,25 @@ using GenerateFn = std::vector (*)(Evaluator&, const CSGParams&, co const std::unordered_map& resolveDispatch(); const std::unordered_map& 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* 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 diff --git a/include/openscad_cpp_evaluator/eval_error.hpp b/include/openscad_cpp_evaluator/eval_error.hpp index eca01d4..5d57723 100644 --- a/include/openscad_cpp_evaluator/eval_error.hpp +++ b/include/openscad_cpp_evaluator/eval_error.hpp @@ -2,12 +2,15 @@ #include "openscad_cpp_parser/position.hpp" +#include #include #include #include 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& 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>& 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& params, + const std::vector>& arguments); + } // namespace oscadeval diff --git a/pyproject.toml b/pyproject.toml index 69c7b39..99411e2 100644 --- a/pyproject.toml +++ b/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" diff --git a/src/builtins/function_builtins.cpp b/src/builtins/function_builtins.cpp index 9393573..95a02a5 100644 --- a/src/builtins/function_builtins.cpp +++ b/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* 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 positional = allPositional(args); if (positional.size() < arityIt->second) return Value{}; diff --git a/src/builtins/registry.cpp b/src/builtins/registry.cpp index 769fea6..438d343 100644 --- a/src/builtins/registry.cpp +++ b/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& generateDispatch() { return table; } +const std::vector* 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> 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(callNode); + if (const std::vector* declared = builtinParamNames(call.name->name)) { + warnUnexpectedArgs(ev, *declared, call.arguments); + } +} + } // namespace oscadeval diff --git a/src/bytecode_vm.cpp b/src/bytecode_vm.cpp index f957e3a..dc1acec 100644 --- a/src/bytecode_vm.cpp +++ b/src/bytecode_vm.cpp @@ -40,18 +40,25 @@ CallArgs buildCallArgs(const CompiledChunk::CallSite& site, std::vector& // 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& args, size_t argCount, +BoundArgs buildBoundArgs(Evaluator& ev, const CompiledChunk::CallSite& site, std::vector& args, size_t argCount, const std::vector>& 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(*argPtr); Value v = ev.evalExpr(*a.expr, callerCtx); @@ -137,6 +147,8 @@ void bindAstArgsIntoFrame(Evaluator& ev, const CompiledChunk& chunk, frame.slots[static_cast(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(&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(&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 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;` diff --git a/src/csg_resolve.cpp b/src/csg_resolve.cpp index 3279c6c..06861bd 100644 --- a/src/csg_resolve.cpp +++ b/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(*userModuleDecl), node, ctx); diff --git a/src/eval_error.cpp b/src/eval_error.cpp index d016f3a..ffaaebf 100644 --- a/src/eval_error.cpp +++ b/src/eval_error.cpp @@ -2,6 +2,8 @@ #include "openscad_cpp_evaluator/evaluator.hpp" +#include + namespace oscadeval { void Evaluator::error(const std::string& msg, const oscad::ASTNode& node, const std::string& innermostFrame) { @@ -122,4 +124,39 @@ std::string formatWarning(const std::string& msg, const oscad::Position* nodePos return full; } +bool isConfigVariable(const std::string& name) { + return !name.empty() && name[0] == '$' && name != "$children"; +} + +bool declaresParam(const std::vector>& params, const std::string& name) { + for (const auto& p : params) { + if (p->name->name == name) return true; + } + return false; +} + +void warnUnexpectedNamedArg(Evaluator& ev, const std::string& name, const oscad::Position* pos) { + ev.warn("variable " + name + " not specified as parameter", pos); +} + +void warnTooManyPositionalArgs(Evaluator& ev, const oscad::Position* pos) { + ev.warn("Too many unnamed arguments supplied", pos); +} + +void warnUnexpectedArgs(Evaluator& ev, const std::vector& params, + const std::vector>& arguments) { + size_t positionalIdx = 0; + for (const auto& argPtr : arguments) { + if (argPtr->kind() == oscad::NodeKind::NamedArgument) { + const std::string& name = static_cast(*argPtr).name->name; + if (!isConfigVariable(name) && + std::find(params.begin(), params.end(), name) == params.end()) { + warnUnexpectedNamedArg(ev, name, &argPtr->position()); + } + } else if (positionalIdx++ == params.size()) { + warnTooManyPositionalArgs(ev, &argPtr->position()); + } + } +} + } // namespace oscadeval diff --git a/src/user_calls.cpp b/src/user_calls.cpp index 12eda53..ffd6a47 100644 --- a/src/user_calls.cpp +++ b/src/user_calls.cpp @@ -191,11 +191,18 @@ BoundArgs Evaluator::bindArgs(const std::vectorkind() == oscad::NodeKind::NamedArgument) { auto& a = static_cast(*argPtr); - result.set(a.name->name, evalExprMaybeCompiled(*a.expr, ctx)); + const std::string& name = a.name->name; + if (!isConfigVariable(name) && !declaresParam(params, name)) { + warnUnexpectedNamedArg(*this, name, &argPtr->position()); + } + result.set(name, evalExprMaybeCompiled(*a.expr, ctx)); } else { auto& a = static_cast(*argPtr); if (positionalIdx < nparams) { result.set(params[positionalIdx]->name->name, evalExprMaybeCompiled(*a.expr, ctx)); + } else if (positionalIdx == nparams) { + // Once per call, like the reference's warned_for_extra_arguments. + warnTooManyPositionalArgs(*this, &argPtr->position()); } ++positionalIdx; } diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 81e6d41..03b90dc 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -35,6 +35,7 @@ add_executable(oscad_eval_tests test_debug_hooks.cpp test_multi_color_merge.cpp test_viewport_params.cpp + test_unexpected_args.cpp test_cli.cpp ) target_link_libraries(oscad_eval_tests PRIVATE openscad_cpp_evaluator oscad_cli_lib GTest::gtest_main) diff --git a/tests/test_unexpected_args.cpp b/tests/test_unexpected_args.cpp new file mode 100644 index 0000000..949e9ff --- /dev/null +++ b/tests/test_unexpected_args.cpp @@ -0,0 +1,151 @@ +#include "openscad_cpp_evaluator/evaluator.hpp" + +#include "test_helpers.hpp" + +#include + +// 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. Every expectation below was checked +// against a real OpenSCAD 2022.08.22 run of the same script -- same message +// text, same set of calls warned about. +// +// Every case runs twice, VM off and VM on: user calls bind through +// bindArgs (interpreter) or buildBoundArgs/bindAstArgsIntoFrame (VM), and +// the transform/color/hull/extrude/CSG builtins bypass evalModularCall +// entirely once compiled (Op::PushBuiltinWrap/Op::PushCsgWrap). Any of those +// paths missing the check is exactly the bug this file exists to catch. +using namespace oscadeval; +using namespace oscadeval::test; + +namespace { + +class ScopedVm { +public: + explicit ScopedVm(bool enabled) { Evaluator::setBytecodeVmEnabledForTesting(enabled); } + ~ScopedVm() { Evaluator::setBytecodeVmEnabledForTesting(std::nullopt); } +}; + +std::vector warningsFor(const std::string& code) { + std::vector out; + Evaluated e = evalSrc(code, [&](const std::string& msg) { + if (msg.rfind("WARNING: ", 0) == 0) out.push_back(msg); + }); + (void)e; + return out; +} + +// Every warning line, both VM settings, asserted to be identical between +// them -- the two binding paths must not disagree about what is unexpected. +std::vector warningsBothPaths(const std::string& code) { + std::vector interpreted; + { + ScopedVm vm(false); + interpreted = warningsFor(code); + } + std::vector compiled; + { + ScopedVm vm(true); + compiled = warningsFor(code); + } + EXPECT_EQ(interpreted, compiled) << "VM-on and VM-off disagree for: " << code; + return interpreted; +} + +// True if some warning contains `needle`. +bool has(const std::vector& warnings, const std::string& needle) { + for (const std::string& w : warnings) { + if (w.find(needle) != std::string::npos) return true; + } + return false; +} + +} // namespace + +TEST(UnexpectedArgs, UserModuleNamedArgNotAParameter) { + auto w = warningsBothPaths("module m(a=1) { } m(a=2, b=3);"); + ASSERT_EQ(w.size(), 1u); + EXPECT_NE(w[0].find("WARNING: variable b not specified as parameter"), std::string::npos); +} + +TEST(UnexpectedArgs, UserFunctionNamedArgNotAParameter) { + auto w = warningsBothPaths("function f(x=1) = x; echo(f(x=2, y=3));"); + ASSERT_EQ(w.size(), 1u); + EXPECT_NE(w[0].find("WARNING: variable y not specified as parameter"), std::string::npos); +} + +// A call from inside another function is the case the bytecode VM's own +// Op::CallFn path (buildBoundArgs) handles, distinct from the top-level +// entry (bindAstArgsIntoFrame). +TEST(UnexpectedArgs, NestedUserFunctionCallStillWarns) { + auto w = warningsBothPaths("function inner(n=1) = n; function outer(k) = inner(n=k, bogus=1); echo(outer(2));"); + ASSERT_EQ(w.size(), 1u); + EXPECT_NE(w[0].find("WARNING: variable bogus not specified as parameter"), std::string::npos); +} + +TEST(UnexpectedArgs, FunctionLiteralWarnsToo) { + auto w = warningsBothPaths("g = function(p) p * 2; echo(g(3, extra=9));"); + ASSERT_EQ(w.size(), 1u); + EXPECT_NE(w[0].find("WARNING: variable extra not specified as parameter"), std::string::npos); +} + +TEST(UnexpectedArgs, TooManyPositionalArgsWarnsExactlyOnce) { + // One warning for the whole call, however many extra arguments -- the + // reference's own warned_for_extra_arguments latch. + auto w = warningsBothPaths("module m(a) { } m(1, 2, 3);"); + ASSERT_EQ(w.size(), 1u); + EXPECT_NE(w[0].find("WARNING: Too many unnamed arguments supplied"), std::string::npos); +} + +TEST(UnexpectedArgs, DollarVariablesAreNeverUnexpected) { + // ContextFrame::is_config_variable: a $-name is a dynamic-scope + // override, not a parameter, so passing one to anything is fine. + EXPECT_TRUE(warningsBothPaths("module m(a=1) { } m(a=2, $custom=3, $fn=8);").empty()); + EXPECT_TRUE(warningsBothPaths("function f(x=1) = x; echo(f(x=2, $custom=3));").empty()); + EXPECT_TRUE(warningsBothPaths("sphere(r=1, $fn=8);").empty()); +} + +TEST(UnexpectedArgs, DeclaredParametersNeverWarn) { + EXPECT_TRUE(warningsBothPaths("module m(a, b=2) { } m(1, b=3);").empty()); + EXPECT_TRUE(warningsBothPaths("cylinder(h=2, r1=1, r2=0, center=true);").empty()); + EXPECT_TRUE(warningsBothPaths("linear_extrude(height=1, twist=10, convexity=2) square(1);").empty()); +} + +TEST(UnexpectedArgs, BuiltinPrimitiveWarns) { + auto w = warningsBothPaths("cube(size=2, bogus=9);"); + ASSERT_EQ(w.size(), 1u); + EXPECT_NE(w[0].find("WARNING: variable bogus not specified as parameter"), std::string::npos); +} + +// The transform/color/hull/CSG family never reaches evalModularCall once +// the bytecode compiler turns it into Op::PushBuiltinWrap/Op::PushCsgWrap +// -- the whole point of running these VM-on as well. +TEST(UnexpectedArgs, BuiltinsWithChildrenWarnOnBothPaths) { + EXPECT_TRUE(has(warningsBothPaths("translate([1,0,0], junk=1) cube(1);"), "variable junk")); + EXPECT_TRUE(has(warningsBothPaths("color(\"red\", junk=1) cube(1);"), "variable junk")); + EXPECT_TRUE(has(warningsBothPaths("hull(junk=1) cube(1);"), "variable junk")); + EXPECT_TRUE(has(warningsBothPaths("union(junk=1) cube(1);"), "variable junk")); + EXPECT_TRUE(has(warningsBothPaths("difference(junk=1) { cube(1); }"), "variable junk")); + EXPECT_TRUE(has(warningsBothPaths("offset(r=1, junk=1) square(1);"), "variable junk")); +} + +// The `#`/`%`/`!` modifiers share BuiltinWrapSite with the transforms but +// carry no argument list at all -- must not trip the ModularCall cast. +TEST(UnexpectedArgs, ModifiersAreNotCalls) { + EXPECT_TRUE(warningsBothPaths("#translate([1,0,0]) cube(1);").empty()); + EXPECT_TRUE(warningsBothPaths("%cube(1);").empty()); +} + +// Real OpenSCAD's builtin FUNCTIONS read their arguments positionally +// without Parameters::parse, so they warn about nothing -- verified against +// 2022.08.22, where both `sin(x=30)` and `sin(bogus=30)` return 0.5 in +// silence. textmetrics/fontmetrics are the exceptions that do parse. +TEST(UnexpectedArgs, BuiltinFunctionsDoNotWarn) { + EXPECT_TRUE(warningsBothPaths("echo(sin(bogus=30));").empty()); + EXPECT_TRUE(warningsBothPaths("echo(len(bogus=[1,2]));").empty()); +} + +TEST(UnexpectedArgs, TextMetricsWarns) { + auto w = warningsBothPaths("echo(textmetrics(text=\"hi\", size=5, nope=1));"); + EXPECT_TRUE(has(w, "variable nope not specified as parameter")); +}