Warn about arguments the callee never declared - #92
Merged
Conversation
Passing a named argument no parameter matches, or more positional arguments than there are parameters, was silently dropped: the call ran, the parameter kept its default, and a typo'd argument name was undetectable. Real OpenSCAD warns for both (Parameters.cc's parse_without_defaults), so this port now does too, with the same message text. The check has to go in five places, because none of the argument-binding paths share a choke point: bindArgs (interpreter), buildBoundArgs and bindAstArgsIntoFrame (bytecode VM), evalModularCall (builtin modules), and the VM's Op::PushBuiltinWrap/Op::PushCsgWrap handlers -- the transform/color/hull/extrude/CSG family never reaches evalModularCall once compiled. test_unexpected_args.cpp runs every case both VM-off and VM-on and asserts the two agree, which is what caught that last gap. $-prefixed names other than $children stay exempt, matching ContextFrame::is_config_variable. Builtin parameter names come from a table in registry.cpp -- the union of upstream's own Parameters::parse declarations and any extra name this port reads via getArg, so it never warns about an argument it goes on to honour. Builtin functions get no entries beyond textmetrics/fontmetrics, since upstream reads their arguments positionally and warns about nothing. Verified message-for-message against OpenSCAD 2022.08.22 on a script covering user modules, user functions, function literals, nested calls and every builtin group; and checked for false positives against the 901 scripts in BOSL2's own test suite, its examples, and Dalek.scad -- zero warnings from any of them. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Passing a named argument no parameter matches, or more positional arguments than there are parameters, was silently dropped: the call ran, the parameter kept its default, and a typo'd argument name was undetectable. Real OpenSCAD warns for both (
Parameters.cc'sparse_without_defaults), so this port now does too, with the same message text:Where the check goes
Five places, because none of the argument-binding paths share a choke point:
bindArgs(interpreter)user_calls.cppbuildBoundArgs,bindAstArgsIntoFrame(bytecode VM)bytecode_vm.cppevalModularCall(builtin modules)csg_resolve.cppOp::PushBuiltinWrap/Op::PushCsgWrapbytecode_vm.cppThat last row is the one that isn't obvious: the transform/color/hull/minkowski/render/extrude/projection/offset/roof/CSG family never reaches
evalModularCallonce the bytecode compiler turns it into a wrap op.tests/test_unexpected_args.cppruns every case twice — VM off and VM on — and asserts the two agree, which is what surfaced it.Rules ported
$-prefixed names other than$childrenare exempt (isConfigVariable, mirroringContextFrame::is_config_variable) — a$-name is a dynamic-scope override, never a parameter.warned_for_extra_argumentslatch.builtinParamNames(registry.cpp), each list the union of upstream's ownParameters::parsedeclaration and any extra name this port reads viagetArg, so it never warns about an argument it goes on to honour. Add to it whenever a builtin gains a parameter.textmetrics/fontmetrics: upstream reads their arguments positionally withoutParameters::parse, sosin(bogus=30)warns about nothing there (verified against 2022.08.22) and warning would be a divergence, not a fix.argument X supplied more than onceandargument X overrides positional argument, separate conditions this port's simpler positional-matching rule doesn't track.Verification
tests/*.scadtest, plus BOSL2'sexamples/, plusDalek.scad— zero unexpected-argument warnings from any of them.OSCAD_BYTECODE_VM=1.Dalek.scad.🤖 Generated with Claude Code