Last Comment Bug 691113 - pyxpidl throws an exception when a trailing ; is missing from an interface definition
: pyxpidl throws an exception when a trailing ; is missing from an interface de...
Status: RESOLVED FIXED
[mentor=jdm] [lang=python]
:
Product: Core
Classification: Components
Component: Build Config (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla11
Assigned To: Atul Aggarwal
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-10-01 20:05 PDT by Josh Matthews [:jdm] (away until 9/3)
Modified: 2011-11-20 14:20 PST (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Traceback (926 bytes, text/plain)
2011-10-01 23:47 PDT, Josh Matthews [:jdm] (away until 9/3)
no flags Details
Patch v1 (1.05 KB, patch)
2011-11-08 13:14 PST, Atul Aggarwal
no flags Details | Diff | Splinter Review
Patch v1.10 (1.26 KB, patch)
2011-11-08 20:06 PST, Atul Aggarwal
khuey: review+
Details | Diff | Splinter Review

Description Josh Matthews [:jdm] (away until 9/3) 2011-10-01 20:05:53 PDT
interface foo : public nsISupports
{
}

^ note missing ;, the error showing doesn't inform you of that at all.
Comment 1 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-10-01 20:12:04 PDT
Is the bug that it throws an error or that the error message is not informative?  I would dispute that the former is a bug, and if the latter, the bug should be resummarized.
Comment 2 Josh Matthews [:jdm] (away until 9/3) 2011-10-01 21:24:49 PDT
The bug is that it throws. It's coincidental that the exception traceback doesn't seem to give any good indication as to why it threw.
Comment 3 Josh Matthews [:jdm] (away until 9/3) 2011-10-01 23:47:49 PDT
Created attachment 564022 [details]
Traceback

The problem is coming form the lexing error handler in xpidl.py, on line 1372.

http://www.dabeaz.com/ply/ply.html#ply_nn12 says the following:

"On the first occurrence of an error, the user-defined p_error() function is called with the offending token as an argument. However, if the syntax error is due to reaching the end-of-file, p_error() is called with an argument of None. Afterwards, the parser enters an "error-recovery" mode in which it will not make future calls to p_error() until it has successfully shifted at least 3 tokens onto the parsing stack."

Given that the error occurs when there's no semicolon as the last character of an IDL file, I think we can get away with a simple None check.
Comment 4 Josh Matthews [:jdm] (away until 9/3) 2011-10-01 23:48:47 PDT
Sorry, I meant http://www.dabeaz.com/ply/ply.html#ply_nn29 .
Comment 5 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-10-02 04:11:20 PDT
I disagree that it's an error that it throws.  Requiring a semicolon is consistent with WebIDL, and afaik xpidlc.
Comment 6 Benjamin Smedberg [:bsmedberg] 2011-10-02 05:39:38 PDT
It shouldn't throw a python exception (which is the current behavior): it should print a syntax error and exit gracefully with a nonzero error code.
Comment 7 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-10-02 05:49:05 PDT
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #6)
> It shouldn't throw a python exception (which is the current behavior): it
> should print a syntax error and exit gracefully with a nonzero error code.

I agree.

I may have misunderstood what was meant by "exception" here. :-)
Comment 8 Atul Aggarwal 2011-11-08 13:14:27 PST
Created attachment 572980 [details] [diff] [review]
Patch v1

Simple check for None and then print a message and exit. Good enough or something else in mind?
Comment 9 Josh Matthews [:jdm] (away until 9/3) 2011-11-08 14:02:27 PST
Comment on attachment 572980 [details] [diff] [review]
Patch v1

Review of attachment 572980 [details] [diff] [review]:
-----------------------------------------------------------------

::: xpcom/idl-parser/xpidl.py
@@ +1441,5 @@
>          p[0] = list(p[3])
>          p[0].insert(0, p[1])
>  
>      def p_error(self, t):
> +        if t == None:

|if not t| is more idiomatic python.

@@ +1442,5 @@
>          p[0].insert(0, p[1])
>  
>      def p_error(self, t):
> +        if t == None:
> +            print "Syntax Error at end of file. Possibly due to missing semicolon(;), braces(}) or both"

I think it would be better to use the existing IDLError exception, and just pass it a null location object.
Comment 10 Atul Aggarwal 2011-11-08 20:06:14 PST
Created attachment 573068 [details] [diff] [review]
Patch v1.10

Accommodating changes suggested.
Comment 11 Josh Matthews [:jdm] (away until 9/3) 2011-11-08 20:09:21 PST
Comment on attachment 573068 [details] [diff] [review]
Patch v1.10

I'll let Kyle comment on the error message here.
Comment 12 Ted Mielczarek [:ted.mielczarek] 2011-11-09 05:05:26 PST
(In reply to Josh Matthews [:jdm] from comment #9)
> ::: xpcom/idl-parser/xpidl.py
> @@ +1441,5 @@
> >          p[0] = list(p[3])
> >          p[0].insert(0, p[1])
> >  
> >      def p_error(self, t):
> > +        if t == None:
> 
> |if not t| is more idiomatic python.

Drive-by idiomatic Python reference: if you're just testing a truth value, then yes, |if not t:| is the best way. If you really are comparing a value to None, then |if t is not None:| would be the best thing to use here.
Comment 13 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-11-16 05:01:48 PST
Comment on attachment 573068 [details] [diff] [review]
Patch v1.10

I don't really care what the error says.  The text here is fine.
Comment 14 Ed Morley [:emorley] 2011-11-19 18:06:11 PST
In my queue with a few other bits that are being sent to try first and then onto inbound :-)

https://tbpl.mozilla.org/?tree=Try&rev=eb96679e6888
Comment 15 Ed Morley [:emorley] 2011-11-19 18:25:47 PST
Bug 687511 included in that try run failed check-sync-dirs, so had to push again:

https://tbpl.mozilla.org/?tree=Try&rev=329bc2d383a3 
(this changeset is there, is just hidden due to the previous push to try)
Comment 17 Ed Morley [:emorley] 2011-11-20 14:20:28 PST
https://hg.mozilla.org/mozilla-central/rev/65e5ac1a24b8

Note You need to log in before you can comment on or make changes to this bug.