nlohmann / json

JSON for Modern C++
https://json.nlohmann.me
MIT License
42.01k stars 6.63k forks source link

The parser doesn't allow parse_error() to return true and continue parsing. #3989

Open francislan opened 1 year ago

francislan commented 1 year ago

Description

The documentation has contradictory information:

Here, it says that parse_error() must return false but here, it says that "the return value indicates whether the parsing should continue, so the function should usually return false."

However it seems that the code expects parse_error() to return false and doesn't continue parsing if it returns true. See parser::sax_parse_internal() where it propagates the return value of parse_error() instead of only propagating false values.

Example:

//  case token_type::value_float:

if (JSON_HEDLEY_UNLIKELY(!std::isfinite(res)))
{
    return sax->parse_error(...);
}

Should be

//  case token_type::value_float:

if (JSON_HEDLEY_UNLIKELY(!std::isfinite(res)))
{
    if (!sax->parse_error(...))
    {
        return false;
    }
    break;
}

This would allow a SAX handler to deal with non-finite numbers, for example by stringifying them instead of failing parsing altogether. Of course a cleaner way would be for the parser to handle that natively by introducing a new method virtual bool number_nonfinite(const string_t& s) = 0;

Reproduction steps

Have a SAX handler return true for some parse_error()

Expected vs. actual results

Expected: the parser to continue parsing the rest of the input. Actual: the parser stops.

Minimal code example

No response

Error messages

No response

Compiler and operating system

N/A

Library version

developer branch

Validation

nlohmann commented 1 year ago

The documentation is not clear enough: the function must return false. Allowing the parser to proceed after an error needs more work.

francislan commented 1 year ago

Thanks for clarifying. Is there a bug that tracks "allow the parser to proceed after an error" or "allow parsing non-finite numbers"?

nlohmann commented 1 year ago

Not that I am aware of.