Fixes #804 Json parsing errors mention JSON.parse in errors #1001

nojvek · 2016-05-18T02:31:52Z

I added JSON.parse specific errors. It still doesn't feel very clean as there is no column info. Would this be relatively easy to add?

msftclas · 2016-05-18T02:31:56Z

Hi @nojvek, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!

It looks like you're a Microsoft contributor (Manoj Patel). If you're full-time, we DON'T require a Contribution License Agreement. If you are a vendor, please DO sign the electronic Contribution License Agreement. It will take 2 minutes and there's no faxing! https://cla.microsoft.com.

TTYL, MSBOT;

ianwjhalliday · 2016-05-18T21:02:37Z

Thanks @nojvek, these new error messages are definitely more helpful.

The error stack will have the line column information for the source of the throw, which will be where the JSON.parse call occurred, so we do have that already.

Could also be useful to have character offset (or line/column?) information for the source of the error in the JSON string passed to parse. I don't know if that would require much additional work on in the JSON parser or not. Certainly this can be raised as a new separate Suggestion issue if you think it is a good idea.

@abchatra any concerns over adding new error messages?

nojvek · 2016-05-19T01:06:21Z

Right now there seems to be a LSC_ERROR_MSG(..) macro definition of a
syntax error.

How do I throw a syntax error with a dynamically created string ?

On Wed, May 18, 2016 at 2:03 PM, Ian Halliday [email protected]
wrote:

Thanks @nojvek https://github.com/nojvek, these new error messages are
definitely more helpful.

The error stack will have the line column information for the source of
the throw, which will be where the JSON.parse call occurred, so we do have
that already.

Could also be useful to have character offset (or line/column?)
information for the source of the error in the JSON string passed to parse.
I don't know if that would require much additional work on in the JSON
parser or not. Certainly this can be raised as a new separate Suggestion
issue if you think it is a good idea.

@abchatra https://github.com/abchatra any concerns over adding new
error messages?

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#1001 (comment)

ianwjhalliday · 2016-05-19T19:19:43Z

Look at rterrors.h where we have errors with two forms of their message string. The first form has a %s parameter that will be replaced by the given string to the JavascriptError::Throw* methods.

I believe you should be able to move the errors to rterrors.h and still use them for ThrowSyntaxError without any issues.

abchatra · 2016-05-19T21:57:15Z

@abchatra any concerns over adding new error messages?

I am happy to see improvement in the error messages.

nojvek · 2016-05-21T01:36:13Z

Thanks abchatra for the help today. I have made the fixes to show the position just like v8 does for JSON.parse. wdyt?

ianwjhalliday · 2016-05-23T22:08:06Z

Looks great! Thanks @nojvek, I'll get this merged.

ianwjhalliday · 2016-05-23T23:19:00Z

@nojvek turns out we have some JSON tests that do not run in jenkins currently because they depend on timezone.

You will need to manually run these two tests, test/JSON/jx2.js and test/es5/jx2.js, and update their baselines to match your change before we can merge this in. These two tests happen to run with no ch.exe command line switches, but in general when running tests manually you'd want to confirm you are using the right switches by examining the test/.../rlexe.xml file to see if it specifies switches and if so what they are. Also this will show you what the baseline file is named, sometimes it doesn't match the source file.

I also just noticed that your new test syntaxerror.js is not added to test/JSON/rlexe.xml. Please add it there so that it runs in the test runner.

ianwjhalliday · 2016-05-23T23:21:03Z

Note the json tests that don't run in jenkins because of their timezone dependency is an understood issue that we have not fixed yet. It depends on PST timezone.

abchatra · 2016-05-23T23:39:19Z

lib/Runtime/Library/JSONParser.cpp

                return object;
            }

        default:
-            Js::JavascriptError::ThrowSyntaxError(scriptContext, ERRsyntax);
+            m_scanner.ThrowSyntaxError(JSERR_JsonSyntax);
+            return NULL;


Not required. This statement won't be reached.

Without this C++ won't compile.

e:\personal\chakracore\lib\runtime\library\jsonparser.cpp(415): warning C4715: 'JSON::JSONParser::ParseObject': not all control paths return a value

That is because JSONScanner::ThrowSyntaxError is not marked with __declspec(noreturn).

Probably reasonable to change the definition of JSONScanner::ThrowSyntaxError to:

void __declspec(noreturn) ThrowSyntaxError(int wErr)

nojvek · 2016-05-31T23:09:11Z

Ensured tests pass in runtests.cmd -x64 -debug and added syntaxError.js to rl.xml

ianwjhalliday · 2016-06-01T22:34:28Z

@nojvek you forgot to update the baseline for test\es5\jx2.js as well.

nojvek · 2016-06-01T22:43:06Z

Oh forgot that. I updated Json/jx2.js

On Wednesday, June 1, 2016, Ian Halliday [email protected] wrote:

@nojvek https://github.com/nojvek you forgot to update the baseline for
test\es5\jx2.js as well.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#1001 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AA-JVFQNPXjf5SZyJzGzCqVO7-G9Su0vks5qHgkFgaJpZM4Ig4Jc
.

ianwjhalliday · 2016-06-02T22:45:51Z

@nojvek when you get a chance please update test\es5\jx3.baseline. I've updated internal test baselines so once you've done that I believe we'll be all ready to merge your change.

nojvek · 2016-06-03T00:37:19Z

Done. Ran all 1655 tests and confirmed they passed.

Not sure why baseline for jx2.js was called jx3.js. I've renamed to
jx2.baseline.

Soz for delay

On Thursday, June 2, 2016, Ian Halliday [email protected] wrote:

@nojvek https://github.com/nojvek when you get a chance please update
test\es5\jx3.baseline. I've updated internal test baselines so once
you've done that I believe we'll be all ready to merge your change.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#1001 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AA-JVKI0UcnShH3KDFscE4dJsXbvpJkKks5qH10xgaJpZM4Ig4Jc
.

ianwjhalliday · 2016-06-03T00:41:00Z

Awesome, thanks for the rename. I don't know why it was different either. Probably some historical reason that didn't get cleaned up nicely.

nojvek · 2016-06-03T18:08:40Z

Would be great to see this go in today. Let me know if you need anything else from my side. Thanks

…rors Merge pull request #1001 from nojvek:master I added JSON.parse specific errors. It still doesn't feel very clean as there is no column info. Would this be relatively easy to add?

ianwjhalliday · 2016-06-06T21:18:27Z

@nojvek we're all good, merged! Thanks for improving these error messages

nojvek · 2016-06-06T23:03:10Z

That's awesome. Thanks Ian.

On Mon, Jun 6, 2016 at 2:18 PM, Ian Halliday [email protected]
wrote:

@nojvek https://github.com/nojvek we're all good, merged! Thanks for
improving these error messages

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#1001 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AA-JVDjgc-Yok9s9XJygkux6ofi-Wcupks5qJI60gaJpZM4Ig4Jc
.

abchatra · 2016-06-06T23:04:08Z

Thank you @nojvek

dilijev · 2016-06-06T23:15:08Z

Note that we're tracking the timezone-sensitive tests issue in #319

msftclas added the cla-already-signed label May 18, 2016

abchatra reviewed May 23, 2016
View reviewed changes

Fixes #804 Json parsing errors mention JSON.parse in errors

2f634ae

update jx2.baseline

69f175d

chakrabot merged commit 69f175d into chakra-core:master Jun 6, 2016

Woodpile37 mentioned this pull request Aug 21, 2023

[Snyk] Upgrade fs-extra from 9.1.0 to 11.1.1 Woodpile37/ChakraCore#2

Merged

Woodpile37 mentioned this pull request Aug 22, 2023

[Snyk] Upgrade fs-extra from 9.1.0 to 11.1.1 Woodpile37/ChakraCore#7

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #804 Json parsing errors mention JSON.parse in errors #1001

Fixes #804 Json parsing errors mention JSON.parse in errors #1001

nojvek commented May 18, 2016 •

edited

Loading

msftclas commented May 18, 2016

ianwjhalliday commented May 18, 2016

nojvek commented May 19, 2016

ianwjhalliday commented May 19, 2016

abchatra commented May 19, 2016

nojvek commented May 21, 2016

ianwjhalliday commented May 23, 2016

ianwjhalliday commented May 23, 2016

ianwjhalliday commented May 23, 2016

abchatra May 23, 2016

nojvek May 31, 2016

ianwjhalliday May 31, 2016

nojvek May 31, 2016

nojvek commented May 31, 2016

ianwjhalliday commented Jun 1, 2016

nojvek commented Jun 1, 2016

ianwjhalliday commented Jun 2, 2016

nojvek commented Jun 3, 2016

ianwjhalliday commented Jun 3, 2016

nojvek commented Jun 3, 2016

ianwjhalliday commented Jun 6, 2016

nojvek commented Jun 6, 2016

abchatra commented Jun 6, 2016

dilijev commented Jun 6, 2016

Fixes #804 Json parsing errors mention JSON.parse in errors #1001

Fixes #804 Json parsing errors mention JSON.parse in errors #1001

Conversation

nojvek commented May 18, 2016 • edited Loading

msftclas commented May 18, 2016

ianwjhalliday commented May 18, 2016

nojvek commented May 19, 2016

ianwjhalliday commented May 19, 2016

abchatra commented May 19, 2016

nojvek commented May 21, 2016

ianwjhalliday commented May 23, 2016

ianwjhalliday commented May 23, 2016

ianwjhalliday commented May 23, 2016

abchatra May 23, 2016

Choose a reason for hiding this comment

nojvek May 31, 2016

Choose a reason for hiding this comment

ianwjhalliday May 31, 2016

Choose a reason for hiding this comment

nojvek May 31, 2016

Choose a reason for hiding this comment

nojvek commented May 31, 2016

ianwjhalliday commented Jun 1, 2016

nojvek commented Jun 1, 2016

ianwjhalliday commented Jun 2, 2016

nojvek commented Jun 3, 2016

ianwjhalliday commented Jun 3, 2016

nojvek commented Jun 3, 2016

ianwjhalliday commented Jun 6, 2016

nojvek commented Jun 6, 2016

abchatra commented Jun 6, 2016

dilijev commented Jun 6, 2016

nojvek commented May 18, 2016 •

edited

Loading