Reset sticky diagnostic state after a rejected InstantiateTemplate#1070
Reset sticky diagnostic state after a rejected InstantiateTemplate#1070conrade-ctc wants to merge 1 commit into
Conversation
| // the incremental parser performs on a real parse failure). | ||
| clang::DiagnosticsEngine& Diags = S.getDiagnostics(); | ||
| bool HadErrors = Diags.hasErrorOccurred(); | ||
| Decl* Result = InstantiateTemplate(TmplD, TemplateArgs, S, instantiate_body); |
There was a problem hiding this comment.
Can can insert a diagnostic trap object -- we have that elsewhere in the codebase.
There was a problem hiding this comment.
indeed, thanks for pointing that out, update forth-coming!
There was a problem hiding this comment.
Done — the manual snapshot is now a DiagnosticErrorTrap-based RAII (StickyDiagnosticResetRAII): the trap detects errors introduced during the instantiation, and on scope exit the guard restores a clean state with the same Diags.Reset(/*soft=*/true) + client clear() cleanup IncrementalParser performs on a real parse failure. I left InstantiateFunctionDefinition's inline copy of this cleanup untouched — it could migrate to the same guard, but #849 is already touching that spot and I didn't want the two PRs to collide.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1070 +/- ##
==========================================
+ Coverage 87.01% 87.04% +0.03%
==========================================
Files 23 23
Lines 6069 6076 +7
==========================================
+ Hits 5281 5289 +8
+ Misses 788 787 -1
🚀 New features to boost your workflow:
|
|
clang-tidy review says "All clean, LGTM! 👍" |
803d04c to
b6fee6d
Compare
The rejection (e.g. a type argument for a non-type parameter) emits its error diagnostic outside any incremental parse, and DiagnosticsEngine's ErrorOccurred flag is sticky: the next IncrementalParser::Parse reported the stale flag as its own failure and then soft-reset it, poisoning exactly one parse. Contain the rejection with a DiagnosticErrorTrap-based RAII that restores a clean diagnostic state when the instantiation introduced errors - the same cleanup IncrementalParser performs on a real parse failure and InstantiateFunctionDefinition performs inline. Co-developed-with-the-help-of: Claude Code (Fable 5, human in the loop)
b6fee6d to
4fd6208
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
2 participants
Description
A rejected
Cpp::InstantiateTemplate(e.g. a type argument supplied for a non-type template parameter) emits its error diagnostic outside any incremental parse, andDiagnosticsEngine'sErrorOccurredflag is sticky. The nextIncrementalParser::Parsesees the stale flag after a perfectly good parse, reports "Parsing failed.", and soft-resets the engine — so exactly one subsequentCpp::Declare/Processfails spuriously. Downstream (cppyy), one rejected template subscript makes the nextcppyy.include/cppyy.cppdefreport failure while actually loading the code, which can silently corrupt fail-soft feature probes.Per review, the rejection is now contained by a
DiagnosticErrorTrap-based RAII (StickyDiagnosticResetRAII): the trap detects errors introduced during the instantiation, and on scope exit the guard restores a clean state with the sameDiags.Reset(/*soft=*/true)+ clientclear()cleanup thatIncrementalParserperforms on a real parse failure (and thatInstantiateFunctionDefinitionperforms inline). The rejection's diagnostic text is still emitted to the client as before.Type of change
Testing
New
ScopeReflection_RejectedInstantiationDoesNotPoisonNextParse: a rejected instantiation followed by twoCpp::Declarecalls, both of which must succeed. Fails before the fix (the first Declare reports failure), passes after.Checklist
🤖 Done with the help of Claude Code (Fable 5, human in the loop)