741125 - Fix up Paris Bindings WebIDL parser

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Justin Lebar (not reading bugmail)]

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.

Comment 1

[Justin Lebar (not reading bugmail)]

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.  :)

Comment 2

[Justin Lebar (not reading bugmail)]

Attachment: Moving ply/ply/*.py to ply/ (for sane importing).

Comment 3

[Justin Lebar (not reading bugmail)]

Attachment: Return a list in finish(), rather than a set.

Comment 4

[Justin Lebar (not reading bugmail)]

Attachment: Add .gitignore (for .pyc). You'll probably want to delete this before committing to hg.

Comment 5

[Justin Lebar (not reading bugmail)]

Attachment: Allow runtests.py to be run from other directories.

Comment 6

[Justin Lebar (not reading bugmail)]

Attachment: Let runtests.py take --quiet and a list of tests to run.

Comment 7

[Justin Lebar (not reading bugmail)]

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

Comment 8

[Justin Lebar (not reading bugmail)]

Attachment: Adding test_deduplicate, which ensures that duplicate forward-declarations of an interface show up only once.

Comment 9

[Justin Lebar (not reading bugmail)]

Attachment: In runtests.py, properly dump the traceback when an exception occurs.

Comment 10

[Justin Lebar (not reading bugmail)]

Attachment: De-duplicate results in finish(), making test_deduplicate pass.

Comment 11

[Justin Lebar (not reading bugmail)]

Attachment: Remove webidlyacc.py -- it's a generated file!

Comment 12

[Justin Lebar (not reading bugmail)]

Attachment: Define Location._line as an instance variable, not a class variable.

Comment 13

[Justin Lebar (not reading bugmail)]

Attachment: Ignore parser.out.

Comment 14

[Justin Lebar (not reading bugmail)]

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

Comment 15

[Justin Lebar (not reading bugmail)]

Attachment: Remove unnecessary assertion that list(foo) is iterable.

Comment 16

[Justin Lebar (not reading bugmail)]

Attachment: Add enum.length, and use it.

Comment 17

[Justin Lebar (not reading bugmail)]

Attachment: Explicitly set self._finished = false, rather than relying on hasattr.

Comment 18

[Justin Lebar (not reading bugmail)]

Attachment: Don't need to do foo=None before setting foo in both branches of a branch.

Comment 19

[Justin Lebar (not reading bugmail)]

Attachment: Some review comments.

Comment 20

[Justin Lebar (not reading bugmail)]

Attachment: Make special method checking shorter.

Comment 21

[Justin Lebar (not reading bugmail)]

Attachment: Use more pythonic check for any constants in self.members.

Comment 22

[Justin Lebar (not reading bugmail)]

Attachment: Don't pass a billion False's to IDLMethod constructor -- add default params instead.

Comment 23

[Justin Lebar (not reading bugmail)]

Attachment: More review comments.

Comment 24

[Justin Lebar (not reading bugmail)]

Attachment: Make Location and BuiltinLocation have the same interface, and rename BuiltinType::type to BuiltinType::_typeTag

Comment 25

[Justin Lebar (not reading bugmail)]

Attachment: Adding three new tests. They don't all pass.

Comment 26

[Justin Lebar (not reading bugmail)]

Attachment: Rollup patch

Comment 27

[Justin Lebar (not reading bugmail)]

Attachment: Rollup patch

Comment 28

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

I knew my parser code was less than great, but wow.

Comment 29

[Justin Lebar (not reading bugmail)]

(In reply to Kyle Huey [:khuey] ([e-mail]) 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.

Comment 30

[Justin Lebar (not reading bugmail)]

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.

Comment 31

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

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?

Comment 32

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

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?

Comment 33

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

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?

Comment 34

[Justin Lebar (not reading bugmail)]

(In reply to Kyle Huey [:khuey] ([e-mail]) 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.

Comment 35

[Justin Lebar (not reading bugmail)]

(In reply to Kyle Huey [:khuey] ([e-mail]) 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__).

Comment 36

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

(In reply to Justin Lebar [:jlebar] from comment #34)
> (In reply to Kyle Huey [:khuey] ([e-mail]) 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++ :-/

Comment 37

[Justin Lebar (not reading bugmail)]

(In reply to Kyle Huey [:khuey] ([e-mail]) 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?

Comment 38

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

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?

Comment 39

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

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 40

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

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.

Comment 41

[Justin Lebar (not reading bugmail)]

(In reply to Kyle Huey [:khuey] ([e-mail]) 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.  :)

Comment 42

[Justin Lebar (not reading bugmail)]

> > +    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?

Comment 43

[Justin Lebar (not reading bugmail)]

(In reply to Kyle Huey [:khuey] ([e-mail]) 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?).

Comment 44

[Justin Lebar (not reading bugmail)]

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?

Comment 45

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

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

Comment 46

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

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.

Comment 47

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

Comment on attachment 611211 [details] [diff] [review]
Some review comments.

Made some, will file bugs on the others.

Comment 48

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

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.

Comment 49

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

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.

Comment 50

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

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.

Comment 52

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

Attachment: Patch for m-c

Comment 53

[Justin Lebar (not reading bugmail)]

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

Comment 54

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

No I think Ted should review the changes I made to client.py.

Comment 55

[(not currently active) Ted Mielczarek]

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.

Comment 56

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

I attempted to land this, but it broke the tree :-)

Comment 57

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

I meant :-( of course.

Comment 58

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

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.

Comment 59

[Justin Lebar (not reading bugmail)]

Like I said in comment 30, I'm pretty surprised that's not caught by a test.

Comment 60

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

https://hg.mozilla.org/mozilla-central/rev/adef80d2b5a0

Comment 61

[Justin Lebar (not reading bugmail)]

FYI, I'm planning to do another pass over the parser during the trip to or from Australia.

Comment 62

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

And backed out again :-/

https://hg.mozilla.org/mozilla-central/rev/b7c1c0145079

Comment 63

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

I sprinkled some fairy dust on it and it stuck this time.