691113 - pyxpidl throws an exception when a trailing ; is missing from an interface definition

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Josh Matthews [:jdm]]

interface foo : public nsISupports
{
}

^ note missing ;, the error showing doesn't inform you of that at all.

Comment 1

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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]]

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]]

Attachment: 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]]

Sorry, I meant http://www.dabeaz.com/ply/ply.html#ply_nn29 .

Comment 5

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

I disagree that it's an error that it throws.  Requiring a semicolon is consistent with WebIDL, and afaik xpidlc.

Comment 6

[Benjamin Smedberg]

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 (Exited; not receiving bugmail, old account, do not use)]

(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]

Attachment: 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]]

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]

Attachment: Patch v1.10

Accommodating changes suggested.

Comment 11

[Josh Matthews [:jdm]]

Comment on attachment 573068 [details] [diff] [review]
Patch v1.10

I'll let Kyle comment on the error message here.

Comment 12

[(not currently active) Ted Mielczarek]

(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 (Exited; not receiving bugmail, old account, do not use)]

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]]

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]]

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 16

[Ed Morley [:emorley]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/65e5ac1a24b8

Comment 17

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/65e5ac1a24b8