Boris Zbarsky [:bzbarsky] Reporter
	Description • 14 years ago

JSON.parse("1.") should throw instead of returning 1.  This came up recently on es-discuss.

	Nicholas Nethercote [inactive] Assignee
	Comment 1 • 14 years ago

JSON.parse("1.e10") should also throw, but doesn't.

The attached patch fixes the "1." case but not the "1.e10" case.

The problem is that json.cpp just sucks up any sequence of number-like chars [0-9.eE+-] and then outsources the parsing of that sequence to js_strtod().  But js_strtod() doesn't require digits after a '.'.  (There may be other things it accepts that aren't valid JSON, but I couldn't find any.)

I think this'll have to be fixed by properly parsing the number within json.cpp.  The only problem is that bug 589664 thoroughly overhauls the JSON parser to be faster and nicer by making it a proper recursive descent parser instead of this odd state-based thing.  Is bug 589664 going to land soon?  If so, this bug should depend on it.  Otherwise, the state-based parser should be fixed and then bug 589664 will need to be fixed separately.

BTW, the only JSON tests I see are in tests/ecma_5/JSON/.  They are not very thorough.

	Jeff Walden [:Waldo]
	Comment 2 • 14 years ago

There's also:

http://mxr.mozilla.org/mozilla-central/source/dom/src/json/test/test_json.html?force=1
http://mxr.mozilla.org/mozilla-central/source/dom/src/json/test/unit/

Having hacked on JSON code slightly in the past, I would be forever in your debt if you moved these into ecma_5/JSON/ so that they don't induce "ohai, ur shell tests are inadequate" for no good reason.  ;-)

	David Mandelin [:dmandelin]
	Comment 3 • 14 years ago

This looks stalled. Do we need a new assignee? Or should we just unblock? It doesn't seem critical, although ES spec support seems like a good goal.

	Nicholas Nethercote [inactive] Assignee
	Comment 4 • 14 years ago

I'm happy to take this on, I just need to know if 598664 will land soon.  At this point I'm guessing not... I'll assume that's the case if Sayre doesn't say otherwise soon.

	Brendan Eich [:brendan]
	Comment 5 • 14 years ago

(In reply to comment #4)
> I'm happy to take this on, I just need to know if 598664

Should be bug 589664.

> will land soon.  At
> this point I'm guessing not... I'll assume that's the case if Sayre doesn't say
> otherwise soon.

Can this bug be spot-fixed? Slow but correct beats fast but incorrect. Fast and correct is best of course ;-) but we are short on time for Firefox 4.

/be

	Nicholas Nethercote [inactive] Assignee
	Comment 6 • 14 years ago

It's a non-trivial spot-fix, it requires adding extra parse states to parse numbers explicitly, rather than just using js_dtostr().

Accepting invalid inputs is better than rejecting valid inputs, which is probably why it was downgraded from "blocking2.0" to "wanted2.0".

	Jeff Walden [:Waldo]
	Comment 7 • 14 years ago

You could still use js_dtostr -- you'd just have to accumulate characters into a buffer through a few more states to do it.  It's no spot-fix, to be sure, but it does seem like something doable with very little effort and complication.

	Nicholas Nethercote [inactive] Assignee
	Comment 8 • 14 years ago

Yes, it wouldn't be too hard.  I was going to work on it when it was a blocker, which is why I assigned it to myself.  But then it was downgraded to "wanted2.0".  And I haven't touched it since because I'm still smarting from 
bug 591698 comment 9, even though I see plenty of other people not following a "all blockers, all the time" strategy.

	Nicholas Nethercote [inactive] Assignee
	Comment 9 • 13 years ago

Waldo, has your recent work on the JSON parser fixed this?

	Nicholas Nethercote [inactive] Assignee
	Comment 10 • 13 years ago

To answer my own question, yes:

  js> JSON.parse("1.")
  typein:1: SyntaxError: JSON.parse: missing digits after decimal point

And js/src/tests/ecma_5/JSON/parse-number-syntax.js tests exactly that case.  Case closed!  Thanks, Waldo.