Enforce that a derivative references only decls it owns. - #1894
Merged
Conversation
A generated derivative must not reference the original function's own parameters or locals -- they do not exist in the derivative, so such a reference is a forgotten reference-remap (a primal clone never registered in m_DeclReplacements) that would miscompile. Until now this was masked by the scope-dependent name-lookup fallback in ReferencesUpdater. Add the check as a hard integrity invariant, alongside the shared-node and primal-splice checks, via a single ASTIntegrity entry point: verifyDerivative walks the body once and returns an IntegrityReport, while DerivativeBuilder keeps the policy -- assert in debug, diagnose in release. The stray-reference check fires only for a clean differentiation: an unsupported construct is cloned wholesale and knowingly keeps such references, so it is skipped when this function's differentiation emitted any diagnostic. A preserved lambda's own parameter is excluded by matching the declaration's immediate function context, not mere enclosure.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| // reference still bound to one of Original's own decls means its remap was | ||
| // forgotten (the primal clone was never registered in m_DeclReplacements). | ||
| // Walk the finished body and flag the first such reference. | ||
| struct Finder : RecursiveASTVisitor<Finder> { |
Contributor
There was a problem hiding this comment.
warning: constructor does not initialize these fields: Original [cppcoreguidelines-pro-type-member-init]
lib/Differentiator/ASTIntegrity.cpp:73:
- const FunctionDecl* Original;
+ const FunctionDecl* Original{};| struct Finder : RecursiveASTVisitor<Finder> { | ||
| const FunctionDecl* Original; | ||
| const ValueDecl* Stray = nullptr; | ||
| bool shouldVisitImplicitCode() const { return true; } |
Contributor
There was a problem hiding this comment.
warning: function 'shouldVisitImplicitCode' should be marked [[nodiscard]] [modernize-use-nodiscard]
Suggested change
| bool shouldVisitImplicitCode() const { return true; } | |
| [[nodiscard]] bool shouldVisitImplicitCode() const { return true; } |
| struct Finder : RecursiveASTVisitor<Finder> { | ||
| const FunctionDecl* Original; | ||
| const ValueDecl* Stray = nullptr; | ||
| bool shouldVisitImplicitCode() const { return true; } |
Contributor
There was a problem hiding this comment.
warning: method 'shouldVisitImplicitCode' can be made static [readability-convert-member-functions-to-static]
Suggested change
| bool shouldVisitImplicitCode() const { return true; } | |
| static bool shouldVisitImplicitCode() { return true; } |
| // parameter (context is the lambda's CXXMethod, not Original) is | ||
| // referenced by design when the lambda is preserved, not a forgotten | ||
| // clone, so exact-context match excludes it. | ||
| const auto* DC = dyn_cast<FunctionDecl>(D->getDeclContext()); |
Contributor
There was a problem hiding this comment.
warning: no header providing "clang::dyn_cast" is directly included [misc-include-cleaner]
lib/Differentiator/ASTIntegrity.cpp:18:
+ #include <clang/Basic/LLVM.h>| // referenced by design when the lambda is preserved, not a forgotten | ||
| // clone, so exact-context match excludes it. | ||
| const auto* DC = dyn_cast<FunctionDecl>(D->getDeclContext()); | ||
| if (isa<VarDecl>(D) && DC && |
Contributor
There was a problem hiding this comment.
warning: no header providing "clang::isa" is directly included [misc-include-cleaner]
if (isa<VarDecl>(D) && DC &&
^
vgvassilev
force-pushed
the
stray-ref-guardrail
branch
from
July 15, 2026 23:55
f79fec4 to
e23e7c5
Compare
| << VD << L; | ||
| } | ||
|
|
||
| #if CLANG_VERSION_MAJOR > 16 |
Contributor
There was a problem hiding this comment.
warning: no header providing "CLANG_VERSION_MAJOR" is directly included [misc-include-cleaner]
lib/Differentiator/DerivativeBuilder.cpp:51:
- #include <cstddef>
+ #include <clang/Basic/Version.h>
+ #include <cstddef>
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
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
1 participant
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.
A generated derivative must not reference the original function's own parameters or locals -- they do not exist in the derivative, so such a reference is a forgotten reference-remap (a primal clone never registered in m_DeclReplacements) that would miscompile. Until now this was masked by the scope-dependent name-lookup fallback in ReferencesUpdater.
Add the check as a hard integrity invariant, alongside the shared-node and primal-splice checks, via a single ASTIntegrity entry point: verifyDerivative walks the body once and returns an IntegrityReport, while DerivativeBuilder keeps the policy -- assert in debug, diagnose in release. The stray-reference check fires only for a clean differentiation: an unsupported construct is cloned wholesale and knowingly keeps such references, so it is skipped when this function's differentiation emitted any diagnostic. A preserved lambda's own parameter is excluded by matching the declaration's immediate function context, not mere enclosure.