Some Bugs
David Shepherdson PHC
Thanks for all your work on Bindgen, particularly the recent fixes
which have meant our usual naming pattern of get/set/has methods
doesn't cause problems when generating bindings!
I've come across four bugs in Bindgen (some of which have been there
since around 2.8/2.9, the last versions we were using before updating
to 2.13). I have simple tests that demonstrate the bugs and I have
patches that I think should fix them (they certainly seem to work for
the project we're using Bindgen for).
What's the best way to report these bugs/send the fixes? I thought I
might be able to upload the tests and patches to the files area of
this group in the Google Groups interface, but it seems that feature
is not available. I don't want to spam everyone with attachments here
on the list (at least, not without prior warning!).
A brief description of the bugs is as follows:
1. When a child class that is Bindable extends a parent class that
isn't, and the parent class has a generics type argument which the
child class fills in (e.g. class Parent<T> and class Child extends
Parent<String>), the bindings generated for properties in the parent
class use the original type argument name, rather than the filled-in
one from the child class (i.e. they'd use 'T' rather than 'String' in
the example above).
2. Bindable inner classes inside interfaces have their bindings
generated with the wrong package/class name.
3. Making enums Bindable doesn't work.
4. If a class has a method with the same name as a property (e.g.
one method called name() and another called getName()), the binding
names aren't generated properly (particularly bad for name()/
getName(), since Bindgen makes its own methods called getName() as
well).
(Sorry if those descriptions are a bit unhelpful -- these bugs are
easier to demonstrate with the tests than to describe.)
Anyway, please let me know what the best way is to get you the test
code and fixes.
Thank you,
David Shepherdson
Stephen Haberman
> What's the best way to report these bugs/send the fixes?
The easiest for me is to just fork the project on github and add commits
to your fork, then send me a pull request.
http://github.com/stephenh/bindgen
> 1. When a child class that is Bindable extends a parent class that
> isn't...
Ah, more type var resolution. Not surprised.
> 2. Bindable inner classes inside interfaces have their bindings
> generated with the wrong package/class name.
>
> 3. Making enums Bindable doesn't work.
Cool.
> 4. If a class has a method with the same name as a property (e.g.
> one method called name() and another called getName())
I think I just fixed this one:
http://github.com/stephenh/bindgen/commit/2fae29c78bac41be512a2f43c158dfdc4811ef8d
:-)
> (Sorry if those descriptions are a bit unhelpful -- these bugs are
> easier to demonstrate with the tests than to describe.)
Nope, that was all great. Tests are always good as well.
> Anyway, please let me know what the best way is to get you the test
> code and fixes.
Give github a try, if you could. Otherwise feel free to send a unified
diff to the mailing list as a patch. I can apply it locally.
Thanks for finding and fixing these bugs--it's greatly appreciated.
- Stephen
Stephen Haberman
> Anyway, please let me know what the best way is to get you the test
> code and fixes.
I just integrated your code and released 2.15--please check it out and
make sure I got everything right.
I tweaked the pom a bit in a way that makes sense to me (added scopes
for the jarjar'd/tests dependencies), but I'm not a maven expert, so
anyone please let me know if the previous poms were better than the
2.15 one.
I fixed that compile error you mentioned in the pull request--I have no
idea how I missed that, nor how long it's been there. Weird.
Also, I really like your fix of the parent/child issue. As is, it's a
little bit of a bandaid. However, I didn't know about the
Types.asMemberOf method. It seems really sweet and could probably
replace all of the by-hand type var resolution I've been doing so far.
E.g. I added a bit more code to your parent/child BindingTest, and
it looks like the setter isn't getting picked up and so the binding
is falsely made read only. I think that could be fixed by more extensive
refactoring towards asMemberOf usage.
Dunno. I need some more time to think about it, but it could
substantially simplify things if it goes well.
So thanks for the fixes and the APT API lesson! :-)
- Stephen
David Shepherdson
> I added a bit more code to your parent/child BindingTest, and
> it looks like the setter isn't getting picked up and so the binding
> is falsely made read only. I think that could be fixed by more extensive
> refactoring towards asMemberOf usage.
Sorry I didn't pick up those test failures when I sent the initial pull request. The reason I never spotted them is that we're mostly using Bindgen at compile-time at the moment, because we need null-safe 'get' protection in the code that's calling it at runtime.
(Short version: we're using Bindgen in a Wicket application, though not using Bindgen-Wicket; using our own way of integrating it, which uses PropertyModel with the path expressions generated by Bindgen bindings.)
So, the comment someone made about adding a getSafelyWithRoot() (or similar) would be exactly what we'd need to be able to forget about the way we're doing it now and use it properly at runtime too!
From a naïve perspective (and not having looked at the code for several months...), that should be fairly easy to hack in, shouldn't it? I know someone posted here a few months ago that they were looking into it, but if nothing came of that, I'd be happy to give it a go...
> So thanks for the fixes and the APT API lesson! :-)
I think the thanks should really go the other way around! I found reading the Bindgen source code to be an excellent way of learning the APT API; it was sheer beginner's luck that I stumbled across Types.asMemberOf() while poking around in the debugger, so I'm glad it turned out to be of use to you.
Thanks Stephen,
David
Stephen Haberman
> So, the comment someone made about adding a getSafelyWithRoot() (or
> similar) would be exactly what we'd need
I just pushed out 2.18 that has a `getSafelyWithRoot` method that, I
assuming, does what you need.
> From a naïve perspective (and not having looked at the code for
> several months...), that should be fairly easy to hack in, shouldn't
> it?
Yep. It was just copy/pasting the `getWithRoot` code, and adding a
`null` check.
> I found reading the Bindgen source code to be an excellent way of
> learning the APT API;
I cannot think of a worse project to learn the APT API from than
Bindgen. Well, maybe if "excellent way of learning" as in "has to do
a lot of hacky crap", then, okay. :-)
> it was sheer beginner's luck that I stumbled across Types.asMemberOf()
I did a spike last week to try and use asMemberOf exclusively, instead
of my hacky type var resolution code. Unfortunately, it's a larger change
than I realized.
When given the TypeElement of a class, very early on I use the
`Elements.allMembers` method to return all inherited methods. Which is
nice, except that by doing so I'm throwing away the DeclaredType for
each base class that `asMemberOf` needs to convert the methods to their
type-parameter-resolved version.
E.g. if:
class Foo<T> { T foo(); }
class Bar extends Foo<String> { }
I get Bar.class, ask for all members, get foo, then later while
processing foo, when I should call asMemberOf to resolve the T, I need
to know that foo came from the "extends Foo<String>" DeclaredType of
Bar. Trying to pass in what I've got (Bar) causes asMemberOf to throw
its IllegalArgumentException.
It doesn't sound like it should be a big change, but it was just enough
for me to give up on it for now. :-)
- Stephen
David Shepherdson
On 4 Nov 2010, at 5.31 pm, Stephen Haberman wrote:
> I just pushed out 2.18 that has a `getSafelyWithRoot` method that, I
> assuming, does what you need.
Perfect -- exactly what we needed! We can now replace pretty much all the bits of reflection-leaning code in our system with alternatives built around Bindgen.
> I cannot think of a worse project to learn the APT API from than
> Bindgen. Well, maybe if "excellent way of learning" as in "has to do
> a lot of hacky crap", then, okay. :-)
:-) Would you believe 'a good example of practical, real-world usage of the APT API'?
> I did a spike last week to try and use asMemberOf exclusively, instead
> of my hacky type var resolution code. Unfortunately, it's a larger change
> than I realized.
...
> It doesn't sound like it should be a big change, but it was just enough
> for me to give up on it for now. :-)
Ah, that's a shame. It was fortunate where my fixes were that we could get away with hacking it like that, but I realise it would be a bit more involved to make it work 'for real'. But still, at least it sounds like it might be doable at some stage in the future.
Anyway, many thanks for the getSafelyWithRoot in 2.18 -- that really helps our code to feel much cleaner!
David