Add assertion if nullptr is passed to parse function

[nlohmann]

Addresses #3584 by adding an assertion and documenting the preconditions.

[coveralls]

Coverage remained the same at 100.0% when pulling 043a0d7 on file_exception into feef0eb on develop.

[Nhattan9961]

A

[nlohmann]

It seems I cannot suppress the issue in the test suite, but I need to add the attribute to the input adapter's constructor. Which makes no sense, because we added the attribute exactly to help the compiler/sanitizer find such an issue.

I am not sure how to continue:

• Removing the test yields untested code that throws an exception.
• That exception only serves the purpose of enforcing that passed files are usable - and this only for the C file API.

I currently tend more to adding an assertion and making the documentation more clear that passing a null pointer yields undefined behavior.

Alternatively, we could make the behavior more similar to the std::ifstream case and (1) check in the constructor of the passed pointer is null and (2) in get_character function return eof() in that case. This would add one if statement to a const bool member which, when annotating with UNLIKELY should not be too expensive for the happy path.

Both alternatives also have the advantage that the constructor remains noexcept - this PR silently removes the specifier which is a breaking change...

What do you think?

[falbrechtskirchinger]

I agree that keeping exceptions out of constructors is a good idea. I'd still prefer a way to differentiate parse errors from invalid input adapters. This goes beyond the scope of this PR but is what I've been asking for from the start.

To that end, an alternative might be to add operator bool() to input_adapter, check that somewhere in or near the parse/sax_parse/accept functions, and throw an exception from there.

For file_input_adapter the operator would simply return m_file != nullptr, for input_stream_adapter (which ought to be renamed to stream_input_adapter before being added to the public API) is->good(), for iterator_input_adapter current != end, etc.

[nlohmann]

I agree that keeping exceptions out of constructors is a good idea. I'd still prefer a way to differentiate parse errors from invalid input adapters. This goes beyond the scope of this PR but is what I've been asking for from the start.

Yes, this is also a pain point for me, because there are issues again and again where people seem to not understand that "unexpected end of file" at position 0 means that a file could not be opened in the first place.

To that end, an alternative might be to add operator bool() to input_adapter, check that somewhere in or near the parse/sax_parse/accept functions, and throw an exception from there.

For file_input_adapter the operator would simply return m_file != nullptr, for input_stream_adapter (which ought to be renamed to stream_input_adapter before being added to the public API) is->good(), for iterator_input_adapter current != end, etc.

Sounds good.

But for this PR, should we go for the behavior that a null pointer yields EOF?

Edit:

We can also not test that behavior, because again we cannot pass a null pointer to the input adapter without being punished by UBSAN... This is how CLion tells me that the parameter cannot be null:

I get the feeling that an assertion and updated documentation is the best we can do without investing too much for this edge case. I like the idea of adding some kind of ok() function - but not for this issue.

[gregmarr]

I guess we assert in the constructor for easy diagnosis in the cases where assert is enabled, and add the nullptr check in get_character() with the unlikely attribute.

[falbrechtskirchinger]

And we can remove the nullptr check from get_character() once my idea (or equivalent) is implemented.