110286 - Multiline comments containing "/*" should not be syntax errors

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Steven Cole]

According to section 7.4 of the ECMA specification, the sequence of characters
/* /* */ should be parsed as a comment.  The current Spidermonkey implenetation
produces a "nested comment" syntax error.

I'm opening this bug because I lack the power to reopen bug 109917.

See also the old (September 2001) discussion in netscape.public.mozilla.jseng,
available at
http://groups.google.com/groups?hl=en&threadm=3BB91DD4.8080405%40atg.com&prev=/groups%3Fnum%3D25%26hl%3Den%26group%3Dnetscape.public.mozilla.jseng%26start%3D75%26group%3Dnetscape.public.mozilla.jseng

(Norris Boyd fixed Rhino, but nobody ever got around to fixing Spidermonkey.)

Comment 1

[Steven Cole]

Bugzilla is refusing to let me attach my patch! Grumble...

The fix is quite simple, just change the severity of the syntax error 
from "error" to "warning".

Here it is:
Index: src/jsscan.c
===================================================================
RCS file: /export/home/cvs/external/mozilla/js/src/jsscan.c,v
retrieving revision 1.8
diff -u -r1.8 jsscan.c
--- src/jsscan.c	2001/10/15 15:18:23	1.8
+++ src/jsscan.c	2001/11/15 17:15:03
@@ -1096,7 +1096,7 @@
 		if (c == '/' && MatchChar(ts, '*')) {
 		    if (MatchChar(ts, '/'))
 			goto retry;
-		    js_ReportCompileErrorNumber(cx, ts, NULL, JSREPORT_ERROR,
+                    js_ReportCompileErrorNumber(cx, ts, NULL, JSREPORT_WARNING,
                                                 JSMSG_NESTED_COMMENT);
 		}
 	    }

Comment 2

[Ashley Bischoff (blog at handcoding.com)]

No dupes found. Marking NEW.

Comment 3

[Guy Hutchison]

Attachment: Patch from [e-mail]

Pulled patch from [e-mail] out of message and into file.

Comment 4

[Steven Cole]

Attachment: Same patch, minus the inadvertant newline

Looks like I can make attachments again, huzzah...

Guy Hutchinson's attempt at pulling my patch from the comments didn't work
quite right (an extra newline got inserted, making the patch bogus).  This one
comes straight from the source.

The should obsolete the prior patch, if only I had such authority.

Comment 5

[Guy Hutchison]

Attachment: Testcase from original bug

Slightly rewrote testcase from original bug (alerts are annoying).  Confirmed
modified testcase passes on IE5 and NS4.7 and fails on Mozilla 0.9.5.

Comment 6

[Ashley Bischoff (blog at handcoding.com)]

Comment on attachment 57945 [details] [diff] [review]
Patch from [e-mail]

-> Obsolete, on behalf of Steven

Comment 7

[Phil Schwartau]

Testcase added to JS testsuite:

          mozilla/js/tests/js1_5/Regress/regress-110286.js

Passing in the Rhino standalone shell; failing in SpiderMonkey.

Thanks, Steven, for your work on this!
cc'ing reviewers for consideration of Steven's patch -

Comment 8

[Brendan Eich [:brendan]]

Steven, thanks again.  Kenton, would you r= attachment 57951 [details] [diff] [review], then I'll sr=?

/be

Comment 9

[Steven Cole]

Reviewers, please note:  while this patch fixes the problem, and lets scripts
run successfully, I'm not personally familiar enough with the error reporting
mechanism to know if anything ought to change in js.msg, where this "nested
comment" error is defined.

I also think that we ought to change the message from "nested comment" to
something like ""/*" found inside multi-line comment. (Possibly nested?)", but
I'm hesitant to do anything that requires new localization.  Do you actual
mozilla guys have any opinions about this?

Comment 10

[Brendan Eich [:brendan]]

The patch is ok as-is -- the js.msg parameters are suitable for a warning.  But
in light of the ECMA standard, should we just eliminate the warning altogether?

/be

Comment 11

[Steven Cole]

I'm not sure how *I* feel about the warning...  Looking through the Spidermonkey
code, it appears that warnings are really only emitted for acceptable violations
of the standard, not for legitimate code which might indicate programmer error.
 In light of this, the warning should go away.

I'll attach a patch that does this (smaller, more understandable code than the
embedded '/*' detector); but I'm still not sure which patch should be the final
winner.  (It also removes the JSMSG_NESTED_COMMENT identifier, and renumbers
some of the error codes; I'm not sure if this part is wise.)

Comment 12

[Steven Cole]

Attachment: Complete removal of Nested Comment error.

This patch removes the "nested comment" warning altogether, removes the
JSMSG_NESTED_COMMENT identifer from the source code completely, and renumbers
the errors above NESTED_COMMENT to avoid an error-number "hole".

I don't know if this is better than attachment 57951 [details] [diff] [review]; I'll defer to other
Spidermonkey brains about that...

Comment 13

[Phil Schwartau]

A question: removing this would mean no warning for /* /* */,
but we still WOULD get a warning for /* /* */ */, wouldn't we?
That is, a truly nested comment, forbidden by ECMA ...

Comment 14

[Steven Cole]

The input stream "/* /* */ */" results in a syntax error.  The "/* /* */" is
still a comment (it looks like whitespace), and then there's a bare "*/" sitting
there --- not valid JS.

In a nutshell, "/* /* */ */" is morally equivalent to "  */", which you'll never
get to compile.

(So no "nested comment" dectector is required.)

Comment 15

[Kenton Hanson (gone)]

r=khanson

Comment 16

[Brendan Eich [:brendan]]

Comment on attachment 58426 [details] [diff] [review]
Complete removal of Nested Comment error.

khanson, please use the patch manager (click on the Edit link to the right of
the attacment link) to record your r=.

I think we can do without this error or warning.  I also believe no one has
localized js.msg in Mozilla yet.  Others will have to track the renumbering,
which could be avoided by replacing the JSMSG_NESTED_COMMENT MSG_DEF macro call
with a JSMSG_UNUSED0 entry.  Stephen, what do you say?	Fix that to minimize
pain on non-Mozilla embedders who have localized js.msg, and
sr=[e-mail].

/be

Comment 17

[Steven Cole]

Attachment: Made exception #139 "unused" instead of just removing and renumbering

Changes made re: Brendan's comment #16.  [Attachemnt 57951 and attachment 58426 [details] [diff] [review]
should be marked obsolete.]

Kenton or Brendon, you'll have to check this in after review; I lack the
authority to do that.

-Steven

Comment 18

[Brendan Eich [:brendan]]

I made a few tweaks (brace "multiline" loop -- multiline includes a loop whose
condition takes more than one source line, even if its body is one-line and/or
empty; strip trailing whitespace) and checked in r=khanson, sr=brendan.  Thanks
again,

/be

Comment 19

[Phil Schwartau]

Verified Fixed. The testcase above passes on WinNT, Linux, Mac 9.1
in both the debug and optimized JS shells.

Comment 20

[Boris Zbarsky [:bzbarsky]]

*** Bug 114554 has been marked as a duplicate of this bug. ***