Skip to content

Argument validation in template names doesn't recurse into nested call expressions #78

@crhntr

Description

@crhntr

Summary

Argument-level validations in internal/generate/routes.go::appendParseArgumentStatements only inspect the immediate call.Args list. When an argument is a nested *ast.CallExpr (e.g. Helper(form) passed as a positional argument to the receiver method), identifiers inside that nested call are not subject to the same cross-argument constraints.

Concrete case: form / multipart mutual exclusion

appendParseArgumentStatements (after #77) rejects template names that pass both form and multipart as call arguments — but only at the top level:

hasForm, hasMultipart := false, false
for _, a := range call.Args {
    if id, ok := a.(*ast.Ident); ok {
        switch id.Name {
        case muxt.TemplateNameScopeIdentifierForm:
            hasForm = true
        case muxt.TemplateNameScopeIdentifierMultipart:
            hasMultipart = true
        }
    }
}
if hasForm && hasMultipart { ... }

The following template name slips past this check:

{{define "POST /upload Upload(form, Helper(multipart))"}}

appendParseArgumentStatements recurses on the nested Helper(multipart) call (existing behavior at the arg switch), so both request.ParseForm() and request.ParseMultipartForm(...) end up emitted in the same handler. In practice these can co-exist when only one body type matches the request's Content-Type, but the user's intent is muddled and the generated handler does redundant work.

Why it slipped through

  • checkArguments in internal/muxt/definition.go already recurses on nested CallExpr to detect unknown identifiers (line ~334).
  • The mutual-exclusion check added in feat: add multipart call-site identifier #77 lives in internal/generate/routes.go and was written for a single-pass scan of the top-level args.
  • Other future cross-argument constraints (e.g. "identifier X requires identifier Y", "at most one of A/B/C") would face the same issue.

Proposed approach

Extract a shared helper that walks all identifier arguments in a call tree:

func walkIdentArgs(call *ast.CallExpr, visit func(*ast.Ident)) {
    for _, a := range call.Args {
        switch e := a.(type) {
        case *ast.Ident:
            visit(e)
        case *ast.CallExpr:
            walkIdentArgs(e, visit)
        }
    }
}

…then implement the form/multipart check (and any future identifier-pair constraint) as a single pass with this walker. The existing recursion in appendParseArgumentStatements already mirrors this shape — moving the check to use the same traversal would close the gap.

Severity

Low. Requires deliberately unusual template syntax (form and multipart in different positions of the same call tree), and the runtime impact is "extra work" rather than incorrect parsing — ParseForm/ParseMultipartForm only consume the body when the Content-Type matches their respective formats. Worth fixing for clarity, not urgency.

Acceptance criteria

  • Add a recursive walker over identifier arguments in a call tree.
  • form + multipart rejection covers nested calls.
  • New err_* testdata case asserting the rejection (e.g. err_multipart_with_form_in_nested_call.txt).
  • Consider whether other identifier-pair constraints should adopt the same walker.

Surfaced during the #77 review.

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't workingenhancementNew feature or requestgoPull requests that update go code

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions