Repository navigation
Keep the string type of a function-local string read with history in a concatenation (#354) - #358
Conversation
Tests only. A function-local string that is read with history and joined with its own current value by + (issue #354) must keep its string type: the current slot is a std::string, so the emitted C++ must not wrap it in std::to_string. tests/test_string_local_history_concat.py compiles, runs and compares the result of the neutral source from the issue, the same shape under the built-in spellings second and hour, a string-annotated local, a reassigned local, two call sites with different arguments, a chart-level pair and swapped operands. It also keeps the translation-time refusal of the nine price-field names read with history in a requested helper, the emission of an explicit string conversion and of a numeric na operand, and the analyzer refusal of a string plus an integer. The expected values are literals computed by hand from closes 10, 20, 30 and opens 1, 2, 3. tests/test_builtin_named_locals_compile.py now requires the neutral control of each string-history row to compile; before, it accepted a compile failure that matched the renamed row's failure. Two older modules compare a complete emission with the emission of an older codegen: tests/test_generated_member_names.py and tests/test_helper_builtin_named_locals.py. For their string rows the older emission holds the join that did not compile, so they now repair that one site in the older text with a helper of the new module (it requires exactly one such site) and compare everything else unchanged. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…a concatenation A function-local string that is read with history (aa[1]) is a string series, but the scalar local-type map has no entry for it, so the type query for the bare name returned the double fallback. In a + whose other operand was a string the bare name was then wrapped in std::to_string, which does not accept a std::string, and the generated C++ did not compile (issue #354). The binary + lowering now asks for the series element type when the query returns double for a bare function-local that reads its own series slot and whose series holds strings; every other operand is typed as before. The general type query and the translation-time refusal of price-field names are unchanged. The changelog drops the exception that named this shape and adds a bullet for the fix. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Brings in the pull request that landed on main after this branch was cut (the reservation of the numbered tuple element bindings, and the string-history refusal rows pinned against the pinned reference). Two files conflict. CHANGELOG.md: the bullet about a local spelled like a built-in variable keeps main's text without its clause saying that a string local read with history and used in a concatenation still does not compile (this branch fixes that), and the new bullet for the fix stays after it. tests/test_builtin_named_locals_compile.py: the docstring keeps main's price-field exception and no longer says that the string-with-history shapes do not compile; the loop of test_string_history_compile_outcome_matches_neutral_control is main's loop over the census of names, with this branch's assertion that the neutral control compiles kept before it. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…e emissions The nine rows of test_price_named_string_library_local_history_keeps_the_base_outcome compare the whole C++ of a script whose library function joins a string local with its previous value against the older reference commit. The reference wrapped the current value in std::to_string (issue #354), which does not compile; with the fix merged, the new emission joins the strings. The reference text gets the same one-site repair that the two other modules of the fix use (it requires exactly one such site), and everything else is still compared unchanged. Rows that compare a refusal are not touched. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
![graphify-labs[bot]](https://avatars.githubusercontent.com/in/4356270?s=60&v=4){kind=small}
There was a problem hiding this comment.
Graphify reviewed this change.
No issues found by static checks — no coupling regressions or blocking issues in the code graph. That is not the same as safe to merge: see what was not checked.
Not checked: tests were not run; formal verification proved 0 of 1 changed function(s) (1 not verified).
Formal verification. PR-changed functions: 0/1 verified (0 proven, 0 may-equivalent, 0 distinguished) · 1 not verified (1 unsupported).
Not verified on this run: \_lower\_binop (unsupported).
Graphify review — findings
Fixes function-local string variables that are read with history so they keep their string type in + concatenations, instead of being treated as double and wrapped in std::to_string (#354). The + operand typing now checks whether an identifier is a series-backed local whose series type is std::string, and treats it as a string. That way the emitted C++ joins the current and previous values directly and compiles. The price-field refusal through request.security is unchanged, and the changelog drops the "still does not compile" exception and adds an entry for the fix. New tests cover the concat behaviour. Existing reference-comparison tests now allow that one repaired join as the only difference from the old emission, and the string-history compile test asserts the neutral control actually compiles.
No blocking issues surfaced.
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 258 functions depend on the 251 functions this change touches.
Health — this change adds coupling hotspots:
- worse:
_assert_keeps_the_base_outcome()— 3 callers, 4 callees
Verification — 258 functions in the blast radius were not formally verified this run (proofs are advisory here).
Gate & verification
graphify gate
PASS — objectively clean (no health regressions, tests not run — proofs not run this pass (advisory)). Grounded, not self-assessed.
Advisory (not blocking):
- verification_scope: 258 function(s) in the blast radius were not formally verified this run
Cross-repo impact
Cross-repo impact
This change impacts 2 symbol(s) across 1 peer repo(s) — blast radius over derived cross-repo edges:
- pineforge-4pass/pineforge-hpo —
python/pineforge_hpo/transpile.py(via import_moniker, heuristic),tests/python/test_transpile.py(via import_moniker, heuristic)
1 peer repo(s) in this group are not yet indexed — cross-repo impact may be incomplete.
Cross-repo linkage is derived (SCIP-moniker matches are sound; name/contract matches are heuristic) and ADVISORY — it never affects this check's conclusion.
Test selection
Test selection
509 of 509 test file(s) selected (100%) via static blast radius.
Escalated to a full run for safety (
noncode-change,uncovered-code) — the selection is not trustworthy on its own (see below). CI should run the whole suite.
tests/test_action_pins.py— full-run-safetytests/test_allow_entry_in_qualifier.py— full-run-safetytests/test_allow_entry_in_request_const.py— full-run-safetytests/test_analyzer.py— full-run-safetytests/test_analyzer_matrix_inference.py— full-run-safetytests/test_analyzer_ta_return_types.py— full-run-safetytests/test_anchor_tf_change.py— full-run-safetytests/test_anchor_time.py— full-run-safetytests/test_anchor_vwap.py— full-run-safetytests/test_array_bounds_insert_fill_slice.py— full-run-safetytests/test_array_checked_access.py— full-run-safetytests/test_array_empty_aggregates.py— full-run-safetytests/test_array_history.py— full-run-safetytests/test_array_missing_integer_index.py— full-run-safetytests/test_array_new_color_fill_and_na_size.py— full-run-safetytests/test_array_new_defaults_and_order.py— full-run-safetytests/test_array_reference_semantics.py— full-run-safetytests/test_array_slice_view.py— full-run-safetytests/test_array_terminal_returns.py— full-run-safetytests/test_arrseg_coded_run_stops.py— full-run-safetytests/test_arrseg_review_fixes.py— full-run-safetytests/test_binop_operand_order.py— full-run-safetytests/test_block_shadow_primitive_types.py— full-run-safetytests/test_bool_na_refusal.py— full-run-safetytests/test_bool_na_refusals_2.py— full-run-safetytests/test_bool_na_values.py— full-run-safetytests/test_bool_string_inference.py— full-run-safetytests/test_builtin_name_shadow.py— full-run-safetytests/test_builtin_named_locals_compile.py— impact, changed-test, full-run-safetytests/test_builtin_named_locals_price_refusal.py— impact, changed-test, full-run-safetytests/test_builtin_named_locals_receipt.py— full-run-safetytests/test_builtin_named_locals_scope.py— impact, full-run-safetytests/test_calc_on_order_fills_codegen.py— full-run-safetytests/test_calendar_lambda_locals.py— full-run-safetytests/test_call_argument_order.py— full-run-safetytests/test_callable_local_histories.py— full-run-safetytests/test_callable_series_param_types.py— full-run-safetytests/test_callable_string_series_type.py— full-run-safetytests/test_callable_ta_forwarded_length.py— full-run-safetytests/test_callable_var_first_reach.py— full-run-safetytests/test_checked_settings.py— full-run-safetytests/test_ci_engine_main_touched.py— full-run-safetytests/test_closed_trade_fold.py— full-run-safetytests/test_closed_trade_fold_emission.py— full-run-safetytests/test_closed_trade_text_equals.py— full-run-safetytests/test_coded_run_stops.py— full-run-safetytests/test_codegen_ascii.py— full-run-safetytests/test_codegen_audit_fixes.py— full-run-safetytests/test_codegen_determinism.py— full-run-safetytests/test_codegen_drawing_data.py— full-run-safety- … and 459 more
non-code file(s) changed (
CHANGELOG.md) → running the full suite for safety (a code graph can't see config/fixture/data deps)
changed code file(s) with no mapped test (
CHANGELOG.md) — a coverage gap or a missing link — running the full suite rather than only the selected tests
Selection is safe under the controlled-regression assumption; always-run tests + a periodic full run are the backstops. Advisory — it never changes the check verdict.
Formal verification
Could not verify: Could not verify \_lower\_binop.
The verifier did not have enough to check \_lower\_binop, so it is saying so rather than guessing. No false assurance is the whole point.
Guarantee: No guarantee either way, this is an honest abstention, not a pass.
Note: Reason: method — first parameter is `self`, which needs a constructed instance (not synthesizable)
· 1 grounded finding(s) anchored inline below.
|
|
||
|
|
||
| def _assert_keeps_the_base_outcome(source, reference, tmp_path, libraries=None): | ||
| def _assert_keeps_the_base_outcome(source, reference, tmp_path, libraries=None, |
There was a problem hiding this comment.
_assert_keeps_the_base_outcome()
high coupling complexity (Ca·Ce = 12).
Grounded coupling-delta finding (deterministic), not an LLM guess.
The only conflict is CHANGELOG.md. This change edits the first bullet of the built-in-named locals entry (it drops the clause saying that a string local read with history and used in a concatenation still does not compile) and adds its own bullet after it; the train branch adds the built-in-named parameter bullet after the original entry. The result keeps this change's edited bullet and its new bullet, then the train's parameter bullet. No other file needed a resolution. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Nine reviewed changes, merged together: - #363: Pine v6 now refuses an `if` or `while` condition whose numeric value comes from a `ta.*` call or a built-in variable such as `strategy.position_size`, as TradingView does. Bool overloads and Pine v5 libraries keep their existing behavior. - #364: a `var` declared in one block no longer shares storage with a plain variable of the same name in a sibling block, so it keeps its value across runs. - #365: a function or method parameter spelled like a built-in variable (`close`, `high`, ...) keeps its `[n]` history operator and gives the same values as a parameter with another name. - #366: removes unused metrics metadata from test fixtures. No change to generated code. - #367: a float setting of a built-in indicator that is computed from an input keeps its fraction, as on TradingView. This covers the multipliers of `ta.bb`, `ta.bbw`, `ta.kc`, `ta.kcw` and the `ta.vwap` bands, the factor of `ta.supertrend`, the arguments of `ta.sar`, and the offset and sigma of `ta.alma`. Regenerate C++ and relink affected strategies. - #368: clarifies fixture and reference wording in the docs. No change to generated code. - #369: a retained record field built from a stable `var` array, and a plain `var` array alias, stay live through their creation bar and then keep their own copy of the array as it stood at the end of that bar. Two holders built from one array do not share it. Regenerate C++ and relink affected strategies. - #358: a function-local string read with history keeps its string type in a concatenation, so the emitted C++ joins the current and previous values without applying `std::to_string` to a string. - #373: removes a test fixture from #363's tests that made them fail where the native engine is not installed; they now skip there. No change to generated code. Removed or reworded documentation: - CHANGELOG.md: the existing entry for function-local variables spelled like built-in variables drops its exception "a string local that is read with history and used in a concatenation still does not compile, for any name". #358 makes that case compile, so the limitation is gone. - docs/reference/known-codegen-quirks.md: two references to the TradingView probe tool by its tool name are removed or reworded as "TradingView counting-probe tape" (#368). A new paragraph says that a float signature slot keeps its fraction where a runtime constructor argument was narrowed to an integer (#367). - docs/reference/support-contracts.md: one table row is reworded only, with no change in meaning: "per-run" and "symbols" replace an internal word, and "fixture readouts" replaces the probe tool name (#368). No other documented behavior is deleted or reverted. Refs #361 (two of its seven forms) Fixes #359 Fixes #360 Fixes #362 Fixes #354 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Summary
Fixes #354. A function-local string that is read with history (
aa[1]) is a string series, but the scalar local-type map has no entry for it, so the type query for the bare name returned thedoublefallback. In a+whose other operand was a string, the bare name was then wrapped instd::to_string, which does not accept astd::string, and the generated C++ did not compile.The binary
+lowering now asks for the series element type when the query returnsdoublefor a bare function-local that reads its own series slot and whose series holds strings. Every other operand is typed as before. The general type query and the translation-time refusal of price-field names are unchanged.What changes
pineforge_codegen/codegen/visit_expr.pyCHANGELOG.mdtests/test_string_local_history_concat.pysecondandhour, a string-annotated local, a reassigned local, two call sites with different arguments, a chart-level pair and swapped operands; the translation-time refusal of the nine price-field names read with history in a requested helper; the emission of an explicit string conversion and of a numericnaoperand; the analyzer refusal of a string plus an integer. The expected values are literals computed by hand from closes 10, 20, 30 and opens 1, 2, 3.tests/test_builtin_named_locals_compile.pytests/test_generated_member_names.py,tests/test_helper_builtin_named_locals.py,tests/test_builtin_named_locals_price_refusal.pyTests
The change is four commits. The first adds the rows and fails on its own: 18 rows fail because the join does not compile. The second adds the fix and the changelog; the rows pass. The third merges main (v1.11.0 and the pull request that followed it; two files conflicted: the changelog bullet about locals spelled like built-in variables, which loses its clause saying that this shape does not compile, and the docstring and loop of one compile test). The fourth repairs the nine library rows described above. They are meant to land together.
Checks
mainin the same setup: 25 failed, 28,184 passed, 304 skipped. The 25 failures are rows that compare against an older reference commit that is not an ancestor of the tested head, which that checkout cannot provide; the same 25 fail onmain, and the lists of failing test ids are identical.Limits
nanin the numeric-naconversion rows is the repository's own convention fornain a string join, and one row pins the glibc spelling. It was not compared with TradingView.==takes the numeric comparison path, by reading the code, onmainand with this change. This change does not touch that path and no row measures it.open,high,low,close,hl2,hlc3,hlcc4,ohlc4,volume) read with history in a requested helper keep their translation-time refusal.🤖 Generated with Claude Code