Thanks for fixing the null-parser crash in WeaselJsonParser_parse - the change is correct, the new test passes, and I verified the full suite (24/24) is green with this branch.
The main thing I'd like to flag is a design/API choice rather than a correctness bug: this introduces a brand-new member (WeaselJson_NULL) to the public C ABI enum WeaselJsonStatus. Issue #51 offered two options - return WeaselJson_REJECT, or behave as a safe no-op - and reusing WeaselJson_REJECT would have been non-breaking for downstream consumers. Because the library itself compiles with -Werror=switch-enum -Wswitch-enum, adding a value forces every switch over WeaselJsonStatus to handle it (which is why validate.cpp needed a new case); external consumers built with -Wswitch/-Werror will see the same breakage on upgrade. A null parser can never accept input, so WeaselJson_REJECT conveys the right semantic without an ABI bump. If a distinct code is intentional (e.g. for diagnostics), that's a reasonable call, but it's worth being deliberate about it given the ABI implications.
A few smaller follow-ups regardless of that decision:
The doc comment for WeaselJsonParser_parse doesn't mention the null-parser behavior; please add a line so the header is self-consistent with the new code path.
weaseljson.py's WeaselJsonStatus enum is now missing NULL=4. It's never returned through the Python bindings (parse() guards via _check_open), but keeping the two enums in sync avoids future surprise.
validate.cpp prints "Could not create parser" for the WeaselJson_NULL case; that message conflates 'parse was handed a null pointer' with 'create failed'. A message like "parse called with a null parser" would be clearer.
The new test passes len=-1 together with the null parser. It works (the null check short-circuits first) but it conflates two edge cases; using a valid length and/or mirroring the issue's reproduction (parse(nullptr, nullptr, 0)) would be clearer.
Thanks for fixing the null-parser crash in WeaselJsonParser_parse - the change is correct, the new test passes, and I verified the full suite (24/24) is green with this branch.
The main thing I'd like to flag is a design/API choice rather than a correctness bug: this introduces a brand-new member (WeaselJson_NULL) to the public C ABI enum WeaselJsonStatus. Issue #51 offered two options - return WeaselJson_REJECT, or behave as a safe no-op - and reusing WeaselJson_REJECT would have been non-breaking for downstream consumers. Because the library itself compiles with -Werror=switch-enum -Wswitch-enum, adding a value forces every switch over WeaselJsonStatus to handle it (which is why validate.cpp needed a new case); external consumers built with -Wswitch/-Werror will see the same breakage on upgrade. A null parser can never accept input, so WeaselJson_REJECT conveys the right semantic without an ABI bump. If a distinct code is intentional (e.g. for diagnostics), that's a reasonable call, but it's worth being deliberate about it given the ABI implications.
A few smaller follow-ups regardless of that decision:
- The doc comment for WeaselJsonParser_parse doesn't mention the null-parser behavior; please add a line so the header is self-consistent with the new code path.
- weaseljson.py's WeaselJsonStatus enum is now missing NULL=4. It's never returned through the Python bindings (parse() guards via _check_open), but keeping the two enums in sync avoids future surprise.
- validate.cpp prints "Could not create parser" for the WeaselJson_NULL case; that message conflates 'parse was handed a null pointer' with 'create failed'. A message like "parse called with a null parser" would be clearer.
- The new test passes len=-1 together with the null parser. It works (the null check short-circuits first) but it conflates two edge cases; using a valid length and/or mirroring the issue's reproduction (parse(nullptr, nullptr, 0)) would be clearer.
Adding a new member to this public C ABI enum is the most impactful part of this PR. Issue #51 suggested returning WeaselJson_REJECT (a null parser can never accept input), which would fix the crash without changing the ABI. Since the project compiles with -Werror=switch-enum -Wswitch-enum, any switch over WeaselJsonStatus now has to handle this new case (hence the validate.cpp change), and external consumers using -Wswitch/-Werror will break on upgrade. If a distinct status is desired for diagnostics that's fine, but consider reusing WeaselJson_REJECT to avoid an ABI bump - or at least call out in the header that this is a newly-added status callers must handle.
Adding a new member to this public C ABI enum is the most impactful part of this PR. Issue #51 suggested returning WeaselJson_REJECT (a null parser can never accept input), which would fix the crash without changing the ABI. Since the project compiles with -Werror=switch-enum -Wswitch-enum, any switch over WeaselJsonStatus now has to handle this new case (hence the validate.cpp change), and external consumers using -Wswitch/-Werror will break on upgrade. If a distinct status is desired for diagnostics that's fine, but consider reusing WeaselJson_REJECT to avoid an ABI bump - or at least call out in the header that this is a newly-added status callers must handle.
The doc comment covers the negative-length behavior but not the null-parser behavior added in this PR. Please add a sentence, e.g. "If parser is null, parse returns WeaselJson_NULL (or WeaselJson_REJECT) without dereferencing it.", so the header stays self-consistent with the implementation.
The doc comment covers the negative-length behavior but not the null-parser behavior added in this PR. Please add a sentence, e.g. "If parser is null, parse returns WeaselJson_NULL (or WeaselJson_REJECT) without dereferencing it.", so the header stays self-consistent with the implementation.
This couples the null-parser check with the negative-length case. Because the null check runs first it returns WeaselJson_NULL, but it would be clearer to use a valid length (e.g. WeaselJsonParser_parse(nullptr, buf, 5)) so the test isolates the null-parser behavior. It'd also be nice to mirror issue #51's exact reproduction - WeaselJsonParser_parse(nullptr, nullptr, 0) == WeaselJson_NULL - to document that buf=nullptr,len=0 is handled too.
This couples the null-parser check with the negative-length case. Because the null check runs first it returns WeaselJson_NULL, but it would be clearer to use a valid length (e.g. WeaselJsonParser_parse(nullptr, buf, 5)) so the test isolates the null-parser behavior. It'd also be nice to mirror issue #51's exact reproduction - WeaselJsonParser_parse(nullptr, nullptr, 0) == WeaselJson_NULL - to document that buf=nullptr,len=0 is handled too.
This message ("Could not create parser") describes a different condition than the status: it's reached when parse() is called with a null pointer, not when create() failed. Something like fprintf(stderr, "parse called with a null parser\n"); would be clearer. (In this program the two coincide because parser.get() is only null when create failed, but the wording is still misleading relative to the status name.)
This message ("Could not create parser") describes a different condition than the status: it's reached when parse() is called with a null pointer, not when create() failed. Something like fprintf(stderr, "parse called with a null parser\n"); would be clearer. (In this program the two coincide because parser.get() is only null when create failed, but the wording is still misleading relative to the status name.)
Thanks for the follow-up commit — this addresses most of the prior round's feedback: the header doc comment now mentions the null-parser behavior, the validate.cpp message is clearer ("parse called with a null parser"), and the new test now mirrors issue #51's exact reproduction (WeaselJsonParser_parse(nullptr, nullptr, 0) == WeaselJson_NULL), which cleanly isolates the null-parser case from the negative-length case. I verified the new test passes and that the build is clean under -Werror=switch-enum -Wswitch-enum.
On the ABI question: the deliberate choice to add WeaselJson_NULL rather than reuse WeaselJson_REJECT is reasonable — REJECT semantically means "invalid json," and a null parser is a different (caller-error) condition, so a distinct code is justified for diagnostics. The header documents it, and validate.cpp handles the new switch case, so internal consumers are covered. The one caveat worth keeping in mind is that this is an ABI/API bump for external consumers built with -Wswitch -Werror, who will need to add a case on upgrade; that's an accepted trade-off here.
One remaining follow-up from the prior review that wasn't picked up: the Python WeaselJsonStatus enum in weaseljson.py is still missing NULL = 4. It's never returned through the bindings today (parse() guards via _check_open), but keeping the two enums in sync avoids future surprise if the bindings ever expose the raw status. This is minor and non-blocking.
Note: two pre-existing test failures (parser3, streaming — both asserting parse(parser, nullptr, 0) == WeaselJson_OK but getting REJECT) are present on upstream/main as well, so they're not introduced by this PR.
Thanks for the follow-up commit — this addresses most of the prior round's feedback: the header doc comment now mentions the null-parser behavior, the validate.cpp message is clearer ("parse called with a null parser"), and the new test now mirrors issue #51's exact reproduction (WeaselJsonParser_parse(nullptr, nullptr, 0) == WeaselJson_NULL), which cleanly isolates the null-parser case from the negative-length case. I verified the new test passes and that the build is clean under -Werror=switch-enum -Wswitch-enum.
On the ABI question: the deliberate choice to add WeaselJson_NULL rather than reuse WeaselJson_REJECT is reasonable — REJECT semantically means "invalid json," and a null parser is a different (caller-error) condition, so a distinct code is justified for diagnostics. The header documents it, and validate.cpp handles the new switch case, so internal consumers are covered. The one caveat worth keeping in mind is that this is an ABI/API bump for external consumers built with -Wswitch -Werror, who will need to add a case on upgrade; that's an accepted trade-off here.
One remaining follow-up from the prior review that wasn't picked up: the Python WeaselJsonStatus enum in weaseljson.py is still missing NULL = 4. It's never returned through the bindings today (parse() guards via _check_open), but keeping the two enums in sync avoids future surprise if the bindings ever expose the raw status. This is minor and non-blocking.
Note: two pre-existing test failures (parser3, streaming — both asserting parse(parser, nullptr, 0) == WeaselJson_OK but getting REJECT) are present on upstream/main as well, so they're not introduced by this PR.
The C enum now has WeaselJson_NULL (value 4), but this Python enum stops at OVERFLOW = 3. It's never returned through the bindings today (parse() raises via _check_open before calling into C when the parser is null), but please add NULL = 4 here so the two stay in sync — otherwise a future change that lets the raw status through would surface an unmapped value.
The C enum now has WeaselJson_NULL (value 4), but this Python enum stops at OVERFLOW = 3. It's never returned through the bindings today (parse() raises via _check_open before calling into C when the parser is null), but please add `NULL = 4` here so the two stay in sync — otherwise a future change that lets the raw status through would surface an unmapped value.
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
weaselbot
Closes #51
Thanks for fixing the null-parser crash in WeaselJsonParser_parse - the change is correct, the new test passes, and I verified the full suite (24/24) is green with this branch.
The main thing I'd like to flag is a design/API choice rather than a correctness bug: this introduces a brand-new member (WeaselJson_NULL) to the public C ABI enum WeaselJsonStatus. Issue #51 offered two options - return WeaselJson_REJECT, or behave as a safe no-op - and reusing WeaselJson_REJECT would have been non-breaking for downstream consumers. Because the library itself compiles with -Werror=switch-enum -Wswitch-enum, adding a value forces every switch over WeaselJsonStatus to handle it (which is why validate.cpp needed a new case); external consumers built with -Wswitch/-Werror will see the same breakage on upgrade. A null parser can never accept input, so WeaselJson_REJECT conveys the right semantic without an ABI bump. If a distinct code is intentional (e.g. for diagnostics), that's a reasonable call, but it's worth being deliberate about it given the ABI implications.
A few smaller follow-ups regardless of that decision:
@@ -39,2 +39,4 @@/** json is too deeply nested */WeaselJson_OVERFLOW,/** Tried to call parse on a null parser */WeaselJson_NULL,Adding a new member to this public C ABI enum is the most impactful part of this PR. Issue #51 suggested returning WeaselJson_REJECT (a null parser can never accept input), which would fix the crash without changing the ABI. Since the project compiles with -Werror=switch-enum -Wswitch-enum, any switch over WeaselJsonStatus now has to handle this new case (hence the validate.cpp change), and external consumers using -Wswitch/-Werror will break on upgrade. If a distinct status is desired for diagnostics that's fine, but consider reusing WeaselJson_REJECT to avoid an ABI bump - or at least call out in the header that this is a newly-added status callers must handle.
WeaselJson_REJECT would mean that the json was invalid. We need something semantically different.
@@ -66,5 +66,5 @@* modified. Call with `len` 0 to indicate end of data. `buf` may be null if* `len` is 0. `len` must not be negative; a negative length is treated as a* rejected input. */WeaselJsonStatus WeaselJsonParser_parse(WeaselJsonParser *parser, char *buf,int len);The doc comment covers the negative-length behavior but not the null-parser behavior added in this PR. Please add a sentence, e.g. "If parser is null, parse returns WeaselJson_NULL (or WeaselJson_REJECT) without dereferencing it.", so the header stays self-consistent with the implementation.
@@ -332,1 +332,4 @@TEST_CASE("Calling parse with nullptr doesn't crash") {char buf[10] = "hello";REQUIRE(WeaselJsonParser_parse(nullptr, buf, -1) == WeaselJson_NULL);This couples the null-parser check with the negative-length case. Because the null check runs first it returns WeaselJson_NULL, but it would be clearer to use a valid length (e.g. WeaselJsonParser_parse(nullptr, buf, 5)) so the test isolates the null-parser behavior. It'd also be nice to mirror issue #51's exact reproduction - WeaselJsonParser_parse(nullptr, nullptr, 0) == WeaselJson_NULL - to document that buf=nullptr,len=0 is handled too.
@@ -34,2 +34,4 @@case WeaselJson_OVERFLOW:return 1;case WeaselJson_NULL:fprintf(stderr, "Could not create parser\n");This message ("Could not create parser") describes a different condition than the status: it's reached when parse() is called with a null pointer, not when create() failed. Something like fprintf(stderr, "parse called with a null parser\n"); would be clearer. (In this program the two coincide because parser.get() is only null when create failed, but the wording is still misleading relative to the status name.)
Thanks for the follow-up commit — this addresses most of the prior round's feedback: the header doc comment now mentions the null-parser behavior, the validate.cpp message is clearer ("parse called with a null parser"), and the new test now mirrors issue #51's exact reproduction (WeaselJsonParser_parse(nullptr, nullptr, 0) == WeaselJson_NULL), which cleanly isolates the null-parser case from the negative-length case. I verified the new test passes and that the build is clean under -Werror=switch-enum -Wswitch-enum.
On the ABI question: the deliberate choice to add WeaselJson_NULL rather than reuse WeaselJson_REJECT is reasonable — REJECT semantically means "invalid json," and a null parser is a different (caller-error) condition, so a distinct code is justified for diagnostics. The header documents it, and validate.cpp handles the new switch case, so internal consumers are covered. The one caveat worth keeping in mind is that this is an ABI/API bump for external consumers built with -Wswitch -Werror, who will need to add a case on upgrade; that's an accepted trade-off here.
One remaining follow-up from the prior review that wasn't picked up: the Python WeaselJsonStatus enum in weaseljson.py is still missing NULL = 4. It's never returned through the bindings today (parse() guards via _check_open), but keeping the two enums in sync avoids future surprise if the bindings ever expose the raw status. This is minor and non-blocking.
Note: two pre-existing test failures (parser3, streaming — both asserting parse(parser, nullptr, 0) == WeaselJson_OK but getting REJECT) are present on upstream/main as well, so they're not introduced by this PR.
@@ -28,5 +28,5 @@class WeaselJsonStatus(enum.Enum):OK = 0AGAIN = 1REJECT = 2The C enum now has WeaselJson_NULL (value 4), but this Python enum stops at OVERFLOW = 3. It's never returned through the bindings today (parse() raises via _check_open before calling into C when the parser is null), but please add
NULL = 4here so the two stay in sync — otherwise a future change that lets the raw status through would surface an unmapped value.