RoNodeMethods should have conditional noexcept

[yrHeTaTeJlb]

Throwing error callback

c4::yml::set_callbacks({ nullptr, nullptr, nullptr, [](const char*, std::size_t, c4::yml::Location, void*){ throw 42; } });

may lead to abort() in that case

ryml::ConstNodeRef node = Tree.rootref();
c4::csubstr invalidName = /*...*/;
node[invalidName];

bacuse noexcept operator[] calls this callback

    C4_ALWAYS_INLINE C4_PURE ConstImpl operator[] (csubstr k) const noexcept
    {
        _C4RV();
        size_t ch = tree_->find_child(id_, k);
        _RYML_CB_ASSERT(tree_->m_callbacks, ch != NONE);
        return {tree_, ch};
    }

I saw you mentioned that I can't return from error callback, so basically I can either call abort() or throw an exception, which in turn also leads to abort(). Config parsing error is an expected scenario in my case. So I'd like to handle such errors(even in debug build), rather than crashing the whole app

[biojppm]

TL;DR: Not an issue with the library. operator[] is only for the happy-path; if the child may not exist, you cannot use operator[]. Use instead .find_child() and then query the result with .valid().

---

operator[] is an analogue to the one in std::vector, which is non-throwing, and embodies the same spirit, where on optimized builds nothing is checked. Thus, the assert is there merely as a scaffold to help during development, so it is enabled only on debug builds (or to be more precise, whenever _RYML_CB_ASSERT is enabled). And accordingly, the noexcept is justified on the grounds that operator[] is meant only for the happy-path.

So relying on a call to the error callback from operator[] to track non-existing children is not safe - the callback will not happen on optimized builds, and you will have a buffer overflow or silent failure.

This means it is your responsibility to ensure that the wanted child/index looking is actually there, so if that may not be the case with your data, you should not use operator[], but use .find_child() instead, and then check if the return value is .valid().

So in your case you should do something like this:

// ERROR: not checked in optimized builds.
// aborts if either a or b are invalid names.
ConstNodeRef b = root["a"]["b"];

// OK, no calls to abort if any name is invalid
// (and throws if either is invalid)
ConstNodeRef a = root.find_child("a");
if(!a.valid()) throw YourError("a");
ConstNodeRef b = a.find_child("b");
if(!b.valid()) throw YourError("b");
// etc

Having said that, I agree the noexcept could be adjusted to be conditionally disabled when _RYML_CB_ASSERT() is enabled.

I'll keep this open to track that issue. Also, I see that there is no RoNodeMethods::at() analogue to std::vector::at(). This will be a good occasion to add such a function.

[biojppm]

Self-reminder: clarify this issue on the documentation.

[jdrouhard]

I think this is actually an issue in rapidyaml.

When you install an error handler via ryml::Callbacks that throws (instead of the default abort), it's expected that any kind of parsing or usage error will result in a catchable exception being thrown. But because most of the functions that use the callback's current error handler are also marked noexcept, it defeats the purpose of changing the error behavior completely. The exception will never escape rapidyaml and the program will terminate.

I'd like to be able to use this library but I can't let the library cause program termination and would rather handle errors via exceptions, but because of all the noexcept functions, it's currently impossible.

There is too much surface area in rapidyaml's API where the error callback is called from a noexcept function. Removing noexcept shouldn't incur much of a performance penalty (if any at all).

[jdrouhard]

@biojppm (tagging you in case GitHub notifications are messed up).

Anyway, the issue seems to be the always inlined property getters and predicates in node.hpp. The functions these forward to in tree are already properly not noexcept (and have their own assertions that may use the error callback).

    /** @name node property getters */
    /** @{ */

    /** returns the data or null when the id is NONE */
    C4_ALWAYS_INLINE C4_PURE NodeData const* get() const noexcept { RYML_ASSERT(tree_ != nullptr); return tree_->get(id_); }
    /** returns the data or null when the id is NONE */
    template<class U=Impl>
    C4_ALWAYS_INLINE C4_PURE auto get() noexcept -> _C4_IF_MUTABLE(NodeData*) { RYML_ASSERT(tree_ != nullptr); return tree__->get(id__); }

    C4_ALWAYS_INLINE C4_PURE NodeType    type() const noexcept { _C4RV(); return tree_->type(id_); }
    C4_ALWAYS_INLINE C4_PURE const char* type_str() const noexcept { return tree_->type_str(id_); }

    C4_ALWAYS_INLINE C4_PURE csubstr key()        const noexcept { _C4RV(); return tree_->key(id_); }
    C4_ALWAYS_INLINE C4_PURE csubstr key_tag()    const noexcept { _C4RV(); return tree_->key_tag(id_); }
    C4_ALWAYS_INLINE C4_PURE csubstr key_ref()    const noexcept { _C4RV(); return tree_->key_ref(id_); }
    C4_ALWAYS_INLINE C4_PURE csubstr key_anchor() const noexcept { _C4RV(); return tree_->key_anchor(id_); }

    C4_ALWAYS_INLINE C4_PURE csubstr val()        const noexcept { _C4RV(); return tree_->val(id_); }
// ...

[jdrouhard]

operator[] is an analogue to the one in std::vector, which is non-throwing, and embodies the same spirit, where on optimized builds nothing is checked. Thus, the assert is there merely as a scaffold to help during development, so it is enabled only on debug builds (or to be more precise, whenever _RYML_CB_ASSERT is enabled).

Sorry, last comment :)

There's another issue with this: https://github.com/biojppm/rapidyaml/blob/master/src/c4/yml/common.hpp#L213

diff --git a/src/c4/yml/common.hpp b/src/c4/yml/common.hpp
index f74de9d..c30d733 100644
--- a/src/c4/yml/common.hpp
+++ b/src/c4/yml/common.hpp
@@ -210,7 +210,7 @@ do                                                                      \
             (cb).m_error(msg, sizeof(msg), c4::yml::Location(__FILE__, 0, __LINE__, 0), (cb).m_user_data); \
         }                                                               \
     } while(0)
-#ifdef RYML_USE_ASSERT
+#if RYML_USE_ASSERT
 #define _RYML_CB_ASSERT(cb, cond) _RYML_CB_CHECK((cb), (cond))
 #else
 #define _RYML_CB_ASSERT(cb, cond) do {} while(0)

Otherwise, these asserts are triggered even in release builds since RYML_USE_ASSERT will always be defined (it will either be 0 or 1 depending on C4_USE_ASSERT, see lines 9-11 and c4core/src/c4/error.hpp:252)

[Neko-Box-Coder]

I am not sure if this is the same issue or not or a similar one.

So both operator[] and find_child() have noexcept which is fine, and we can decide what to do in the error callback.

However, since we are not allowed to "return", all the options left for us are really just to terminate the program, but in different way (abort() vs exception).

It would be fine if I can catch it but the try catch doesn't work even it is surrounding the caller so I have no way to return to the caller and go to a different path to handle the error with context on what's going on.

I think that's the whole point of vector.at() vs vector operator[] where you can catch exceptions for at() and operator[] will just be UB or do nothing if invalid.

I personally think a library should provide the option of handling errors without terminating the program. If I am creating a program that should not be terminating, the only thing I can hope for is I have put checks everytime before I access something, instead of relying on a result or exception catching.

But anyway, I think this is a very good library and I really do appreciate it, especially the ability to parse merge keys which is huge (compare to yaml-cpp). Thank you for creating it :)

[biojppm]

@jdrouhard Sorry for the late reply. Notifications are broken for me, and I only saw this now.

As for the issue, like I said in my reply to the OP, and as the issue title states, the problem is indeed all the methods in the node class, which should be conditional noexcept.

[biojppm]

@Neko-Box-Coder It does look to be the same problem, where several methods were carpet-bombed with noexcept and shouldn't have been.

What is the ryml method calling the error callback? Is that method noexcept? Then that's why you are failing to catch the exception.

Otherwise, please post a minimal example reproducing your problem.

[Neko-Box-Coder]

@biojppm
I think technically all the ones that can call the error callback should not be noexcept, like you said maybe a conditional noexcept would be good.

There are a few ones that are calling error callback but I don't remember all.
The one I have right now is has_child() if there isn't a value. So for example:

test:
  # Commented out child
  # test-child: test-value

Then it will error out with is_map() assertion I believe because it evalues to keyval instead