Skip to content

CXX-2745 add immortal.hh and type_traits.hh #1402

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

eramongodb
Copy link
Contributor

@eramongodb eramongodb commented May 13, 2025

Related to CXX-2745. Precursor changes to CXX-3232.

Adds two new internal bsoncxx components which are used by upcoming v1 interfaces.


The bsoncxx::immortal<T> class is used to define error category objects in a manner that avoids generating exit-time destructors (hence, "immortal") as can be diagnosed by the -Wexit-time-destructors Clang warning.

The pattern will look as follows:

std::error_category const& example() {
    class type final : public std::error_category { ... };

    // The error category object instance. This is never destroyed!
    static bsoncxx::immortal<type> const instance;

    return instance.value();
}

Several new type traits are introduce to ensure consistency and correctness of v1 interfaces via static assertions. These type traits are derived from patterns observed in v_noabi interfaces and are enforced by v1 interfaces in a more rigorous manner.

The new type traits are:

  • is_semitrivial<T>:
    • T is trivial to copy, move, and destroy ("like an int").
    • Trivial default constructibility is excluded (hence "semi") due to most types requiring non-trivial construction (to enforce class invariants) even when other special member functions are all trivial (invariants are entirely enforced by initialization).
  • is_semiregular<T> and is_regular<T>:
    • T has value semantics ("like an int or std::string") and is (optionally) equality-comparable.
    • is_semitrivial<T> implies is_semiregular<T>.
  • is_nothrow_moveable<T>:
    • T does not throw on destruction, move construction, or move assignment ("like a std::unique_ptr<T>").
    • This is meant to roughly approximate the requirements for is_nothrow_relocatable<T> in C++26 without misleadingly implying actual nothrow relocatability (which is complicated to accurately specify). This type trait is primarily to catch missing noexcept specifiers and highlight immovable types.
    • is_semitrivial<T> implies is_nothrow_moveable<T>.

All class types in bsoncxx and mongocxx are expected to (eventually) satisfy semiregular and nothrow moveable at a minimum to ensure consistent user-friendly behavior (e.g. bsoncxx::v_noabi::document::value is notably not semiregular). Some types may additionally support semitrivialty. Some types may additionally support regularity (equality comparison). If there are any circumstances where nothrow moveability is deliberately violated (potentially-throwing move operations or immovability), we should expect such instances to be highlighted with a negated static assertion ("we do something unusual here!") such as for mongocxx::v_noabi::instance (an immovable class type).

Important

Move constructors and move assignment operators which may allocate memory are still declared noexcept. std::bad_alloc exceptions are NOT accounted for by nothrow moveability, as we do not support recovering from out-of-memory situations in either the C++ or C Driver libraries.

@eramongodb eramongodb requested a review from kevinAlbs May 13, 2025 21:22
@eramongodb eramongodb self-assigned this May 13, 2025
@eramongodb eramongodb requested a review from a team as a code owner May 13, 2025 21:22
@kevinAlbs kevinAlbs requested a review from vector-of-bool May 14, 2025 17:41
Copy link
Contributor

@vector-of-bool vector-of-bool left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry to delay. Only minor comments, but otherwise looks good.

Comment on lines +69 to +76
// Equivalent to `is_trivial` without requiring trivial default constructibility.
template <typename T>
struct is_semitrivial : detail::conjunction<
std::is_trivially_destructible<T>,
std::is_trivially_move_constructible<T>,
std::is_trivially_move_assignable<T>,
std::is_trivially_copy_constructible<T>,
std::is_trivially_copy_assignable<T>> {};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is "semitrivial" a (proposed) standard trait? I've not heard the term, but it is reasonably useful.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, not as far as I am aware.

Comment on lines +23 to +24
template <typename T>
class immortal {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a doc comment on what this class template does any why it might want to be used, and how it should not be used (e.g. declaring a non-static immortal<T> is potentially bad).

Comment on lines 35 to 46
template <typename... Args>
immortal(Args&&... args) {
new (_storage) T(BSONCXX_PRIVATE_FWD(args)...);
}

T const& value() const {
return *reinterpret_cast<T const*>(_storage);
}

T& value() {
return *reinterpret_cast<T*>(_storage);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add (conditional) noexcept here. Possibly an explicit on the constructor?

Copy link
Contributor Author

@eramongodb eramongodb May 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd meant to discuss this in the upcoming v1 ABI namespace declarations PR, but will initiate the discussion here since the topic has been mentioned. (CC @kevinAlbs.)


The v1 ABI interface intends to implement "Policy Statement E - Minimal noexcept (MIN)" as specified by P3005R0 (section 4.1.1.1) going forward in a deliberate break from consistency with existing interfaces. (AFAIK there is no established noexcept policy yet for the codebase; various policies have been inconsistently applied to the codebase over time.)

Quoting the MIN policy verbatim:

Only those functions with a sanctioned need (e.g., practical application of the noexcept operator) to be marked noexcept shall be marked unconditionally noexcept. Specifically, move constructors, move assignment operators, and the swap function have all been shown to be used in algorithms where significant algorithmic improvements can be made when these functions can be determined, at compile time, to not throw. No other functions shall be marked noexcept absent an update to this policy.

This policy can be summarized as:

Use noexcept only for:

  • move constructors,
  • move assignment operator overloads, and
  • swap overloads

when they are guaranteed to not throw.

The reason for this is to avoid burdening code reviews and maintainability of the codebase with noexcept correctness evaluations to support often-hypothetical use cases which are likely irrelevant to the actual usage or needs of the C++ Driver libraries. Given the expected usage of the C++ Driver, I believe using noexcept beyond these limited known-and-well-established cases listed above will continue to unnecessarily complicate code reviews and maintenance now and going forward should we adopt an alternative policy. (This is despite my personal preference for "Policy Statement C - Maximal noexcept with Lakos Rule and C Exemption (MAX+LAK+NLC)".)

Note

As an aside, we already effectively implement "No Lakos Rule on C Compatibility (NLC)" (section 4.1.1.2) due to the C Driver not supporting exceptions (e.g. the log handler callback), but we haven't yet propagated this requirement into the public API for APM callbacks yet, whose function types should be marked noexcept, but std::function<T> deficiencies unfortunately limit our available options here. Other policies and decisions listed in the paper such as NLM, POR, NTD, and NCP are not relevant to our situation.

For example, the suggestion to apply (conditional) noexcept to immortal<T> here is consistent with "Maximum noexcept (MAX)" (aka "use noexcept to document and enforce an in-contract nothrow guarantee"), "Lakos Rule (LAK)" (aka "if there are preconditions, do not use noexcept") and "Allow Conditional noexcept (CNN)" (specifically CN2: "wrapping (aka proxy) semantics"). The reason I propose we avoid any of these policies going forward is:

  • MAX: I believe it is challenging to review and maintain that the codebase truly does not throw any exceptions when in contract (even discounting std::bad_alloc, which we do not support). The existing v_noabi interfaces are under-documented regarding whether a given function may throw an exception, and there are many functions whose implementations should be throwing an exception for erroneous runtime conditions (i.e. missing return value checks) but are not. A codebase which diligently ensured all nothrow functions are consistently denoted with noexcept from the start that also fully accounts for thorough error handling might be able to get away with this policy; I do not think the C++ Driver is such a codebase. I believe adopting the MAX policy will risk too much opportunity for over-eager and (arguably) incorrect application of noexcept that may result in a poor user experience (irrecoverable termination as opposed to an unexpected but recoverable exception). See also the addendum below.
  • LAK: for similar reasons, I believe this is unnecessarily challenging to review and maintain for our codebase. This is arguably even more complicated than simply adopting MAX due to requiring a thorough understanding and documentation of narrow vs. wide contracts. This is difficult to apply to the C++ Driver due to the lack of thorough assertions, validation, or documentation of preconditions (let alone exception safety). I believe it will be difficult to consistently and correctly apply LAK to all but the most trivial functions as the codebase continues to evolve and as missing runtime checks continue to be identified and addressed.
  • CNN: I believe this is overkill for anything beyond internal highly-generic utilities (e.g. detail/compare.hpp) and does not apply to most of the codebase (including immortal<T>: who/when/why will anyone care whether its ctor or .value() is noexcept?).

Therefore, I propose we adopt the MIN noexcept policy going forward for the C++ Driver. It is the easiest to identify, review, and maintain (limited to nothrow moves and nothrow swap) and is the most forgiving and permissive policy w.r.t. respecting API backward compatibility guarantees that will allow us to continue improving the state of our error-handling and precondition assertion behavior and documentation with minimal resistance or premature commitment to excessive nothrow guarantees. Under typical circumstances, we should not impose or deal with the cognitive burden of potential noexcept termination on ourselves or our users; consideration and use of noexcept can be limited to the bare minimum (move+swap and C compat).

Please let me know if you have any thoughts or opinions against adopting this proposal.

Note

As an addendum, MongoDB Server's coding guidelines state:

The noexcept feature is easy to overuse. Do not use it solely as "documentation" since it affects runtime behavior. It's a large topic, covered in the Exception Architecture document.

The referenced Exception Architecture document is littered with warnings concerning potential overuse of noexcept:

Server code should generally be written to be exception safe. Historically, we've had bugs due to code being overzealously marked noexcept. In such contexts, throwing an exception crashes the server, which can compromise availability. However, just removing noexcept from such code is not a viable solution [...] We want to work towards ensuring that functions that ought to be are in fact exception safe, and remove noexcept usage where it's not warranted. [...] be very careful when putting noexcept on a function that interacts with untrusted input. This has been the root cause of serious past bugs. [...] be careful putting noexcept on a function if there’s a chance it may need to be removed later. noexcept generally should not be used solely for reasons of performance optimization.

The only situations listed where noexcept is explicitly encouraged as valid use cases are the aforementioned "move operations" and "swap operations" (consistent with MIN), "destructors and 'destructor-safe' functions" (which we assume to be a given), and "hash functions" (also consistent with MIN, but not applicable to our codebase).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the detailed write-up. I very much support limiting noexcept to the most practical value. I expect applying noexcept widely adds complexity and risks incorrectly marking a function that really throws.

noexcept can be limited to the bare minimum (move+swap and C compat).

As I understand it, the reasoning to use noexcept in move+swap and C Compat differ (but I agree with both):

  • move+swap: use noexcept for practical performance improvement.
  • C Compat: use noexcept on C++ functions called by C to ensure defined behavior. (i.e. terminate if the C++ function unexpectedly throws).

Copy link
Contributor Author

@eramongodb eramongodb May 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

move+swap: use noexcept for practical performance improvement.

Correct, but specifically/importantly, for practical performance improvement due to known use of the noexcept operator. Paraphrasing P3005R0 (page 13):

Nearly 15 years of experience with noexcept [by the committee] has revealed that applying noexcept superfluously [...] can be highly detrimental to writing safe and reliable software. Therefore, [we] must scrupulously reconsider when noexcept is truly needed, which is when a function must — not just theoretically but in practice — have the noexcept operator applied to it (i.e., in a generic context) to enable an algorithmically better approach to be taken by a templated function. Up to now, only move construction and move assignment are used in this way by generic functions in the Standard Library itself, while swap is similar enough to potentially be useful in algorithms from other libraries. No other functions in any library have ever been known to demonstrate such value. Hence, until an existence proof can be demonstrated [to us], the argument can be made that no other functions shall be candidates for being marked as unconditionally noexcept.


C compat: use noexcept on C++ functions called by C to ensure defined behavior (i.e. terminate if the C++ function unexpectedly throws).

Correct, as described by "No Lakos Rule on C Compatibility (NLC)" on page 14:

That a C++ library function, such as those in the <atomics> API, intended to mirror a C library function could never throw was perfectly obvious. Hence, the original LEWG policy on the use of noexcept included an exemption for library functions designed for compatibility with C code. [...] A library function designed for compatibility with C code that does not throw in contract may always be marked as unconditionally noexcept, even when it has a narrow contract.

In our case, as described in #1015, rather than the (re)declaration of a C++ equivalent of a C library function, we are dealing with callback functions passed to the C library (mongoc) which may potentially throw an exception. It is undefined behavior for an exception to be thrown from within such a callback, as the C library is not designed to handle any exceptions being thrown from within it (that is, all callbacks should effectively behave like a C function). Therefore, any such callback function should be unconditionally noexcept as a matter of correctness and safety, ensuring what would otherwise be undefined behavior is instead well-defined program termination. (This is technically not NLC because we are not making an exemption to LAK, which we would not be applying either, but it's convenient to refer to it as such anyways for this discussion.)

@eramongodb
Copy link
Contributor Author

Re-requesting reviews to solicit feedback regarding the adoption of a new noexcept policy going forward.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants