[AST][Diagnostics] Reading DiagnosticTransaction's hasErrors() has unexpected side effects

[jamieQ]

Description

the implementation of DiagnosticTransaction's hasErrors() method calls through to the underlying engine state's determineBehavior() method. that method has side-effects in the case that its argument is an error, in which cases it mutates the relevant error flags. this leads to confusing behavior where reading hasErrors() on an open transaction can affect the behavior of the parent diagnostic engine.

Reproduction

somewhere in the compiler (i tested in TypeCheckConstraints.cpp), do something like the following:

DiagnosticTransaction txn(ctx.Diags);
ctx.Diags.diagnose(/*some error*/);
assert(!ctx.Diags.hadAnyError()); // error state hasn't changed because transaction is open
assert(txn.hasErrors()); // check transaction for errors
txn.abort();
assert(!ctx.Diags.hadAnyError()); // 💥 assertion failure – passes if `hasErrors()` isn't read

Expected behavior

i would expect either no side-effects when calling an open transactions hasErrors() or if the transaction is aborted, the error state should be reset to what it originally was when the transaction was opened.

Environment

swift-driver version: 1.90.11.1 Apple Swift version 5.10 (swiftlang-5.10.0.13 clang-1500.3.9.4)
Target: arm64-apple-macosx14.0

Additional information

associated forums post