The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in mozilla11

Status

()

Core
Build Config
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: jdm, Assigned: Atul Aggarwal)

Tracking

Trunk
mozilla11
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [mentor=jdm] [lang=python])

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

6 years ago
interface foo : public nsISupports
{
}

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

Updated

6 years ago
Whiteboard: [mentor=jdm]
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.
(Reporter)

Comment 2

6 years ago
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.
(Reporter)

Comment 3

6 years ago
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.
(Reporter)

Comment 4

6 years ago
Sorry, I meant http://www.dabeaz.com/ply/ply.html#ply_nn29 .
I disagree that it's an error that it throws.  Requiring a semicolon is consistent with WebIDL, and afaik xpidlc.
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.
(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. :-)
(Reporter)

Updated

6 years ago
Whiteboard: [mentor=jdm] → [mentor=jdm] [lang=python]
(Assignee)

Comment 8

6 years ago
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?
Assignee: nobody → atulagrwl
Attachment #572980 - Flags: review?(josh)
(Reporter)

Comment 9

6 years ago
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.
Attachment #572980 - Flags: review?(josh)
(Assignee)

Comment 10

6 years ago
Created attachment 573068 [details] [diff] [review]
Patch v1.10

Accommodating changes suggested.
Attachment #572980 - Attachment is obsolete: true
Attachment #573068 - Flags: review?(josh)
(Reporter)

Comment 11

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

I'll let Kyle comment on the error message here.
Attachment #573068 - Flags: review?(josh) → review?(khuey)
(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 on attachment 573068 [details] [diff] [review]
Patch v1.10

I don't really care what the error says.  The text here is fine.
Attachment #573068 - Flags: review?(khuey) → review+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
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
Status: NEW → ASSIGNED
Keywords: checkin-needed
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)
https://hg.mozilla.org/integration/mozilla-inbound/rev/65e5ac1a24b8
OS: Mac OS X → All
Hardware: x86 → All
Target Milestone: --- → mozilla11
https://hg.mozilla.org/mozilla-central/rev/65e5ac1a24b8
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.