opt: do not fold !! of non bool - #3560
Open
aleksisch wants to merge 11 commits into
Open
Conversation
aleksisch
force-pushed
the
aleksisch/no-folding-for-non-bool
branch
6 times, most recently
from
July 28, 2026 16:21
3a358f9 to
bcff236
Compare
It's incorrect to fold !!x if x of any other type than bool. For example it can be user provided type.
We support it in same way it works in aot.
We can't test it in CI, however das allow das_config to be configured with various standard libraries and various setups. Some of them have stronger requirements.
The splice substitutes the caller's argument expression for a parameter.
That breaks when the body passes the parameter on to a REF parameter:
the substituted value has no storage, so the inner call stops matching
("can't pass non-ref to ref"), and a program that compiled before the
pass now does not.
Record, per callee parameter, whether it is read as the argument of a ref
parameter, and bind a temp for those instead of substituting - for the
constant case as well as the computed one. A by-value parameter is a copy
in the real call anyway, so the temp is what the un-inlined code did.
Worse than a diagnostic when the substituted value is constant: the
folder then evaluates the native with a value where the C++ signature
wants `const T &` and dereferences storage that was never materialized,
which segfaults the compiler:
das::FoldingVisitor::evalAndFold -> Context::evalWithCatch -> <native>
The added test covers that shape - a private (so budget-exempt) callee
whose parameter goes to an implicit const-ref native, called with an
expression built from const locals. It crashed the compiler before this
change.
Found in dagor's enlisted: traceray_normalized in shell_fx_common.das
reported as no matching function after get_floor_material_name spliced.
The splice manufactures by-value locals - `__inl<N>_arg_<param>` for an
argument it cannot substitute (not a ref, or not pure), `__inl<N>_res`
for the statement path and the lazy-operator rewrite root. A type that
cannot be such a local produced a declaration infer rejects, reported
against a generated name the author never wrote and cannot act on:
error[31020]: local variable of type ecs::Point3List const requires unsafe
error[30199]: local variable __inl0_arg_offsets can only be initialized
with type constructor
error[30316]: local variable of type soundEvent::SoundEventHandle
needs to be initialized
Argument temps now get the same check the result type already had: a
non-local annotation (isLocal()==false) or a type that neither copies
nor moves refuses the splice and the site stays a plain call. Block
types keep the old behavior - infer exempts them from the isLocal test.
The result-side shape checks only covered a refType result, so a
by-value result with a nontrivial C++ constructor still reached the
splice via the uninitialized `__inl<N>_res` store; both the statement
path and the lazy-operator rewrite now refuse those too.
Found in dagor (active_matter human_gun.das, daNetGameLibs
sound_player_common - the latter exposed once the argument-temp refusal
reshaped those bodies).
dasUnitTest gains testNotLocalObject: das had no way to obtain a value
of a non-local type, which is what the temp must refuse.
aleksisch
force-pushed
the
aleksisch/no-folding-for-non-bool
branch
from
July 28, 2026 17:02
bcff236 to
626e0b8
Compare
aleksisch
changed the title
The splice renamed every declaration in the body into the __inl<N>_l_ namespace and then renamed references purely by name; the pure-graft path (expression bodies) skipped renaming entirely on the grounds that "expression bodies declare no locals". Both halves mishandled block arguments: * An expression body declares no locals, but a block literal inside one declares ARGUMENTS, and after the graft those sit in the caller's scope chain. infer re-resolves every ExprVar by name, so an unrenamed argument captured a substituted parameter expression spelling the same name - dagor hit `squad_team != team` emitted as `team != team`. The graft path now runs the same collector the statement path always ran (LocalNameCollect::preVisitBlockArgument). * A reference bound OUTSIDE a declaration's scope but spelling the same name was renamed too: a caller reference lexically before a nested can_shadow block that redeclares the name went looking for __inl<N>_l_<name>, which exists nowhere (30838). The rename is now scope gated: a declaration activates its name in a frame stack (blocks, for loops, lets at their declaration point) and a reference renames only while a matching declaration is in scope. * can_shadow block argument names are semantic: a host ECS resolves the query component by the argument name (dagor das_es marks every [es] block argument can_shadow), so renaming them changes what the query reads. They keep their names now. * With the names kept, a substituted caller expression that uses such a name free would be silently captured - that is exactly what can_shadow permits - and no renaming can fix it, because both sides are component names. Such a site is not inlined: the hazard set (free names of every actual call argument, block literals included) is intersected with the body's can_shadow argument names, and a hit declines the splice. Renaming rather than declining matters where renaming is sound: a can_shadow argument capture is silently wrong, an explicit [inline] callee has no heuristic gate, and the same shape without can_shadow does not compile at all (30701). Nested splices re-prefix correctly - four levels of colliding `team` still evaluate right. Reduced repros from dagor active_matter (squad_behaviour, migration.das). The declinable checks run BEFORE the eager path hoists the impure prefix: the hoist rewrites the statement in place, so a decline after it discarded the pending __inl<N>_pre* declarations while the statement kept the references - error 30838 at best (daslib/linq order-by keys, tutorial 56), a segfault in the access-flags pass at worst (inline_shadow_prefix_decline.das pins the shape). A kept can_shadow argument also SHIELDS its name within its scope: the rename map is name-keyed, so a same-named renamable declaration in the body would otherwise rename both the argument (losing the semantic name) and the references under it - the silent capture again, this time manufactured by the renamer itself. The explicit [inline] flavor of a capture is a hard error by design - the name cannot be renamed away and the author demanded the splice; failed_inline_shadow_capture.das expects it, and the runtime tests keep to the flavors that stay callable.
Invoke devirtualization splices the block literal in place and consumes the
holder local's only read. The patch slot runs before lint, and optimize -
which reaps the now-dead `let` - runs after it, so lint saw a variable with
no access flag left and reported it unused:
def apply_via_call(x : int) : int
let add_two = $(v : int) : int { return v + 2 }
return apply_block(add_two, x) // LINT002: unused variable add_two
At lint time add_two carries only access_init, while a function-pointer local
in the same position carries access_get|access_init|access_pass. That
asymmetry makes the gap look specific to block-typed variables, but it is not
- @@fn simply never devirtualizes. `options disable_auto_inline` compiles the
same file clean, which is what pins this on the splice rather than on passing
a variable as an argument.
The devirtualizer now marks the holder marked_used, the same "a transform ate
the reads" contract the unused-argument checks in ast_lint.cpp already honor,
and daslib/lint.das honors that flag for locals as it already did for
arguments. Optimize still reaps the dead `let`, so there is no codegen cost.
Found in dagor, where the reported variables could not be renamed away
without breaking the build.
A returned argument needs its mutability for the move (a non-copyable result cannot move from a const argument), so a `var` argument that is never written but flows into `return` is not a false `var`. Collect candidates at the arguments and decide after the body, once every return has been seen.
aleksisch
force-pushed
the
aleksisch/no-folding-for-non-bool
branch
from
July 28, 2026 22:30
626e0b8 to
82c6302
Compare
The macro stopped ending in a semicolon, so two bare uses on consecutive
lines spliced into one expression and the second leading * became a
multiplication:
error: invalid operands to binary expression ('unsigned int' and
'DasThreadLocal<unsigned int, ...>')
Put the semicolon back. A call site that writes its own gets an empty
statement.
The macro deliberately stays a bare qualified call (::register_*, no
declaration): a block-scope declaration binds to the innermost enclosing
namespace, so a self-declaring NEED_MODULE inside `namespace host { }`
declares host::das_pull_* that nothing defines - test_cpp_module_pull
pins the namespace contract. The default modules are declared by this
header; a separately compiled module keeps its file scope DECLARE_MODULE.
requireEx identity-matches a shared module by the canonical require string
it was promoted with. The same file is legitimately spelled several ways -
mount prefix, '/' vs '.' - so the check rejected the promoted module and a
second instance of it got compiled:
skip module 'base': promoted '%gameLibs.das.math.base' vs require 'math.base'
skip module 'strings_boost': promoted '%daslib/strings_boost' vs require 'daslib/strings_boost'
skip module 'strings_boost': promoted 'daslib/strings_boost' vs require 'daslib.strings_boost'
Code compiled against the first instance cannot see the second one's
functions, so a generic in a shared module stopped resolving:
error[30341]: no matching functions or generics: square(float const&)
base::square(x: auto const): auto at %gameLibs/das/math/base.das:29:4
module base is not visible directly from grav_zones_common
It needs several files in one compiler invocation - which is how a game's
AOT step calls the tool - so single-file compiles of the same sources look
clean. requireEx now also takes the resolved file name and accepts a
promoted module whose file matches, whatever the spelling; a require that
resolves to a different file still fails.
aleksisch
force-pushed
the
aleksisch/no-folding-for-non-bool
branch
from
July 28, 2026 22:54
82c6302 to
699ba15
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
This PR tightens compiler/JIT correctness around constant folding and inlining, motivated by cases where seemingly “safe” optimizations (like folding !!x) are not semantics-preserving for non-bool types and where inlining can manufacture illegal or capture-prone AST shapes. It also improves JIT handling of block annotation payloads by resolving them via SID lookups instead of attempting to bake compile-process addresses into generated code.
Changes:
- Restrict
!!x → xfolding tobooloperands and add fold gating for selected built-in functions viaBuiltInFunction::noFolding. - Harden the inliner against ref-argument storage needs and can_shadow capture/shadowing hazards; add multiple regression tests.
- Resolve block
annotationDatain LLVM JIT viajit_ad_by_sid(context SID→data table), removing prior “trap/unimplemented” behavior for exe emission.
Reviewed changes
Copilot reviewed 39 out of 39 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/README.md | Adds entries for new regression tests (index updates) |
| tests/lint/test_lint002_block_var_used.das | Regression for LINT002 false positive after devirt |
| tests/language/inline_shadow_prefix_decline.das | Pins decline-after-prefix-hoist rewrite safety |
| tests/language/inline_shadow_nested_query.das | Regression for can_shadow rename/capture interactions |
| tests/language/inline_shadow_capture.das | Regression for capture risk in spliced can_shadow blocks |
| tests/language/inline_ref_arg.das | Regression for compile-time folding of native const-ref calls |
| tests/language/handle.das | Regression for !! folding on non-bool handle types |
| tests/language/failed_inline_shadow_capture.das | Negative test for explicit inline + can_shadow capture |
| tests/language/block_annotation_data.das | Cross-tier block annotationData round-trip test |
| tests/language/_inline_shadow_helper.das | Block macro setting can_shadow on block arguments |
| tests/jit_tests/trap_block_ann.das | JIT test ensuring exe emission works with annotationDataSid |
| tests/jit_tests/inline_const_ref_arg.das | JIT regression: inlined rvalue to const-ref binds temp storage |
| tests/jit_tests/fold_extern_at_compile.das | Documents/guards compile-time folding of externs |
| tests/jit_tests/_trap_block_ann_helper.das | Helper block macro that fills annotationData(+Sid) |
| tests/handle_types/inline_arg_temp_not_local.das | Regression: refuse inliner arg temp for non-local types |
| tests-cpp/small/test_promoted_module_spelling.cpp | C++ test for promoted module identity across require spellings |
| src/misc/job_que.cpp | Namespace-qualify max/chrono usage |
| src/misc/daScriptC.cpp | Namespace-qualify min usage for portability |
| src/builtin/module_jit.cpp | Add jit_ad_by_sid helper and export address getter |
| src/builtin/module_builtin_jobque.cpp | Namespace-qualify std::chrono usage |
| src/ast/ast_parse.cpp | Pass resolved filename into Module::requireEx |
| src/ast/ast_module.cpp | Match promoted modules by file identity across spellings |
| src/ast/ast_inline.cpp | Inliner fixes: ref-arg storage, capture risk checks, scope-safe renames |
| src/ast/ast_infer_type_helper.cpp | Respect BuiltInFunction::noFolding in const-expr folding gate |
| src/ast/ast_dse.cpp | Fix das::min template calls for size_t |
| src/ast/ast_const_folding.cpp | Restrict !! fold to bool; add foldable-builtin gating |
| src/ast/ast_alias.h | Avoid reverse-iterator assign for dagor vector compatibility |
| modules/dasUnitTest/unitTest.h | Add UnitTest APIs for block annotation and non-local temp test |
| modules/dasUnitTest/test_handles.cpp | Implement new UnitTest externs/annotations/operators |
| modules/dasLLVM/daslib/llvm_jit.das | JIT make-block: resolve annotation payload via SID lookup |
| modules/dasLLVM/daslib/llvm_jit_run.das | Bump LLVM JIT codegen version for cache invalidation |
| modules/dasLLVM/daslib/llvm_jit_common.das | Register jit_ad_by_sid extern in JIT ABI |
| modules/dasLLVM/daslib/llvm_exe.das | Remove trap/unimplemented path for annotationData exe emission |
| modules/dasLLVM/daslib/llvm_dll_utils.das | Remove obsolete block-ann global UID helper |
| include/daScript/simulate/aot_builtin_jit.h | Declare das_get_jit_ad_by_sid |
| include/daScript/daScriptModule.h | Add semicolon in NEED_MODULE macro expansion |
| include/daScript/daScriptBind.h | Add das_bind_no_fold + mark selected binds as no-fold |
| include/daScript/ast/ast.h | Add BuiltInFunction::noFolding flag/API |
| daslib/lint.das | Adjust LINT014 timing; honor marked_used to avoid false positives |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
503
to
509
| | table.das | Table operations | | | ||
| | table_value_key.das | Table value/key access | | | ||
| | trap_block_ann.das | Block with `annotationData` filled by a `[block_macro]` — runs under JIT, and a standalone exe is still emitted (the LLVM backend used to store a trap in the block's global instead) | | | ||
| | _trap_block_ann_helper.das | *(helper)* `[block_macro]` whose `finish()` fills `annotationData`/`annotationDataSid` | | | ||
| | try_recover.das | Try/recover codegen | | | ||
| | tuple.das | Tuple operations | | | ||
| | type_constructors.das | Type constructor codegen | | |
Comment on lines
575
to
579
| | block_invoke.das | Block invocation — twice, nested chaining, value capture, ref passing | | | ||
| | block_access_function_arg.das | Nested block accessing outer lambda variable via helper | | | ||
| | block_annotation_data.das | Block `annotationData` round-trip — `[block_ann_data]` writes it in finalize, `testBlockAnnotationData` reads it back off the runtime block; must agree across interpreter / C++ AOT (`adBySid`) / LLVM JIT (`jit_ad_by_sid`) | | | ||
| | block_args_nested.das | Deeply nested blocks — int, ref, ptr, struct passthrough | | | ||
| | block_variable.das | Local block variables — void/result × no-arg/with-arg × value/cmres | | |
Comment on lines
+9
to
+11
| // Minimal shape, reduced from dagor active_matter scripts/game/es/migration.das | ||
| // (lauch_migration): my_region has an expression body, so its call site takes the pure-graft | ||
| // splice. Its block literal declares `transform`, which the graft renames to __inl1_l_transform. |
A host container mapped onto das::vector may implement iterator assign as a raw-pointer cast (dagor's dag::Vector does), which does not compile for a reverse_iterator range. Copy in reverse explicitly.
aleksisch
force-pushed
the
aleksisch/no-folding-for-non-bool
branch
from
July 28, 2026 23:05
699ba15 to
27ebece
Compare
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.
It's incorrect to fold !!x if x of any other type than bool. For example it can be user provided type.