From 0d36692275b601d1690e337d180cead359576d19 Mon Sep 17 00:00:00 2001 From: Matthew Woehlke Date: Wed, 22 Jul 2026 15:29:59 -0400 Subject: [PATCH] Diagnostics: Fix interaction with cached state The prior mechanism for reconciling cached diagnostic state with updates from presets / command-line arguments based on whether the latter modified diagnostics from their default state did not work. This is because alterations that would not change the state relative to the default were not recorded, even if they would alter the state relative to the prior (cached) state. The best fix for this is to apply alterations on top of the cached state, rather than trying to determine whether an alteration is important. Unfortunately, the only way to do this is to defer alterations until after we can load the cache, which somewhat defeats one of the original goals of reducing the number of mechanisms by which alterations are recorded. Modify how we handle diagnostic alterations to map them through a helper function, which either defers them (in Project mode) or acts as a pass-through (applying the changes immediately, in all other modes). While this does, as noted, introduce another means of storing diagnostics, this new storage effectively consists of a list of deferred function calls, rather than a unique format for recording changes, which is what we had prior to the Great Refactor. --- Source/cmake.cxx | 96 ++++++++++--------- Source/cmake.h | 17 ++++ Tests/RunCMake/Diagnostics/Persist1.cmake | 4 + Tests/RunCMake/Diagnostics/Persist2.cmake | 4 + Tests/RunCMake/Diagnostics/RunCMakeTest.cmake | 15 +++ 5 files changed, 91 insertions(+), 45 deletions(-) create mode 100644 Tests/RunCMake/Diagnostics/Persist1.cmake create mode 100644 Tests/RunCMake/Diagnostics/Persist2.cmake diff --git a/Source/cmake.cxx b/Source/cmake.cxx index 4e9c451b9e..594d504a02 100644 --- a/Source/cmake.cxx +++ b/Source/cmake.cxx @@ -293,13 +293,6 @@ bool cmakeCheckStampList(std::string const& stampList) return true; } -bool isDiagnosticSet(cmStateSnapshot const& state, - cmDiagnosticCategory category) -{ - constexpr cmDiagnosticAction unset = cmDiagnostics::Undefined; - return (state.GetDiagnostic(category, unset) == unset); -} - } // namespace cmDocumentationEntry cmake::CMAKE_STANDARD_OPTIONS_TABLE[15] = { @@ -513,22 +506,22 @@ void cmake::SetDiagnosticsFromPreset( auto const wi = warnings.find(category); if (wi != warnings.end()) { if (wi->second) { - this->CurrentSnapshot.PromoteDiagnostic( // clang-format: break - category, cmDiagnostics::Warn, true); + this->AlterDiagnostic(&cmStateSnapshot::PromoteDiagnostic, category, + cmDiagnostics::Warn, true); } else { - this->CurrentSnapshot.DemoteDiagnostic( // clang-format: break - category, cmDiagnostics::Ignore, true); + this->AlterDiagnostic(&cmStateSnapshot::DemoteDiagnostic, category, + cmDiagnostics::Ignore, true); } } auto const ei = errors.find(category); if (ei != errors.end()) { if (ei->second) { - this->CurrentSnapshot.PromoteDiagnostic( // clang-format: break - category, cmDiagnostics::SendError, true); + this->AlterDiagnostic(&cmStateSnapshot::PromoteDiagnostic, category, + cmDiagnostics::SendError, true); } else { - this->CurrentSnapshot.DemoteDiagnostic( // clang-format: break - category, cmDiagnostics::Warn, true); + this->AlterDiagnostic(&cmStateSnapshot::DemoteDiagnostic, category, + cmDiagnostics::Warn, true); } } } @@ -676,13 +669,13 @@ bool cmake::SetCacheArgs(std::vector const& args) } if (foundNo) { - state->CurrentSnapshot.DemoteDiagnostic( - *category, foundError ? cmDiagnostics::Warn : cmDiagnostics::Ignore, - true); + state->AlterDiagnostic( + &cmStateSnapshot::DemoteDiagnostic, *category, + foundError ? cmDiagnostics::Warn : cmDiagnostics::Ignore, true); } else { - state->CurrentSnapshot.PromoteDiagnostic( - *category, foundError ? cmDiagnostics::SendError : cmDiagnostics::Warn, - true); + state->AlterDiagnostic( + &cmStateSnapshot::PromoteDiagnostic, *category, + foundError ? cmDiagnostics::SendError : cmDiagnostics::Warn, true); } return true; }; @@ -1333,8 +1326,9 @@ void cmake::SetArgs(std::vector const& args) "--warn-uninitialized", CommandArgument::Values::Zero, [](std::string const&, cmake* state) -> bool { warnDeprecated("--warn-uninitialized"_s, "-Wuninitialized"_s); - state->CurrentSnapshot.PromoteDiagnostic( - cmDiagnostics::CMD_UNINITIALIZED, cmDiagnostics::Warn, true); + state->AlterDiagnostic(&cmStateSnapshot::PromoteDiagnostic, + cmDiagnostics::CMD_UNINITIALIZED, + cmDiagnostics::Warn, true); return true; } }, CommandArgument{ "--warn-unused-vars", CommandArgument::Values::Zero, @@ -1343,8 +1337,9 @@ void cmake::SetArgs(std::vector const& args) "--no-warn-unused-cli", CommandArgument::Values::Zero, [](std::string const&, cmake* state) -> bool { warnDeprecated("--no-warn-unused-cli"_s, "-Wno-unused-cli"_s); - state->CurrentSnapshot.DemoteDiagnostic(cmDiagnostics::CMD_UNUSED_CLI, - cmDiagnostics::Ignore, true); + state->AlterDiagnostic(&cmStateSnapshot::DemoteDiagnostic, + cmDiagnostics::CMD_UNUSED_CLI, + cmDiagnostics::Ignore, true); return true; } }, CommandArgument{ @@ -2436,13 +2431,9 @@ int cmake::Configure() #endif // We now need to harmonize the previous initial diagnostic state with any - // changes requested via command line options. This is a bit tricky, because - // we need to underlay what is specified by the cache beneath whatever state - // has been built from command line processing. - - cmDiagnosticAction deprecated = this->CurrentSnapshot.GetDiagnostic( - cmDiagnostics::CMD_DEPRECATED, cmDiagnostics::Undefined); - bool const deprecatedAlreadySet = (deprecated != cmDiagnostics::Undefined); + // changes requested via command line options and/or presets. We do this by + // first applying the prior (cached) state, then applying all deferred + // alterations. if (cmValue cachedDiagnostics = this->State->GetCacheEntryValue("CMAKE_DIAGNOSTIC_INIT")) { @@ -2456,11 +2447,7 @@ int cmake::Configure() cmDiagnostics::GetDiagnosticAction(v.substr(n + 1)); if (category && action) { - // Only use the cache if command-line options have not modified the - // diagnostic. - if (isDiagnosticSet(this->CurrentSnapshot, *category)) { - this->CurrentSnapshot.SetDiagnostic(*category, *action, false); - } + this->CurrentSnapshot.SetDiagnostic(*category, *action, false); } } } @@ -2470,9 +2457,11 @@ int cmake::Configure() this->State->GetCacheEntryValue("CMAKE_WARN_DEPRECATED"); if (cachedWarnDeprecated) { if (cachedWarnDeprecated.IsOn()) { - deprecated = cmDiagnostics::Warn; + this->CurrentSnapshot.PromoteDiagnostic(cmDiagnostics::CMD_DEPRECATED, + cmDiagnostics::Warn, false); } else { - deprecated = cmDiagnostics::Ignore; + this->CurrentSnapshot.DemoteDiagnostic(cmDiagnostics::CMD_DEPRECATED, + cmDiagnostics::Ignore, false); } } @@ -2480,15 +2469,17 @@ int cmake::Configure() this->State->GetCacheEntryValue("CMAKE_ERROR_DEPRECATED"); if (cachedErrorDeprecated) { if (cachedErrorDeprecated.IsOn()) { - deprecated = cmDiagnostics::SendError; + this->CurrentSnapshot.PromoteDiagnostic(cmDiagnostics::CMD_DEPRECATED, + cmDiagnostics::SendError, false); + } else { + this->CurrentSnapshot.DemoteDiagnostic(cmDiagnostics::CMD_DEPRECATED, + cmDiagnostics::Warn, false); } } - if (!deprecatedAlreadySet && deprecated != cmDiagnostics::Undefined) { - // CMD_DEPRECATED was not set by command-line options, but was altered by - // one or both of CMAKE_{WARN,ERROR}_DEPRECATED. - this->CurrentSnapshot.SetDiagnostic(cmDiagnostics::CMD_DEPRECATED, - deprecated, false); + for (DiagnosticAlteration const& da : this->DiagnosticAlterations) { + (this->CurrentSnapshot.*da.Alteration)(da.Category, da.DesiredAction, + da.Recurse); } // Now write the diagnostic state back to the cache. @@ -4527,6 +4518,21 @@ void cmake::RunCheckForUnusedVariables() #endif } +void cmake::AlterDiagnostic(DiagnosticAlterationMethod alteration, + cmDiagnosticCategory category, + cmDiagnosticAction desiredAction, bool recurse) +{ + // In Project mode, we need to defer applying any diagnostic changes until + // after reading the prior state from the cache. In all other modes, changes + // should be applied immediately. + if (this->State->GetRole() == cmState::Role::Project) { + this->DiagnosticAlterations.emplace_back( // clang-format: break + DiagnosticAlteration{ alteration, category, desiredAction, recurse }); + } else { + (this->CurrentSnapshot.*alteration)(category, desiredAction, recurse); + } +} + void cmake::SetDebugFindOutputPkgs(std::string const& args) { this->DebugFindPkgs.emplace(args); diff --git a/Source/cmake.h b/Source/cmake.h index 6344339e4f..8e81cf4851 100644 --- a/Source/cmake.h +++ b/Source/cmake.h @@ -817,6 +817,23 @@ private: cmStateSnapshot CurrentSnapshot; std::unique_ptr Messenger; + using DiagnosticAlterationMethod = + void (cmStateSnapshot::*)(cmDiagnosticCategory, cmDiagnosticAction, bool); + + struct DiagnosticAlteration + { + DiagnosticAlterationMethod const Alteration; + cmDiagnosticCategory const Category; + cmDiagnosticAction const DesiredAction; + bool const Recurse; + }; + + std::vector DiagnosticAlterations; + + void AlterDiagnostic(DiagnosticAlterationMethod alteration, + cmDiagnosticCategory category, + cmDiagnosticAction desiredAction, bool recurse); + #ifndef CMAKE_BOOTSTRAP bool SarifFileOutput = false; std::string SarifFilePath; diff --git a/Tests/RunCMake/Diagnostics/Persist1.cmake b/Tests/RunCMake/Diagnostics/Persist1.cmake new file mode 100644 index 0000000000..46c7215d57 --- /dev/null +++ b/Tests/RunCMake/Diagnostics/Persist1.cmake @@ -0,0 +1,4 @@ +include(Assertions.cmake) + +expect_cached(CMD_AUTHOR IGNORE) +expect_cached(CMD_DEPRECATED IGNORE) diff --git a/Tests/RunCMake/Diagnostics/Persist2.cmake b/Tests/RunCMake/Diagnostics/Persist2.cmake new file mode 100644 index 0000000000..161d5ed8b5 --- /dev/null +++ b/Tests/RunCMake/Diagnostics/Persist2.cmake @@ -0,0 +1,4 @@ +include(Assertions.cmake) + +expect_cached(CMD_AUTHOR WARN) +expect_cached(CMD_DEPRECATED WARN) diff --git a/Tests/RunCMake/Diagnostics/RunCMakeTest.cmake b/Tests/RunCMake/Diagnostics/RunCMakeTest.cmake index 924ef61f15..630d18229b 100644 --- a/Tests/RunCMake/Diagnostics/RunCMakeTest.cmake +++ b/Tests/RunCMake/Diagnostics/RunCMakeTest.cmake @@ -14,3 +14,18 @@ run_cmake_with_options(CommandLine1 -Werror=author -Wdeprecated) run_cmake_with_options(CommandLine1 -Wno-deprecated -Werror=author) run_cmake_with_options(CommandLine2 -Werror=author -Wno-deprecated) run_cmake_with_options(CommandLine3 -Wno-error=uninitialized -Winstall-absolute-destination) + +function(run_persist_test NAME) + set(RunCMake_TEST_VARIANT_DESCRIPTION "-first") + run_cmake_with_options(${NAME} ${ARGN}) + set(RunCMake_TEST_NO_CLEAN 1) + set(RunCMake_TEST_VARIANT_DESCRIPTION "-second") + run_cmake(${NAME}) +endfunction() + +block() + set(RunCMake_TEST_BINARY_DIR ${RunCMake_BINARY_DIR}/Persist-build) + run_persist_test(Persist1 -Wno-author) + set(RunCMake_TEST_NO_CLEAN 1) + run_persist_test(Persist2 -Wauthor) +endblock()