We should remove static_asserts

[theodelrieu]

Sorry for the click-bait title, but I'd like to have some feedbacks about this :)

I'll try to be very clear, since it's related to the heart of the from/to_json conversion system.

DISCLAIMER: This post is long. Really.

Reminder of the initial problem

Since version 2.1.0 and the support of user-defined types,
we have a non-explicit templated constructor for basic_json.

This can be very dangerous:

struct Innocent {};
struct Evil {
  // Don't try this at home
  template <typename T>
  Evil(T) {}
};

bool operator==(Evil const&, Evil const&) { return true; }

int main() {
  Innocent a, b;

  // Should never compile, right?
  // But it does, thanks to the Evil non-explicit templated constructor
  a == b;
}

I'll call it Implicit Conversion Mania (ICM).

We could not make the constructor explicit, it would have ruined the great syntax that one can use to construct json objects from other types.

So we had to add constraints to that constructor.

The fix(?)

Ideally the constraint should be:

• void basic_json::JSONSerializer::to_json(basic_json&, T&&) is valid

Note: JSONSerializer is the latest of basic_json's template arguments, and defaults to adl_serializer.

The workaround(?)

But there were more to add to prevent clashes with other constructors (e.g. the old basic_json(std::istream&)).
So we added those, and everything was looking good... Until compiling unit-iterators1.cpp.

There was a test on primitive_iterator_t which triggered the ICM.
After a rude fight with the error message, I found that Catch was doing some SFINAE checks to know if some type was printable.

I'll explain why that triggered the ICM on a later section.
At that point, I just wanted to make stuff work.

The hack

After a bit of digging, I discovered that the only classes that were triggering the ICM were basic_json nested classes... (I couldn't find exactly why, I have to open a SO question about that).

So the ugly fix (which is still in the code today) was to add another constraint on the template constructor,
checking T against every basic_json's nested class.

It's ugly, but it works well. So no problem right? Why should anyone care?

The return of the ICM

A few days ago, one of my colleagues had a huge compiler error while writing some tests with trompeloeil.

That library was doing the same SFINAE checks that Catch is doing, which caused the compiler error that I've described ahead.

So I sighed.

And this time, the type that was in cause was: boost::function<void (nlohmann::json const&)>.

WAT

What the hell? How could that be?

My theory is that ICM happens when a type T has a template somewhere, and when basic_json is a part of it:

// nested type
nlohmann::basic_json<...>::primitive_iterator_t;

// basic_json is part of the template arguments
// (Signature form: <Return (Args...)>)
boost::function<void (nlohmann::json const&)>;

I really need to open that SO question, a language lawyer is required in this case.

The huge hack

But heh, there's a workaround!

By specializing the adl_serializer<boost::function<void (nlohmann::json const&)>>
and by not defining any functions in it, it works!

That's very brittle to say the least...

Why does it happen?

That's the trickiest part.

TL;DR

SFINAE problems, you cannot have static_asserts and 100% correct constraints.

Long version

At the very bottom of the User-Defined Type (UDT) conversion mechanism is the detail::to_json_fn::call functions.

The first overload is enabled if there is a to_json function that can be called.
The second is called if not, and there are static_asserts in there.

But there are no constraints whatsoever (no enable_if) on the second overload.
Which means that during overload resolution, the compiler will ALWAYS choose the latter, which can NEVER compile, since the static_assert will always fail.
That's great when you're calling to_json on a type that has no conversion function of course. That's what you want!

But when libraries are doing SFINAE checks to determine if they can know if your type has a specific requirement or not
(e.g. is your type printable?), that's a huge problem.
Especially because you want to know if a piece of code is valid or not (i.e. does compile or not). You do not want to trigger the compiler error at all!

So why are there no constraints then?

Well...

You simply cannot have static_assert and enable_if together.

If you want the former, it means the function has to be called (not just tested with non-evaluated things like traits, SFINAE checks if you will).
If you want the latter, the function will not be called, the code will be 100% correct but you will not have static_assert.

Note that compilation will fail in the exact same cases as before, but with undecipherable errors.

And if you really want both, it's easy: Wait C++20's Concepts and use them inside your fork!

You get the idea...

What can we do?

Two possibilities:

1. Leave everything as-is.
2. Add proper constraints and remove static_asserts in the UDT conversion mechanism.

I know that 1 is better for debugging, it can be hard to know what is happening when the compiler yells at you.
(It yells at you with static_assert too, but you can understand it in a few seconds).

2 is my preferred option (as you might expect from the issue title).

Option 1 can lead to incorrect and incomprehensible things, as I showed with the boost::function example.
Having a "correct" workaround requires a very good understanding of template meta-programming and of the library.

And in the previous examples, there was a compiler error!
It could be a runtime or silent error at worst (if the incorrect SFINAE results in a different branch at runtime).

I understand that those cases does not happen to most users. But when you'll become one of those users, you'll cry :).

Apart from the loss of static_assert, the change will not break anything.

The hope

There is a way to help users though.

We can add a separate function (e.g. nlohmann::find_what_happens()), that will do the same thing, trigger a static_assert.

I need to hack in the code to see how far we could go, meanwhile I'll stop writing this monologue!

EDIT: Small mistake, was showing from_json instead fo to_json in the beginning.

[theodelrieu]

Thanks for the feedback stale, don't close please.

EDIT: accidentally marked someone which has the username stale, sorry!

[msarehn]

I've just been bitten by this. I wanted to do some expression-SFINAE trickery to see if a function call was valid and the static_assert wouldn't let me do this. I've spent nearly two days on this before I finally gave up.

An alternative (at least for my use case) would be to keep the current behaviour for easy debugging. But on top of that, something like nlohmann::can_cast_to_json<T> would be provided which can be used in SFINAE stuff.

[theodelrieu]

Sorry to hear that :/

That could work, but only when the library author knows they work with nlohmann::json (I guess that's your use case).

If not, std::is_constructible might be used and then all bets are off.

[theodelrieu]

Can we disable stale bot on this one? I'd like to have more people seeing it and giving their opinion (which would be helped by it being open)

[theodelrieu]

It might be a good idea to add a .github/stale.yml to exempt issues with please discuss label from being closed by the stale bot: https://github.com/apps/stale

[gregmarr]

There is already a stale.yml as it's needed to enable the bot, and the 'pinned' label will currently make stalebot ignore an issue.

[theodelrieu]

Right, should've looked... I can wait for Niels to come back from his vacation :)

[nlohmann]

I guess this issue is fixed with merging #1228, right?

[theodelrieu]

Indeed, I'm happy to close this one 😄