Fix CI + new Doctest

[nlohmann]

Same as #3978, but with recent Doctest release.

[coveralls]

Coverage: 100.0%. Remained the same when pulling d9f2db4 on fix_ci_new_doctest into 546370c on develop.

[LecrisUT]

I believe you know how to fix this issue right, you just haven't had the time? It seems that it is is just a clang_tidy detecting issue. Mostly it seems to suggest to change the order to:

basic_json(basic_json&& other) noexcept
        : m_data(std::move(other.m_data)),
          json_base_class_t(std::move(other))

But maybe you are referring to a different issue?

[nlohmann]

I believe you know how to fix this issue right, you just haven't had the time? It seems that it is is just a clang_tidy detecting issue. Mostly it seems to suggest to change the order to:

basic_json(basic_json&& other) noexcept
        : m_data(std::move(other.m_data)),
          json_base_class_t(std::move(other))

I have not thought much about it. I can try tonight whether changing the order is fixing it indeed.

But maybe you are referring to a different issue?

Yes, the other one is the actual blocker.

[nlohmann]

I believe you know how to fix this issue right, you just haven't had the time? It seems that it is is just a clang_tidy detecting issue. Mostly it seems to suggest to change the order to:

basic_json(basic_json&& other) noexcept
        : m_data(std::move(other.m_data)),
          json_base_class_t(std::move(other))

I have not thought much about it. I can try tonight whether changing the order is fixing it indeed.

But maybe you are referring to a different issue?

Yes, the other one is the actual blocker.

[nlohmann]

I believe you know how to fix this issue right, you just haven't had the time? It seems that it is is just a clang_tidy detecting issue. Mostly it seems to suggest to change the order to:

basic_json(basic_json&& other) noexcept
        : m_data(std::move(other.m_data)),
          json_base_class_t(std::move(other))

I have not thought much about it. I can try tonight whether changing the order is fixing it indeed.

But maybe you are referring to a different issue?

Yes, the other one is the actual blocker.

[nlohmann]

I believe you know how to fix this issue right, you just haven't had the time? It seems that it is is just a clang_tidy detecting issue. Mostly it seems to suggest to change the order to:

basic_json(basic_json&& other) noexcept
        : m_data(std::move(other.m_data)),
          json_base_class_t(std::move(other))

I have not thought much about it. I can try tonight whether changing the order is fixing it indeed.

But maybe you are referring to a different issue?

Yes, the other one is the actual blocker.

[LecrisUT]

Oh, now I see it is:

/__w/json/json/include/nlohmann/detail/conversions/to_chars.hpp:912:2: error: preprocessor condition is always 'false', consider removing both the condition and its contents [readability-avoid-unconditional-preprocessor-if,-warnings-as-errors]
#if 0

So basically clang-tidy has made the checks stricter and you want to resolve all of these issues? Not sure why the original code had the #if 0. It dates back to 2015, and maybe it should just be removed at this point.

About the previous issue, I have tried that and it didn't work. But this one works

basic_json(basic_json&& other) noexcept
        : json_base_class_t(std::forward<json_base_class_t>(other)),
          m_data(std::move(other.m_data))
    {
        // check that passed value is valid
        other.assert_invariant(false);
    }

Reading about it a bit, my understanding is that when you use std::move and downcast it doesn't actually move it (pass it as a lvalue? I am still fuzzy on l-r value definition) but rather pass it as a reference. Because of that, I am indeed confused why people use std::move in those situation instead of std::forward. For me the latter sounds more like what is actually happening. (reference)

[gregmarr]

basic_json(basic_json&& other) noexcept
    : json_base_class_t(std::forward<json_base_class_t>(other)),
      m_data(std::move(other.m_data))
{
    // check that passed value is valid
    other.assert_invariant(false);
}

This is a potential use after move bug, but since it's the base class type that's being passed along, it is probably okay as long as the base class doesn't do anything weird.

Having said that, the current code should have the same potential use-after-move bug, because the order of the member inits doesn't actually effect the order in which the operations happen. The base class init always happens first, and then the members are initialized in declaration order.

Reading about it a bit, my understanding is that when you use std::move and downcast it doesn't actually move it (pass it as a lvalue? I am still fuzzy on l-r value definition) but rather pass it as a reference. Because of that, I am indeed confused why people use std::move in those situation instead of std::forward. For me the latter sounds more like what is actually happening.

The std::move() function could have been more accurately named std::rvalue_cast(), which in this case causes the move constructor to be called. It doesn't need to be told what type the variable is, since it uses the actual type and just converts it from an lvalue (a named value) to an rvalue (an unnamed value).

The std::forward<>() function takes a template type and is primarily intended for cases where the original type of the parameter being forwarded has already been determined, and you want to pass it along as that exact type.

[LecrisUT]

This is a potential use after move bug, but since it's the base class type that's being passed along, it is probably okay as long as the base class doesn't do anything weird.

Indeed that felt weird to me, but when I googled how to write a move constructor of a derived class, that is what was suggested. Do you know of a better guide on that topic, maybe one that explains the inner-workings.

Having said that, the current code should have the same potential use-after-move bug

From what I read, when you downcast and std::forward, it would not do an std::move (passed as rvalue instead of lvalue iiuc). That is why clang-tidy does not consider it a bug compared to std::move which specifies the intent that you do not have access to it afterwards.

std::rvalue_cast() is not an actual standard, just a clarification of what it is internally right? Is std::forward<>() a bad design choice in this case and std::move() should be used instead with linting ignore for that? I do think that at least grammatically this is clearer that there is not an actual std::move() and instead we are simply calling the constructor json_base_class_t(json_base_class_t&&).

But as a side-note, what happens to other object after the constructor is completed? Assuming that all fields are properly moved and copied, the original address where that was in is just marked as dealocated (e.g. if there are fields that are not moveable and are copied instead, what happens to those)?

[gregmarr]

Indeed that felt weird to me, but when I googled how to write a move constructor of a derived class, that is what was suggested. Do you know of a better guide on that topic, maybe one that explains the inner-workings.

Nope. If that is indeed a recommended pattern, then I guess it's probably safe, as I was thinking.

From what I read, when you downcast and std::forward, it would not do an std::move (passed as rvalue instead of lvalue iiuc). That is why clang-tidy does not consider it a bug compared to std::move which specifies the intent that you do not have access to it afterwards.

It's passing json_base_class_t&& to the base class move constructor, where std::move() would pass basic_json&&. I'm assuming then the analysis tools are smart enough to know that the basic_json added fields are still intact.

std::rvalue_cast() is not an actual standard, just a clarification of what it is internally right?

Yes.

But as a side-note, what happens to other object after the constructor is completed? Assuming that all fields are properly moved and copied, the original address where that was in is just marked as dealocated (e.g. if there are fields that are not moveable and are copied instead, what happens to those)?

A moved-from object is specified to be left in a valid-but-unspecified state. The object is still live, but you can't assume that any of its invariants hold. You can only safely call functions with no preconditions, unless you know the details of exactly what was done to the object during the move operation.

[LecrisUT]

I'm assuming then the analysis tools are smart enough to know that the basic_json added fields are still intact.

Well the issue that @nlohmann found here is that in the most recent clang-tidy version (somewhere between 16.0.2 and a7bf92a7cb636af95e16349f9f2a8b8c9ec977ce) they have changed the analysis so that it raises the warning even when such down-conversion is done. Or maybe this is not intended and it is only a bug? We should probably get a clarification from someone on the clang-tidy team about this one.

[gregmarr]

I was referring to my (perhaps incorrect) understanding that this code fixed the issue, and thus it was determining that the forwarded parameter was only partially-moved, so we could safely access other.m_data afterwards. Is that not the case?

basic_json(basic_json&& other) noexcept
    : json_base_class_t(std::forward<json_base_class_t>(other)),
      m_data(std::move(other.m_data))
{
    // check that passed value is valid
    other.assert_invariant(false);
}

[LecrisUT]

That indeed is my understanding of this as well. In principle the std::move would be doing the same thing after down-conversion, but clang-tidy seems to either decided to discourage that practice completely, or they have a bug in there that does not account for that. I do confirm that using std::forward<>() does indeed overcome the lint issue both for other.m_data, as well as for other.assert_invariant. It might be only a style choice if it should be written in one way or another, or maybe there is some standard change down the line around that.

[gregmarr]

I think std::forward<>() is a better choice than std::move() here as it is passing it as the base type, and not just an rvalue of the same type (which is an upcast, not a downcast, btw).

[nlohmann]

Thanks! I hope 6e74433 brings us forward.

[nlohmann]

Alright, the Clang-Tidy warnings are gone. Now it's just https://github.com/nlohmann/json/actions/runs/5032287688/jobs/9025846237 missing...

/__w/json/json/tests/src/unit-constructor1.cpp:282:34: error: invalid operands to binary expression ('Expression_lhs<const basic_string<char, char_traits<char>, allocator<char>> &>' and 'const value_type' (aka 'const nlohmann::basic_json<>'))
            CHECK(std::get<2>(t) == j[2]);
            ~~~~~~~~~~~~~~~~~~~~~^~~~~~~~

[nlohmann]

It's frustrating, but I am thinking about commenting this test to finally get a green CI back. What do you think?

[LecrisUT]

Well looking at the failure state, I find it weird that it seems to have failed a genuine test. But the gcc tests did not fail. But the test environment is obscure because each one has different targets.

I think the whole CI should be simplified and modernized using cmake presets instead and minimizing targets.

But because this is a blocking CI I think it can be marked as experimental (reference)

[nlohmann]

Well looking at the failure state, I find it weird that it seems to have failed a genuine test. But the gcc tests did not fail. But the test environment is obscure because each one has different targets.

What do you mean?

I think the whole CI should be simplified and modernized using cmake presets instead and minimizing targets.

I am not too happy with the CI either, but there are just too many combinations of operating systems, compilers, and checks... The current approach tries to rely less on the CI provider (we had to move from Travis CI recently), but to put as much logics as possible to files in the repo. I wonder if CMake presets help here.

But because this is a blocking CI I think it can be marked as experimental (reference)

I think I would rather mark the single test in Doctest as failable.

[LecrisUT]

I think I would rather mark the single test in Doctest as failable.

I don't know, it feels like a slippery slope. At least this case only 1 test is the issue. But still it should be documented in an issue to discuss what's going on over there. I'll try to check with my ide to confirm what the issue is next week if you can open a separate issue about this.

but there are just too many combinations of operating systems

Imo this should always be manually managed in the CI system

compilers, and checks

This however we can manage in the presets. But still it should be as lightweight and vanilla as possible. E.g. I design to have only separate compilers configuration (and in this case also c++ standards) as presets, but I would not create separate targets for theses.

Well looking at the failure state, I find it weird that it seems to have failed a genuine test. But the gcc tests did not fail. But the test environment is obscure because each one has different targets.

What do you mean?

I mean a test failed not a clang-tidy. About it being obscure, I mean that it is configured to build and test separate targets for gcc and clang. So we need to confirm if it is a clang version issue, cmake configuration issue, source issue etc. If they were alll the same target we would rule out a cmake configuration issue in most cases.

[nlohmann]

I was wrong about Doctest as the issue is that the test does not compile in the first place... Anyway, I added a comment about this and will merge this now to finally have a green CI again. Happy to follow up.