contracts: add violation handler infrastructure and annotated optional#7129
contracts: add violation handler infrastructure and annotated optional#7129Sanchit2662 wants to merge 10 commits intoTheHPXProject:masterfrom
Conversation
|
Can one of the admins verify this patch? |
|
@Sanchit2662 What's your plans with moving this forward? |
|
Hey @hkaiser , thanks for taking a look! Yeah so the plan is definitely not to keep this as a separate standalone thing. The idea is to integrate it properly with the existing assertion infrastructure in HPX. What I'm thinking is that The three modes map pretty naturally to what HPX already does. IGNORE is basically how release builds work today where For what to annotate first, I was thinking of following the same priority P3471 uses for the hardened STL. So hpx::optional first since it's a direct equivalent (operator*, operator->, value() all need has_value() to be true). Then HPX containers for out-of-bounds stuff, then On the macro side, the design I have in mind keeps a clean migration path. When a C++26 compiler with native contract support shows up, One thing I'm genuinely unsure about though: should the contract mode be its own CMake option like |
IIUC, you suggest adding an additional mode to the existing infrastructure that logs violations. That would be ok, I think.
As said, modifying
For pre-C++26 compilers this has to be a HPX CMake option. For conforming compilers this can be derived from the compiler options themselves. |
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 0 |
| Duplication | 0 |
TIP This summary will be updated as you push new changes. Give us feedback
|
Hey @hkaiser , thanks for the correction on hpx::optional, I didn't realize it was just an alias for std::optional on modern compilers so yeah annotating it doesn't really make sense. Also noticed the Codacy flags, looking into those now. |
If it is possible to have it separate, leave it separate. If this results in too much code duplication (or other issues), let's integrate it with the assertion module. The current contracts support lives in its own HPX module, maybe we should try to have everything related there. |
|
Hey @hkaiser , quick update on how I'm planning to move this forward so you have the full picture. First thing, I'll delete the whole poc/contracts/ directory from this branch. All of it, including the README. That code was just to validate the design, it doesn't need to live in the repo.
Planning to split this across separate commits so each piece is reviewable on its own instead of one giant diff. Let me know if any of this feels off before I start. Also looking into the Codacy flags, most of them are on the PoC files which will be gone anyway, but I'll go through them properly. |
hkaiser
left a comment
There was a problem hiding this comment.
Please force push to completely remove the poc subdirectory from the git history.
| // Copyright (c) 2025 The STE||AR-Group | ||
| // Copyright (c) 2025 Alexandros Papadakis | ||
| // Copyright (c) 2026 The STE||AR-Group |
There was a problem hiding this comment.
If you want to update the copyright for the STE||AR-Group (why would you, BTW), please consolidate the lines:
| // Copyright (c) 2025 The STE||AR-Group | |
| // Copyright (c) 2025 Alexandros Papadakis | |
| // Copyright (c) 2026 The STE||AR-Group | |
| // Copyright (c) 2025-2026 The STE||AR-Group | |
| // Copyright (c) 2025 Alexandros Papadakis |
| HPX_CORE_EXPORT void set_violation_handler( | ||
| violation_handler_t handler) noexcept; | ||
| HPX_CORE_EXPORT violation_handler_t get_violation_handler() noexcept; | ||
|
|
||
| HPX_CORE_EXPORT void default_violation_handler(violation_info const& info); | ||
| HPX_CORE_EXPORT void invoke_violation_handler(violation_info const& info); |
There was a problem hiding this comment.
Most likely these functions should be marrked with export if C++ modules are enabled:
| HPX_CORE_EXPORT void set_violation_handler( | |
| violation_handler_t handler) noexcept; | |
| HPX_CORE_EXPORT violation_handler_t get_violation_handler() noexcept; | |
| HPX_CORE_EXPORT void default_violation_handler(violation_info const& info); | |
| HPX_CORE_EXPORT void invoke_violation_handler(violation_info const& info); | |
| HPX_CXX_CORE_EXPORT HPX_CORE_EXPORT void set_violation_handler( | |
| violation_handler_t handler) noexcept; | |
| HPX_CXX_CORE_EXPORT HPX_CORE_EXPORT | |
| violation_handler_t get_violation_handler() noexcept; | |
| HPX_CXX_CORE_EXPORT HPX_CORE_EXPORT | |
| void default_violation_handler(violation_info const& info); | |
| HPX_CXX_CORE_EXPORT HPX_CORE_EXPORT | |
| void invoke_violation_handler(violation_info const& info); |
|
|
||
| namespace hpx::contracts { | ||
|
|
||
| enum class contract_kind |
There was a problem hiding this comment.
The defined data types most likely need annotations for C++ modules:
| enum class contract_kind | |
| HPX_CXX_CORE_EXPORT enum class contract_kind |
same is true for the struct violation_info below.
| # Contract evaluation mode for pre-C++26 compilers. Not registered via | ||
| # hpx_option(MODULE CONTRACTS) to avoid triggering the config_entries.cpp | ||
| # generation mechanism before add_hpx_module creates the build directory. | ||
| set(HPX_WITH_CONTRACTS_MODE |
There was a problem hiding this comment.
You can use hpx_option here:
hpx_option(
HPX_WITH_CONTRACTS_MODE
STRING
"Contract evaluation mode for pre-C++26 compilers, options are: ENFORCE, OBSERVE, IGNORE"
"ENFORCE"
STRINGS "ENFORCE;OBSERVE;IGNORE"
)
There was a problem hiding this comment.
Also, should we make ENFORCE the default in Debug mode only?
There was a problem hiding this comment.
That's a good idea. How should we handle multi-config generators though, where CMAKE_BUILD_TYPE is empty at configure time? Is there a preferred pattern for this in HPX already?
|
|
||
| if(HPX_WITH_CONTRACTS_MODE STREQUAL "ENFORCE") | ||
| hpx_add_config_define_namespace( | ||
| DEFINE HPX_CONTRACTS_MODE |
There was a problem hiding this comment.
We usually use HPX_HAVE_xxx as the preprocessor macro for a corresponding HPX_WITH_xxx CMake option.
| }; | ||
|
|
||
| HPX_CXX_CORE_EXPORT using violation_handler_t = | ||
| void (*)(violation_info const&); |
There was a problem hiding this comment.
Would it be better if this was accepting any callable of the given signature:
| void (*)(violation_info const&); | |
| hpx::unique_function<void(violation_info const&)> |
?
There was a problem hiding this comment.
just to confirm , when you said hpx::unique_function , did you mean hpx::move_only_function? That's what I found in the codebase. Also switching from a raw function pointer to a move-only wrapper would change the internal storage in violation_handler.cpp ; since we can't hold it in a plain static pointer anymore.
Makes sense to me if that's the direction, just wanted to confirm before changing it.
There was a problem hiding this comment.
I meant move_only_function, sorry for the confusion.
| HPX_CXX_CORE_EXPORT using violation_handler_t = | ||
| void (*)(violation_info const&); | ||
|
|
||
| HPX_CORE_EXPORT void set_violation_handler( |
There was a problem hiding this comment.
This function should return the previously registered violation handler to allow chaining several handlers, if needed.
| // Distributed under the Boost Software License, Version 1.0. (See accompanying | ||
| // file LICENSE_1_0.txt or copy at http://www.boost.org/LICENSE_1_0.txt) | ||
|
|
||
| #include <hpx/contracts/config/defines.hpp> |
There was a problem hiding this comment.
You may need to #include<hpx/config.hpp>
Signed-off-by: Sanchit2662 <sanchit2662@gmail.com>
Signed-off-by: Sanchit2662 <sanchit2662@gmail.com>
Signed-off-by: Sanchit2662 <sanchit2662@gmail.com>
Signed-off-by: Sanchit2662 <sanchit2662@gmail.com>
e2595a6 to
7dfefd1
Compare
Signed-off-by: Sanchit2662 <sanchit2662@gmail.com>
|
so i've addressed the review feedback. |
hpx_add_config_define_namespace treats a numeric "0" argument as falsy and emits a valueless #define, which caused the #if HPX_HAVE_CONTRACTS_MODE preprocessor check to fail with a syntax error on every build job. Shift the mode encoding to 1/2/3 (ENFORCE/OBSERVE/IGNORE) and guard the checks with defined() so the macro is robust to the emit path. Also update the fallback test WILL_FAIL expectations: HPX_CONTRACT_ASSERT now routes through the violation handler in every build config, so tests that hit it must WILL_FAIL unconditionally under ENFORCE (the default), not only in Debug.
…itions Adds HPX_PRE(this->valid()) to both get() overloads as the first real end-to-end use of the contracts infrastructure. Under native C++26 the compiler enforces the pre-condition; in fallback mode HPX_PRE is a no-op and the existing no_state throw continues to handle invalid futures. Also fold in clang-format and cmake-format fixes flagged by CI on the previous commit. Signed-off-by: Sanchit2662 <sanchit2662@gmail.com>
enforced pre-conditions; under fallback they are no-ops, matching the design of HPX_PRE as a declaration specifier. Signed-off-by: Sanchit2662 <sanchit2662@gmail.com>
Signed-off-by: Sanchit2662 <sanchit2662@gmail.com>
|
While scaling up the annotations I ran into a design question I'd like your input on before I go much further. Right now HPX_PRE and HPX_POST are pure no-ops in fallback mode, regardless of HPX_WITH_CONTRACTS_MODE. The reasoning is in the macros.hpp comment: they're declaration specifiers in C++26 ( The consequence is that an annotation like HPX_PRE(this->valid()) on future::get() only does anything under a C++26-with-contracts compiler. In every other build ,which is every CI config today it's documentation only. That makes the ENFORCE/OBSERVE/IGNORE modes effectively useless for HPX_PRE/HPX_POST; they only affect HPX_CONTRACT_ASSERT. Two ways I can see to resolve this:
My lean is (1) for simplicity, but (2) is more useful in the near term. Which way would you like to go? |
That's a tough call. However, (1) is preferred as it avoids code duplication. We should combine that with a CI that actually supports contracts (similar to what we have added for the C++ reflection capabilities). It might be worth thinking about whether we can find expansions for |
Answering my own comment: this is probably not possible as both, the |
|
Hey @hkaiser , glad we're aligned on option (1) , that keeps things clean and avoids a parallel API that we'd eventually have to maintain. Before I go ahead and write the workflow file though, I had a few questions I wanted to check with you: First, do you know if we can reuse the existing gcc_trunk_build_env:1 image? Since GCC 15 already has -fcontracts support, if that image was built against GCC 15 or newer, we might not need a new image at all , just a new YAML file pointing at the same container. That'd be the simplest path. If a fresh image is needed (something like gcc_contracts_build_env:1), I'm happy to put together a Dockerfile for it , but I'd need someone with access to the stellargroup Docker Hub to actually build and push it. Is that something you'd handle, or is there a process I should follow for that? And lastly, any preference on whether we go GCC-only for now, or also add a Clang variant? The Clang contracts implementation is still pretty experimental upstream, so GCC-only feels like the safer starting point to me , but happy to go either way if you have a preference. |
…ctor with preconditions Add HPX_PRE(first <= last) to hpx::sort and hpx::stable_sort (both sequential and parallel overloads). Both algorithms already enforce std::random_access_iterator via static_assert, so <= is always valid. Add HPX_PRE(pos < size()) to partitioned_vector::operator[] (const and non-const). Mirrors the existing small_vector annotation. Signed-off-by: Sanchit2662 <sanchit2662@gmail.com>
135db57 to
50e9ea1
Compare
|
Just as a FYI: https://wg21.link/p4044 |
What this is
A standalone, buildable contracts infrastructure - violation handler with enforce/observe/ignore mode dispatch, the macro layer (
HPX_CONTRACT_ASSERT,HPX_PRE,HPX_POST), and a minimaloptional<T>annotated with P3471 preconditions. Lives inpoc/contracts/and builds independently of HPX's CMake so anyone can run it without a full HPX build.This is a proof-of-concept : not the real integration, just enough to show the approach is sound.
What I built
Three files that form the core of what the full project would need:
violation_handler.hpp- aviolation_infostruct that mirrors whatstd::contract_violationwill look like in C++26, plus a customizable handler (default: log to stderr + abort). This is the same pattern HPX already uses forassertion_handler.contract_macros.hpp-HPX_CONTRACT_ASSERT,HPX_PRE,HPX_POSTmacros that dispatch to the right behavior depending on a compile-time mode flag.annotated_optional.hpp- a minimaloptional<T>withHPX_CONTRACT_ASSERT(has_value())onoperator*,operator->, andvalue()- exactly the P3471 preconditions, and exactly what was done in the realoptional.hppon thefeat/contracts-optional-operator-starbranch.Plus a
contracts_poc.cppwith 20 tests that run across all three modes and aCMakeLists.txtthat builds all three at once.How it works
The key idea is compile-time mode selection:
-DHPX_CONTRACTS_MODE=0→ ENFORCE: check predicate, call handler, abort on violation-DHPX_CONTRACTS_MODE=1→ OBSERVE: check predicate, call handler, continue-DHPX_CONTRACTS_MODE=2→ IGNORE: predicate never evaluated (zero cost)These map directly to P2900's three semantics. In IGNORE mode
HPX_CONTRACT_ASSERT(expr)compiles to((void)0)- zero overhead.The violation handler is user-installable (same API as
hpx::assertion::set_assertion_handler), which makes it testable - instead of aborting, tests install a recording handler that captures violations and check them.Build it yourself
What this isn't
This is not the actual implementation. Specifically:
.cppunderlibs/core/contracts/src/withHPX_CORE_EXPORTlinkage.HPX_ASSERTat all. The real integration (viaHPX_CONTRACTS_WITH_ASSERTS_AS_CONTRACT_ASSERTS) is already in the existing contracts module.optional<T>here is a standalone toy. The actual annotation is already on thefeat/contracts-optional-operator-starbranch in the realoptional.hpp.Questions for reviewers
The violation handler is
noexceptthroughout (matching the existinghandle_assert). Is that right for observe mode too, or should we allow the handler to throw in observe mode to integrate with HPX's exception-based error propagation?For
HPX_PRE/HPX_POSTin fallback mode - I currently put them as body statements (top of function / before return). When native contracts land, they move to the declaration. Does HPX want a migration story for that, or is it acceptable that fallback-mode and native-modeHPX_PREare syntactically in different positions?Is the priority order right - containers first, then futures, then parallel algorithms? Or should futures come first since
future::get()on an invalid future is probably the most common real-world bug?