Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Fix up Paris Bindings WebIDL parser

RESOLVED FIXED in mozilla14

Status

()

Core
DOM
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Justin Lebar (not reading bugmail), Assigned: khuey)

Tracking

unspecified
mozilla14
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(25 attachments, 2 obsolete attachments)

475 bytes, patch
khuey
: review+
khuey
: checkin+
Details | Diff | Splinter Review
773 bytes, patch
khuey
: review+
khuey
: checkin+
Details | Diff | Splinter Review
290 bytes, patch
khuey
: review+
khuey
: checkin+
Details | Diff | Splinter Review
1.07 KB, patch
khuey
: review+
khuey
: checkin+
Details | Diff | Splinter Review
3.30 KB, patch
khuey
: review+
khuey
: checkin+
Details | Diff | Splinter Review
863 bytes, patch
khuey
: review+
khuey
: checkin+
Details | Diff | Splinter Review
819 bytes, patch
khuey
: review+
khuey
: checkin+
Details | Diff | Splinter Review
2.08 KB, patch
khuey
: review+
khuey
: checkin+
Details | Diff | Splinter Review
1010 bytes, patch
khuey
: review+
khuey
: checkin+
Details | Diff | Splinter Review
46.09 KB, patch
khuey
: review+
khuey
: checkin+
Details | Diff | Splinter Review
956 bytes, patch
khuey
: review+
khuey
: checkin+
Details | Diff | Splinter Review
238 bytes, patch
khuey
: review+
khuey
: checkin+
Details | Diff | Splinter Review
2.13 KB, patch
khuey
: review+
khuey
: checkin+
Details | Diff | Splinter Review
897 bytes, patch
khuey
: review+
khuey
: checkin+
Details | Diff | Splinter Review
1.64 KB, patch
khuey
: review+
khuey
: checkin+
Details | Diff | Splinter Review
1.55 KB, patch
khuey
: review+
khuey
: checkin+
Details | Diff | Splinter Review
830 bytes, patch
khuey
: review+
khuey
: checkin+
Details | Diff | Splinter Review
3.33 KB, patch
khuey
: review+
khuey
: checkin+
Details | Diff | Splinter Review
5.55 KB, patch
khuey
: review+
khuey
: checkin+
Details | Diff | Splinter Review
856 bytes, patch
khuey
: review+
khuey
: checkin+
Details | Diff | Splinter Review
3.10 KB, patch
khuey
: review+
khuey
: checkin+
Details | Diff | Splinter Review
1.96 KB, patch
Details | Diff | Splinter Review
7.13 KB, patch
khuey
: review+
khuey
: checkin+
Details | Diff | Splinter Review
4.84 KB, patch
khuey
: review+
khuey
: checkin+
Details | Diff | Splinter Review
43.17 KB, patch
ted
: review+
Details | Diff | Splinter Review
(Reporter)

Description

5 years ago
I've been reviewing and editing the Paris Bindings WebIDL parser, and Kyle and I can use this bug to track at least one iteration of fixing it up.
(Reporter)

Comment 1

5 years ago
Kyle, I'm not entirely sure how we want to do this.  I have a bunch of patches, which I'm going to attach here.  I'll also attach a roll-up patch with all the changes.

Some of these patches are fixes, which you'll need to review.  Some of them are questions of mine for you -- these are marked with "REVIEW".

I also added some tests which currently fail.  In particular, test_array_of_interface asserts, and test_nullable_equivalency fails for the same reason.  There's a separate failure in test_nullable_equivalency, which you'll see after we fix the assertion.

I'm not done with my review, but we may want to use more than one bug for the review.  It seems to me that the easiest way to handle this would be to push the code to github, hack on it there, and eventually push back to m-c, but I'm not sure you'd like that.  :)
(Reporter)

Comment 2

5 years ago
Created attachment 611194 [details] [diff] [review]
Moving ply/ply/*.py to ply/ (for sane importing).
(Reporter)

Comment 3

5 years ago
Created attachment 611195 [details] [diff] [review]
Return a list in finish(), rather than a set.
(Reporter)

Comment 4

5 years ago
Created attachment 611196 [details] [diff] [review]
Add .gitignore (for .pyc).  You'll probably want to delete this before committing to hg.
(Reporter)

Comment 5

5 years ago
Created attachment 611197 [details] [diff] [review]
Allow runtests.py to be run from other directories.
(Reporter)

Comment 6

5 years ago
Created attachment 611198 [details] [diff] [review]
Let runtests.py take --quiet and a list of tests to run.
(Reporter)

Comment 7

5 years ago
Created attachment 611199 [details] [diff] [review]
Remove list(set()) in finish() -- it breaks test_attrs.  We now need a failing test for the lack of de-duplication afforded by this checkin.
(Reporter)

Comment 8

5 years ago
Created attachment 611200 [details] [diff] [review]
Adding test_deduplicate, which ensures that duplicate forward-declarations of an interface show up only once.
(Reporter)

Comment 9

5 years ago
Created attachment 611201 [details] [diff] [review]
In runtests.py, properly dump the traceback when an exception occurs.
(Reporter)

Comment 10

5 years ago
Created attachment 611202 [details] [diff] [review]
De-duplicate results in finish(), making test_deduplicate pass.
(Reporter)

Comment 11

5 years ago
Created attachment 611203 [details] [diff] [review]
Remove webidlyacc.py -- it's a generated file!
(Reporter)

Comment 12

5 years ago
Created attachment 611204 [details] [diff] [review]
Define Location._line as an instance variable, not a class variable.
(Reporter)

Comment 13

5 years ago
Created attachment 611205 [details] [diff] [review]
Ignore parser.out.
(Reporter)

Comment 14

5 years ago
Created attachment 611206 [details] [diff] [review]
Don't cut off the end of the lines without terminating '\n's in error messages, rewrite error message '^' placement code, and add a test.
(Reporter)

Comment 15

5 years ago
Created attachment 611207 [details] [diff] [review]
Remove unnecessary assertion that list(foo) is iterable.
(Reporter)

Comment 16

5 years ago
Created attachment 611208 [details] [diff] [review]
Add enum.length, and use it.
(Reporter)

Comment 17

5 years ago
Created attachment 611209 [details] [diff] [review]
Explicitly set self._finished = false, rather than relying on hasattr.
(Reporter)

Comment 18

5 years ago
Created attachment 611210 [details] [diff] [review]
Don't need to do foo=None before setting foo in both branches of a branch.
(Reporter)

Comment 19

5 years ago
Created attachment 611211 [details] [diff] [review]
Some review comments.
(Reporter)

Comment 20

5 years ago
Created attachment 611212 [details] [diff] [review]
Make special method checking shorter.
(Reporter)

Comment 21

5 years ago
Created attachment 611213 [details] [diff] [review]
Use more pythonic check for any constants in self.members.
(Reporter)

Comment 22

5 years ago
Created attachment 611214 [details] [diff] [review]
Don't pass a billion False's to IDLMethod constructor -- add default params instead.
(Reporter)

Comment 23

5 years ago
Created attachment 611215 [details] [diff] [review]
More review comments.
(Reporter)

Comment 24

5 years ago
Created attachment 611216 [details] [diff] [review]
Make Location and BuiltinLocation have the same interface, and rename BuiltinType::type to BuiltinType::_typeTag
(Reporter)

Comment 25

5 years ago
Created attachment 611217 [details] [diff] [review]
Adding three new tests.  They don't all pass.
(Reporter)

Comment 26

5 years ago
Created attachment 611219 [details] [diff] [review]
Rollup patch
(Reporter)

Updated

5 years ago
Attachment #611219 - Attachment is obsolete: true
(Reporter)

Comment 27

5 years ago
Created attachment 611220 [details] [diff] [review]
Rollup patch
Attachment #611220 - Flags: review?(khuey)
I knew my parser code was less than great, but wow.
(Reporter)

Comment 29

5 years ago
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #28)
> I knew my parser code was less than great, but wow.

Well, it might have been smaller as a set of review comments, I'm not sure.  I was hoping a patch queue would be easier overall, but I might have been wrong.  :)

The big thing is the edge cases where the parser falls over or does the wrong thing.  We're not out of the woods in that respect.
(Reporter)

Comment 30

5 years ago
Comment on attachment 611216 [details] [diff] [review]
Make Location and BuiltinLocation have the same interface, and rename BuiltinType::type to BuiltinType::_typeTag

This one is necessary to pass some of the tests I added.  There are cases when we'd try to read _file on a BuiltinLocation, which didn't work.

Although looking at this, it has a bug which I'm surprised isn't caught by any tests:

+    def filename(self):
+        return self.filename
+

should be

        return self._file.
Attachment #611194 - Flags: review+
Attachment #611195 - Flags: review+
Comment on attachment 611196 [details] [diff] [review]
Add .gitignore (for .pyc).  You'll probably want to delete this before committing to hg.

What about the various other things in .hgignore?
Attachment #611197 - Flags: review+
Attachment #611199 - Flags: review+
Attachment #611200 - Flags: review+
Attachment #611201 - Flags: review+
Comment on attachment 611202 [details] [diff] [review]
De-duplicate results in finish(), making test_deduplicate pass.

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

::: WebIDL.py
@@ +2865,5 @@
> +        for p in self._productions:
> +            if p not in seen:
> +                seen.add(p)
> +                result.append(p)
> +        return result

Why do we need the set and the array here?  Isn't if p not in result: enough?
Attachment #611203 - Flags: review+
Comment on attachment 611204 [details] [diff] [review]
Define Location._line as an instance variable, not a class variable.

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

Is _line actually used?
Attachment #611205 - Flags: review+
Attachment #611207 - Flags: review+
Attachment #611206 - Flags: review+
Attachment #611208 - Flags: review+
(Reporter)

Comment 34

5 years ago
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #32)
> Comment on attachment 611202 [details] [diff] [review]
> De-duplicate results in finish(), making test_deduplicate pass.
> 
> Review of attachment 611202 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: WebIDL.py
> @@ +2865,5 @@
> > +        for p in self._productions:
> > +            if p not in seen:
> > +                seen.add(p)
> > +                result.append(p)
> > +        return result
> 
> Why do we need the set and the array here?  Isn't if p not in result: enough?

Sure, but that's O(n^2) in the number of interfaces.  Which may be fine -- I dunno.
(Reporter)

Comment 35

5 years ago
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #33)
> Comment on attachment 611204 [details] [diff] [review]
> Define Location._line as an instance variable, not a class variable.
> 
> Review of attachment 611204 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Is _line actually used?

Yes, in lots of places (e.g. Location.__str__).
(In reply to Justin Lebar [:jlebar] from comment #34)
> (In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #32)
> > Comment on attachment 611202 [details] [diff] [review]
> > De-duplicate results in finish(), making test_deduplicate pass.
> > 
> > Review of attachment 611202 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: WebIDL.py
> > @@ +2865,5 @@
> > > +        for p in self._productions:
> > > +            if p not in seen:
> > > +                seen.add(p)
> > > +                result.append(p)
> > > +        return result
> > 
> > Why do we need the set and the array here?  Isn't if p not in result: enough?
> 
> Sure, but that's O(n^2) in the number of interfaces.  Which may be fine -- I
> dunno.

Aha.  For some reason I only think about perf when writing C++ :-/
Attachment #611202 - Flags: review+
Attachment #611204 - Flags: review+
(Reporter)

Comment 37

5 years ago
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #31)
> Comment on attachment 611196 [details] [diff] [review]
> Add .gitignore (for .pyc).  You'll probably want to delete this before
> committing to hg.
> 
> What about the various other things in .hgignore?

.hgignore will take care of .pyc.  The gitignore also has webidlyacc.py and parser.out, but I thought you were planning to move these to the objdir?
Attachment #611209 - Flags: review+
Attachment #611210 - Flags: review+
Attachment #611213 - Flags: review+
Attachment #611196 - Flags: review+
Comment on attachment 611198 [details] [diff] [review]
Let runtests.py take --quiet and a list of tests to run.

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

::: runtests.py
@@ +102,5 @@
>  
>  if __name__ == '__main__':
> +    usage = """%prog [OPTIONS] [TESTS]
> +               Where TESTS are relative to the tests directory."""
> +    parser = optparse.OptionParser(usage='%prog [OPTIONS] [TESTS]')

Why have a usage string and then not use it?
Attachment #611198 - Flags: review-
Comment on attachment 611211 [details] [diff] [review]
Some review comments.

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

::: WebIDL.py
@@ +196,5 @@
>              return self._name.QName() + "::"
>          return "::"
>  
>      def ensureUnique(self, identifier, object):
> +        # REVIEW: This is tricky; please add a docstring.

What part exactly do you find tricky about this?

@@ +289,5 @@
>          return "<unresolved scope>::" + self.name
>  
>      def resolve(self, scope, object):
> +        # REVIEW: It would be good to explain the difference between resolve
> +        # and finish, in general.

Agreed.

@@ +378,5 @@
>      def __str__(self):
>          return "Interface '%s'" % self.identifier.name
>  
>      def ctor(self):
> +        # REVIEW: getCtor(), getConstructor()?

Yeah, probably.

@@ +899,5 @@
> +        # REVIEW: Is this, and the rest of the forwarding to inner below,
> +        # correct?  Like, why should the type "string[]" return true for
> +        # isString()?
> +        #
> +        # This appears in other types, too.

No, its totally bogus.
Comment on attachment 611215 [details] [diff] [review]
More review comments.

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

::: WebIDL.py
@@ +606,5 @@
>      def isType(self):
>          return True
>  
>      def nullable(self):
> +        # REVIEW: isNullable?

Yes.

@@ +694,5 @@
>          assert obj
>          if obj.isType():
>              return obj
>  
> +        name = self.name.resolve(scope, None) # Review: why None, rather than obj, here?

Good question.  I'll have to try to page that back in.
(Reporter)

Comment 41

5 years ago
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #40)
> Comment on attachment 611215 [details] [diff] [review]
>
> More review comments.
> @@ +694,5 @@
> >          assert obj
> >          if obj.isType():
> >              return obj
> >  
> > +        name = self.name.resolve(scope, None) # Review: why None, rather than obj, here?
> 
> Good question.  I'll have to try to page that back in.

Just add a comment once you figure it out, if you don't mind.  Seems important, whatever is going on here.  :)
(Reporter)

Comment 42

5 years ago
> > +    parser = optparse.OptionParser(usage='%prog [OPTIONS] [TESTS]')
>
> Why have a usage string and then not use it?

The usage string is just the first line of '--help':

> Usage: runtests.py [OPTIONS] [TESTS]
>
> Options:
>   -h, --help   show this help message and exit
>   -q, --quiet  Don't print passing tests.

Does this look OK to you?
(Reporter)

Comment 43

5 years ago
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #39)
> Comment on attachment 611211 [details] [diff] [review]
> Some review comments.
> 
> Review of attachment 611211 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: WebIDL.py
> @@ +196,5 @@
> >              return self._name.QName() + "::"
> >          return "::"
> >  
> >      def ensureUnique(self, identifier, object):
> > +        # REVIEW: This is tricky; please add a docstring.
> 
> What part exactly do you find tricky about this?

Specifically the object argument.  It wasn't clear to me what object I should be passing in.  And there's a rule that you can't pass in the same object twice (and, iirc, the object may be null, so what does that mean?).
(Reporter)

Comment 44

5 years ago
How do you want to handle landing these changes plus review comments?  Do you want to qimport these patches (and qfold some of them, or whatever) and make the changes we've agreed on here yourself?  Or do you want some help from me somehow?
(In reply to Justin Lebar [:jlebar] from comment #44)
> How do you want to handle landing these changes plus review comments?  Do
> you want to qimport these patches (and qfold some of them, or whatever) and
> make the changes we've agreed on here yourself?

Yeah, I think that's the way to go.
Assignee: nobody → khuey
Attachment #611195 - Flags: checkin+
Attachment #611196 - Flags: checkin+
Attachment #611197 - Flags: checkin+
Attachment #611199 - Flags: checkin+
Attachment #611200 - Flags: checkin+
Comment on attachment 611198 [details] [diff] [review]
Let runtests.py take --quiet and a list of tests to run.

I changed this to actually use the usage variable.
Attachment #611198 - Flags: review-
Attachment #611198 - Flags: review+
Attachment #611198 - Flags: checkin+
Attachment #611201 - Flags: checkin+
Attachment #611202 - Flags: checkin+
Attachment #611203 - Flags: checkin+
Attachment #611204 - Flags: checkin+
Attachment #611205 - Flags: checkin+
Attachment #611206 - Flags: checkin+
Attachment #611207 - Flags: checkin+
Attachment #611208 - Flags: review+
Attachment #611209 - Flags: checkin+
Attachment #611210 - Flags: checkin+
Attachment #611213 - Flags: review+
Attachment #611208 - Flags: review+ → checkin+
Attachment #611213 - Flags: review+ → checkin+
Attachment #611212 - Flags: review+
Attachment #611212 - Flags: checkin+
Comment on attachment 611211 [details] [diff] [review]
Some review comments.

Made some, will file bugs on the others.
Attachment #611211 - Flags: review+
Attachment #611211 - Flags: checkin+
Comment on attachment 611214 [details] [diff] [review]
Don't pass a billion False's to IDLMethod constructor -- add default params instead.

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

::: WebIDL.py
@@ +1464,5 @@
>      def __init__(self, location, identifier, returnType, arguments,
> +                 static=False, getter=False, setter=False, creator=False,
> +                 deleter=False, specialType=NamedOrIndexed.Neither,
> +                 legacycaller=False, stringifier=False):
> +        # REVIEW: specialType is NamedOrIndexed -- wow, this is messed up.

Will file a bug on this.
Attachment #611214 - Flags: review+
Attachment #611214 - Flags: checkin+
Comment on attachment 611216 [details] [diff] [review]
Make Location and BuiltinLocation have the same interface, and rename BuiltinType::type to BuiltinType::_typeTag

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

::: WebIDL.py
@@ +785,5 @@
>          self.name = self.inner.name
>          return self
>  
>      def unroll(self):
> +        # REVIEW: Should this be self.inner.unroll()?

Yes, and that's not the only place that has this problem.
Attachment #611216 - Flags: review+
Attachment #611216 - Flags: checkin+
Comment on attachment 611217 [details] [diff] [review]
Adding three new tests.  They don't all pass.

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

And a commit to make these tests pass.
Attachment #611217 - Flags: review+
Attachment #611217 - Flags: checkin+
Attachment #611220 - Attachment is obsolete: true
Attachment #611220 - Flags: review?(khuey)
Attachment #611194 - Flags: checkin+

Updated

5 years ago
Duplicate of this bug: 742165
Created attachment 612230 [details] [diff] [review]
Patch for m-c
Attachment #612230 - Flags: review?(ted.mielczarek)
(Reporter)

Comment 53

5 years ago
Comment on attachment 612230 [details] [diff] [review]
Patch for m-c

Do you think Ted needs to review the changes we made to WebIDL.py?  They had two sets of eyes on them...
No I think Ted should review the changes I made to client.py.
Comment on attachment 612230 [details] [diff] [review]
Patch for m-c

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

::: client.py
@@ +52,5 @@
> +    if os.path.exists(fulldir):
> +        shutil.rmtree(fulldir)
> +
> +    assert not os.path.exists(fulldir)
> +    check_call_noisy([hg, 'clone', '-u', tag, repository, fulldir])

Maybe you should just call do_hg_pull here, or merge this function with do_hg_pull? There's a bit of overlap.
Attachment #612230 - Flags: review?(ted.mielczarek) → review+
I attempted to land this, but it broke the tree :-)
I meant :-( of course.
Comment on attachment 611216 [details] [diff] [review]
Make Location and BuiltinLocation have the same interface, and rename BuiltinType::type to BuiltinType::_typeTag

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

::: WebIDL.py
@@ +106,5 @@
>          return self._lexpos == other._lexpos and \
>                 self._file == other._file
>  
> +    def filename(self):
> +        return self.filename

Should be self._file of course.
(Reporter)

Comment 59

5 years ago
Like I said in comment 30, I'm pretty surprised that's not caught by a test.
https://hg.mozilla.org/mozilla-central/rev/adef80d2b5a0
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla14
(Reporter)

Comment 61

5 years ago
FYI, I'm planning to do another pass over the parser during the trip to or from Australia.
And backed out again :-/

https://hg.mozilla.org/mozilla-central/rev/b7c1c0145079
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
I sprinkled some fairy dust on it and it stuck this time.
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.