Skip to content

Convert JSON parsing failures from assertions to exceptions#7531

Merged
dschuff merged 7 commits intomainfrom
parseexception
Apr 24, 2025
Merged

Convert JSON parsing failures from assertions to exceptions#7531
dschuff merged 7 commits intomainfrom
parseexception

Conversation

@dschuff
Copy link
Copy Markdown
Member

@dschuff dschuff commented Apr 18, 2025

This allows some useful error reporting with bad source maps.
The JSON exception is converted to the existing map parse exception by the
source map parser, allowing a unified interface. Also add several tests of
bogus source maps.

This allows some useful error reporting with bad source maps.
The JSON exception is converted to the existing map parse exception by the
source map parser, allowing a unified interface. Also add several tests of
bogus source maps.
@dschuff dschuff requested a review from kripken April 18, 2025 23:12
Copy link
Copy Markdown
Member

@kripken kripken left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm % question

Comment thread src/wasm/source-map.cpp

next = peek();
if (next == ';') {
throw MapParseException("Unexpected mapping for 2nd generated line");
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does "2nd" mean here?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A generated JS file can have multiple lines. Mappings for each line are separated by a ; character. But wasm files are treated as if they have only one line, and the column number is treated as the binary offset. I'll add a comment.

Comment thread test/gtest/CMakeLists.txt
if(BUILD_FUZZTEST)
set(unittest_SOURCES ${unittest_SOURCES} type-domains.cpp)
else()
set(unittest_SOURCES ${unittest_SOURCES} source-map.cpp)
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tlively I've had to exclude this file from fuzztest builds, because it uses the gmock-matchers.h header (gmock is included with "standard" gtest but not with the one bundled with fuzztest). I tried including the gmock header along with the fuzztest gtest, but it seems to depend on some kind of internal gtest bit that the fuzztest gtest doesn't have. Do you know if fuzztest's bundled gtest is some sort of subset, or if they have any recommendations for use with gtest features they don't support themselves? Or maybe this is just WAI, and the intention is that fuzztests are logically separate enough from regular unit tests that it just makes sense to build them separately.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I don't know the details of why fuzztest's bundled gtest is different from normal gtest, nor have I seen any recommendations for this. It's definitely not WAI, though. fuzztests are supposed to coexist peacefully with normal gtests AFAICT.

It's probably fine excluding the file to work around the problem for now. The fuzztest build isn't important enough (yet) to spend time on. It would be good to add a comment explaining the problem, though.

@dschuff dschuff merged commit 5f04581 into main Apr 24, 2025
14 checks passed
@dschuff dschuff deleted the parseexception branch April 24, 2025 16:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants