perf_lint: add PERF025 — redundant string() in string interpolation

daslib/perf_lint.das

@@ -36,6 +36,8 @@ module perf_lint shared private
 //!   PERF021 — cond ? T(a) : T(b) on workhorse cast T — hoist to T(cond ? a : b)
 //!   PERF022 — for (s in A) { B |> push(s) } / push_clone — use 'B |> push_from(A)' (bulk reserve+copy)
 //!   PERF023 — var X = clone_expression(E); ... $e(X) ... — qmacro splice already clones; drop the pre-clone
+//!   PERF024 — var X = clone_*(E); func(X) where func is [clone(...)]-annotated — callee clones; drop the pre-clone
+//!   PERF025 — string(...) inside string interpolation is redundant — interpolation already converts to string
 
 require daslib/ast_boost
 require strings
@@ -113,6 +115,14 @@ struct Perf023Candidate {
     disqualified : bool
 }
 
+enum private Perf025Kind {
+    // How a string()-cast value inside interpolation compares to direct
+    // interpolation, deciding whether/how PERF025 fires.
+    skip            // not flaggable: value-shape change, or not a string() cast
+    equivalent      // "{x}" == string(x): drop the cast
+    unsigned_hex    // "{x}" is hex but string(x) is decimal: drop + ':d' hint
+}
+
 class PerfLintVisitor : AstVisitor {
     compile_time_errors : bool
     // variable + loop tracking
@@ -564,6 +574,60 @@ class PerfLintVisitor : AstVisitor {
         }
     }
 
+    // --- PERF025: redundant string(...) inside string interpolation helpers ---
+
+    def perf025_value_kind(arg : Expression const?) : Perf025Kind {
+        //! Classifies a string() cast's value argument by how `"{x}"` (the string
+        //! builder's own conversion) compares to `string(x)`:
+        //!   equivalent   — identical output (drop the cast): signed ints, float,
+        //!                  double, string, das_string.
+        //!   unsigned_hex — decimal vs hex difference (drop, but suggest the ':d'
+        //!                  tag): the four unsigned int widths interpolate as hex.
+        //!   skip         — not flaggable: value-shape-changing conversions
+        //!                  (array<uint8>, uri, text_range, other handled types)
+        //!                  and anything else.
+        if (arg == null || arg._type == null || !empty(arg._type.dim)) return Perf025Kind.skip
+        let bt = arg._type.baseType
+        if (bt == Type.tInt || bt == Type.tInt8 || bt == Type.tInt16 || bt == Type.tInt64
+            || bt == Type.tFloat || bt == Type.tDouble || bt == Type.tString
+            || (bt == Type.tHandle && arg._type.annotation != null
+                && arg._type.annotation.name == "das_string")) return Perf025Kind.equivalent
+        if (bt == Type.tUInt || bt == Type.tUInt8 || bt == Type.tUInt16 || bt == Type.tUInt64) return Perf025Kind.unsigned_hex
+        return Perf025Kind.skip
+    }
+
+    def perf025_string_call_value(e : Expression const?) : Expression const? {
+        //! If `e` is a `string(value)` cast call, return its value argument
+        //! (arguments[0]). Skips an explicit `string(x, true)` hex request — the
+        //! second argument being a literal `true` means the user wants hex, which
+        //! no longer matches plain interpolation for signed types. The float /
+        //! double / string / das_string overloads have no hex param, so their
+        //! arguments[1] (a FakeContext) is not an ExprConstBool and passes through.
+        if (e == null || !(e is ExprCall)) return null
+        let c = e as ExprCall
+        if (c.func == null || empty(c.arguments)) return null
+        let fname = string(c.func.fromGeneric != null ? c.func.fromGeneric.name : c.func.name)
+        if (fname != "string") return null
+        if (length(c.arguments) >= 2) {
+            let a1 = c.arguments[1]
+            if (a1 != null && a1 is ExprConstBool && (a1 as ExprConstBool).value) return null
+        }
+        return c.arguments[0]
+    }
+
+    def perf025_unwrap_element(elem : Expression const?) : Expression const? {
+        //! Peels one ExprRef2Value off an interpolation element so a `string(...)`
+        //! call read through a value-context wrapper is still exposed. A format
+        //! spec (`"{x:fmt}"`) lowers to `_::fmt(":fmt", x)`, but `fmt` has no
+        //! string-valued overload, so `string(x)` can never sit under a fmt
+        //! wrapper (it would not compile) — no fmt unwrap is needed.
+        var ex = elem
+        if (ex != null && ex is ExprRef2Value) {
+            ex = (ex as ExprRef2Value.subexpr)
+        }
+        return ex
+    }
+
     // --- function tracking ---
 
     def override preVisitFunction(var fn : FunctionPtr) : void {
@@ -1640,7 +1704,7 @@ class PerfLintVisitor : AstVisitor {
         // Arm A: inner is itself a clone_* call → redundant.
         if (is_clone_init_call(inner)) {
             perf_warning(
-                "PERF024: argument to function annotated [clone({string(expr.func.arguments[arg_index].name)})] is wrapped in clone_*; the callee clones internally, drop the outer clone wrap",
+                "PERF024: argument to function annotated [clone({expr.func.arguments[arg_index].name})] is wrapped in clone_*; the callee clones internally, drop the outer clone wrap",
                 arg.at)
             return
         }
@@ -1796,6 +1860,20 @@ class PerfLintVisitor : AstVisitor {
         return <- expr
     }
 
+    // --- PERF025: redundant string(...) inside string interpolation ---
+
+    def override preVisitExprStringBuilderElement(sb : ExprStringBuilder?; elem : ExpressionPtr; last : bool) {
+        let inner = perf025_unwrap_element(elem)
+        let value = perf025_string_call_value(inner)
+        return if (value == null)
+        let kind = perf025_value_kind(value)
+        if (kind == Perf025Kind.equivalent) {
+            perf_warning("PERF025: string(...) inside string interpolation is redundant — the interpolation already converts to string; drop the cast", inner.at)
+        } elif (kind == Perf025Kind.unsigned_hex) {
+            perf_warning("PERF025: string(...) inside string interpolation is redundant; drop the cast — unsigned values interpolate as hex by default, so use the '\{x:d\}' format tag to keep decimal output", inner.at)
+        }
+    }
+
     // --- Central ExprVar handler (PERF002, PERF004, PERF005) ---
 
     def override preVisitExprVar(expr : ExprVar?) : void {

doc/source/reference/language/lint.rst

@@ -1012,6 +1012,58 @@ semantics while still letting ``apply_template`` produce N clones.
 ``clone_type`` call sites typically feed direct AST construction, not qmacro
 splices).
 
+PERF025 — redundant ``string(...)`` inside string interpolation
+================================================================
+
+String interpolation already converts every element to a string via the
+string builder's ``DebugDataWalker``. Wrapping an element in ``string(...)``
+allocates an intermediate string that the builder then copies — a wasted heap
+allocation per interpolation.
+
+.. code-block:: das
+
+    // Bad
+    def f(iv : int) : string {
+        return "{string(iv)}"                   // PERF025
+    }
+
+    // Good
+    def f(iv : int) : string {
+        return "{iv}"
+    }
+
+The rule fires only for **stringify-equivalent** value types — those where
+``"{x}"`` and ``string(x)`` produce identical text: the signed integer widths
+(``int`` / ``int8`` / ``int16`` / ``int64``), ``float``, ``double``,
+``string``, and ``das_string`` (e.g. ``"{string(fn.name)}"`` where ``fn.name``
+is a ``das_string``).
+
+Unsigned integers (``uint`` / ``uint8`` / ``uint16`` / ``uint64``) still warn,
+but with an extra note: they interpolate as **hex** by default
+(``"{42u}"`` → ``0x2a``), so dropping the cast changes the output. Use the
+``:d`` format tag to keep decimal:
+
+.. code-block:: das
+
+    // Bad — string() gives decimal "42"
+    return "{string(uv)}"                       // PERF025 + ':d' hint
+
+    // Good — same decimal output, no intermediate allocation
+    return "{uv:d}"
+
+The rule deliberately does NOT fire when dropping ``string(...)`` would change
+the **value shape**, not just the numeric format:
+
+- ``string(array<uint8>)`` reinterprets the bytes as text, whereas ``"{bytes}"``
+  prints the array structure.
+- ``string(uri)`` / ``string(text_range)`` likewise produce a different
+  representation than direct interpolation.
+
+It also does NOT fire when ``string(...)`` is nested as an argument to another
+call inside the braces (e.g. ``"{length(string(x))}"``) — only direct
+interpolation elements are flagged — nor for an explicit hex request
+``string(x, true)``.
+
 .. _style_lint:
 
 -----------

skills/perf_lint.md

@@ -129,6 +129,7 @@ After compilation, `Expression._type` is resolved. Check `expr._type.baseType ==
 | PERF022 | `for (s in A) { B \|> push(s) }` / `push_clone(s)` where body is exactly that one statement, single iter-var, source is `array<T>` / C-array, destination is `array<T>` | Medium | use `B \|> push_from(A)` / `push_clone_from(A)` (bulk reserve+copy in `daslib/builtin.das`). Single-name `push`/`push_clone` is overloaded between single-element and bulk forms (ambiguous when destination is `array<T[]>`); the `_from` suffix names the bulk intent. Warns at the for-loop's `.at` rather than the inner push's `.at` to avoid colliding with PERF006 under `perf_warning`'s same-location dedup. `emplace` is out of scope: for-iter-var is const-ref, but `emplace` requires var-ref, so the hand-rolled shape doesn't compile. Range/iterator/generator sources are not flagged — no bulk overload to migrate to |
 | PERF023 | `var X = clone_expression(E)` whose only uses are `$e(X)` splice tags inside `qmacro` / `qmacro_block` / `qmacro_expr` / `qmacro_block_to_array` bodies | Medium | drop the pre-clone, inline `$e(E)` at each splice site. `apply_template` (templates_boost.das:251) calls `clone_expression` on every substitution input, so the user-side pre-clone is wasted work. Detection: post-expansion, `$e(X)` becomes `add_ptr_ref(X)` inside an `ExprMakeBlock`; the visitor tracks `perf023_splice_depth++` on entry to any `add_ptr_ref` call and classifies each candidate's `ExprVar` reference as "safe" if `depth > 0` else "disqualified". Fires only when ALL uses are safe AND at least one is observed. Multi-clone-of-same-source is still flagged (e.g. `var a = clone_expression(takeExpr); var b = clone_expression(takeExpr)` for two `$e(...)` slots): inlining `$e(takeExpr)` at both slots preserves "N independent clones" semantics because apply_template clones each substitution. The lint runs unconditionally inside closures because the rules-block IS a closure (`apply_qrules` builds an `ExprBlock` with `isClosure` flag). Closure-scoped *decls* are skipped — only parent-scope `var X = clone_expression(...)` is a candidate. `clone_type` is out of scope (no matching `add_type_ptr_ref` splice wrapper in the user-facing qmacro grammar) |
 | PERF024 | `func(clone_*(X))` (Arm A) or `var X = clone_*(E); func(X)` with no other uses (Arm B) where `func` carries a `[clone(paramName)]` annotation at the matching arg position | Medium | drop the outer clone wrap — the callee clones internally. `[clone(p1, p2)]` is a C++-registered metadata-only annotation (zero runtime cost); each annotated function promises to clone the named params internally. Detection in `preVisitExprCallArgument`: compute the callee's annotated param indices, identity-match the current arg's position via `intptr`, peel `ExprRef2Value` wrapper (typer inserts it whenever a ref-typed var — e.g. `var X : Expression?` — is passed to a non-ref param), then either flag `clone_*(...)` directly (Arm A) or mark a bare ExprVar candidate as safe (Arm B). Arm B reuses PERF023's seed-+-classify shape with a per-function `Perf024Candidate` array and `perf024_skip_iptr` flag. Annotation is wrong when the function MUTATES its input in place (e.g. `apply_template` / all `apply_qmacro_*` / `apply_qblock_*` go through `TemplateVisitor` which substitutes in place) — pre-clones at mutating callsites are load-bearing, NOT redundant; verify by reading the body before annotating |
+| PERF025 | `"{string(x)}"` — a `string(value)` cast as a direct string-interpolation element, where `value` is stringify-equivalent: signed `int`/`int8`/`int16`/`int64`, `float`, `double`, `string`, `das_string` (`tHandle` + annotation `"das_string"`) | Low | drop the cast — interpolation already converts via the builder's `DebugDataWalker`, so the cast just allocates an intermediate string. Detection in `preVisitExprStringBuilderElement` (per-element hook): `perf025_unwrap_element` peels one `ExprRef2Value`, `perf025_string_call_value` matches a `string(...)` call (skipping explicit `string(x, true)` hex) and returns its value arg, `perf025_value_kind` classifies it. **Unsigned** ints (`uint`/`uint8`/`uint16`/`uint64`) still warn but append a `:d`-tag hint — they interpolate as **hex** by default (`"{42u}"` → `0x2a`), unlike `string()`'s decimal, so the bare drop changes output. **Skipped (kind 0):** `array<uint8>`/`uri`/`text_range` (value-shape change, not just format), `string()` nested as an arg to another call (`"{length(string(x))}"` — only direct elements are flagged), and any type with no `string()` overload. Reports at the `string` call's `.at`; for a `string(string)` arg PERF025 claims the location and the overlapping PERF020 is deduped. `string(x)` under a `_::fmt(":fmt", …)` format wrapper cannot occur — `fmt` has no string-valued overload, so `"{string(x):fmt}"` does not compile |
 
 ## Visitor gotchas
 

utils/lint/tests/perf025_redundant_string_in_interp.das

@@ -0,0 +1,88 @@
+options gen2
+// PERF025: string(...) inside string interpolation is redundant.
+//
+// Problem:
+//   String interpolation already converts every element to a string via the
+//   string builder's DebugDataWalker. Wrapping the element in string(...) just
+//   allocates an intermediate string that the builder then copies — a wasted
+//   heap allocation per interpolation.
+//
+// Bad pattern:
+//   def f(iv : int) : string {
+//       return "{string(iv)}"                    // PERF025
+//   }
+//
+// Good pattern:
+//   def f(iv : int) : string {
+//       return "{iv}"
+//   }
+//
+// Note for dastest readers: these use function PARAMETERS (not literals) so the
+// string() calls survive constant folding. `"{string(42)}"` would be folded to a
+// single ExprConstString during inference (visitStringBuilderElement ->
+// evalAndFoldString), leaving the rule nothing to attach to — and a folded
+// constant carries no runtime cost anyway. (Same approach as the PERF020 fixture.)
+//
+// Unsigned ints get the ':d' format-tag hint: they interpolate as hex by default
+// ("{42u}" -> "0x2a"), so dropping string() changes the output unless ':d' is used.
+//
+// das_string is also covered (reusing the PERF007 detection idiom: tHandle whose
+// annotation is "das_string"). It is not exercised here to keep the fixture free of
+// a daslib/ast require — `"{string(fn.name)}"` was verified separately. See the rst.
+
+expect 31208:8
+
+require daslib/perf_lint
+require strings
+
+// --- Bad patterns (one PERF025 each) ---
+
+def bad_int(iv : int) : string {
+    return "{string(iv)}"                                   // PERF025 — drop
+}
+
+def bad_int64(iv : int64) : string {
+    return "{string(iv)}"                                   // PERF025 — drop
+}
+
+def bad_float(fv : float) : string {
+    return "{string(fv)}"                                   // PERF025 — drop
+}
+
+def bad_double(dv : double) : string {
+    return "{string(dv)}"                                   // PERF025 — drop
+}
+
+def bad_string(sv : string) : string {
+    return "{string(sv)}"                                   // PERF025 — drop (PERF020 deduped at same loc)
+}
+
+def bad_uint(uv : uint) : string {
+    return "{string(uv)}"                                   // PERF025 — drop + ':d' hint (interp default is hex)
+}
+
+def bad_uint64(uv : uint64) : string {
+    return "{string(uv)}"                                   // PERF025 — drop + ':d' hint
+}
+
+def bad_uint8(uv : uint8) : string {
+    return "{string(uv)}"                                   // PERF025 — drop + ':d' hint
+}
+
+// --- Good patterns (no warnings) ---
+
+def good_nested_arg(iv : int) : string {
+    return "{length(string(iv))}"                           // string() is an arg to length(), not a direct element
+}
+
+def good_bytes_to_text(bytes : array<uint8>) : string {
+    return "{string(bytes)}"                                // value-shape conversion (bytes -> text); skipped
+}
+
+def good_plain_interp(iv : int) : string {
+    return "{iv}"                                           // no string() cast at all
+}
+
+def good_outside_interp(iv : int) : string {
+    return string(iv)                                       // string() outside any interpolation
+}