Issue1285
Created on 2009-03-23.03:02:45 by marcdownie, last changed 2009-04-27.05:07:57 by cgroves.
msg4337 (view) |
Author: Marc Downie (marcdownie) |
Date: 2009-03-23.03:02:44 |
|
A simple java class hierarchy:
package somepackage;
public class Banana {
public void amethod() {}
public void amethod(int x, int y) {}
}
---
and:
package somepackage;
public class Pear extends Banana {
public void amethod(int x, int y)
{
}
}
---
Inside Jython as it ships:
Jython 2.5b3 (Release_2_5beta3:6092, Mar 10 2009, 15:34:57)
[Java HotSpot(TM) Client VM (Apple Inc.)] on java1.5.0_16
Type "help", "copyright", "credits" or "license" for more information.
>>> from somepackage import *
>>> x = Pear()
>>> x.amethod()
All ok.
with python.security.respectJavaAccessibility = false
Jython 2.5b3 (Release_2_5beta3:6092, Mar 10 2009, 15:34:57)
[Java HotSpot(TM) Client VM (Apple Inc.)] on java1.5.0_16
Type "help", "copyright", "credits" or "license" for more information.
>>> from somepackage import *
>>> x = Pear()
>>> x.amethod()
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
TypeError: amethod(): expected 2 args; got 0
It's broken at least this far:
>>> print x.amethod.argslist
array(org.python.core.ReflectedArgs, [class somepackage.Pear, false, 0, public void
somepackage.Pear.amethod(int,int)
(int, int, )])
----
I can't say for sure how long this behavior has been in place (at least as far back as beta1, but maybe
I've gotten lucky for a lot longer than that). Since every method that's implicated here is public I
find it very odd (and very inconvenient) that respectJavaAccessibility = false appears to reduce the
visibility of the method. I'm stepping through PyJavaType#init but I haven't found anything obviously
wrong yet.
|
msg4521 (view) |
Author: Kevin Fitch (kfitch42) |
Date: 2009-04-16.17:34:59 |
|
I think I know where the issue is.
PyJavaType.java line 232
When respectJavaAccessibility==False it uses getDeclaredMethods, which
the java docs say does not find inherited methods.
When respectJavaAccessibility==True it uses getMethods, which does find
inherited methods (but only public ones) ... so we need to walk up the
heirarchy when respectJavaAccessibility==False (possibly making use of
the mro that was just computed) and collect ALL the methods.
|
msg4526 (view) |
Author: Marc Downie (marcdownie) |
Date: 2009-04-17.15:47:05 |
|
I fear it's more complex than this.
If I change my example to:
package somepackage;
public class Banana {
public void amethod() {}
public void amethod(int x, int y) {}
// added this method
public void bmethod(){}
}
---
and:
package somepackage;
public class Pear extends Banana {
public void amethod(int x, int y)
{
}
}
---
then Pear().bmethod() works as expected (even though
Pear.class.getDeclaredMethods() will not contain a bmethod). Had it not
worked I would have seen this bug immediately (I have a lot of code that
runs with respectJavaAccessibility=false).
|
msg4575 (view) |
Author: Kevin Fitch (kfitch42) |
Date: 2009-04-23.03:37:03 |
|
Well, even though you said it wouldn't work I tried tweaking things
with respect to getMethods and getDeclaredMethods. And, I got something
that seems to resolve the bug. patch attached.
Basically I turned the ordering around. I first make all the methods
public, then call getMethods in both cases. I have checked that it
makes the given example work, nothing else.
|
msg4577 (view) |
Author: Marc Downie (marcdownie) |
Date: 2009-04-23.04:15:32 |
|
I'm afraid I don't understand your patch at all --- it seems to remove the
else clause for !respectJavaAccessiblity (which is the clause that is
suspect) completely, and the last line makes the one remaining if clause
irrelevant as far as I can tell.
With your patch applied on r6255 respectJavaAccessibility = false behaves
just like respectJavaAccessibility = true and we can't access non-public
methods. Yes, this makes this particular bug here go away, but ...
If this bug is still active by then, I'll be able to take a close look at
it this weekend.
|
msg4582 (view) |
Author: Kevin Fitch (kfitch42) |
Date: 2009-04-23.10:45:58 |
|
Doh, that will teach me to write code that late at night.
Yeah, the logic in the conditional was backward from my intent, and it
just didn't work. My hope was that when respectJavaAccessibility=false
I could call setAccessible(true) on every method in the hierarchy, then
getMethods would treat them as public, so that we could use getMethods
in both cases. Turns out I this hope had nothing to do with reality :(
I would like to retract the patch then.
|
msg4585 (view) |
Author: Kevin Fitch (kfitch42) |
Date: 2009-04-24.02:37:19 |
|
OK, I really fixed it this time (well mostly ... my new patch almost
definitely needs some tweaks to be more robust). This time I actualy
implemented a real test case that tests both sides of the issue.
|
msg4586 (view) |
Author: Kevin Fitch (kfitch42) |
Date: 2009-04-24.02:38:46 |
|
And here is my test case. Unzip in a sibling directory to where the
jython code is checked out and the included shell script should work
for you, if not it should be pretty trivial to fix up.
|
msg4600 (view) |
Author: Charlie Groves (cgroves) |
Date: 2009-04-27.05:07:41 |
|
I've committed a fix for this in r6266. Rather than trying to build up a set of method signatures just for the
respectJavaAccessibility portion of the code, the commit grabs all the methods, stuffs them into a list, and lets
PyReflectedFunction sort out the argument conflicts.
Both the patch and the committed version will lead to a circumstance where if a superclass declares a private or
package protected method with more restrictive types than a subclass, the subclass method won't be callable from Jython
with those types. The alternative is doing more type analysis for respectJavaAccessibility and not including
superclass methods if they're more specific than subclass methods, but that means the superclass method isn't directly
accessible on the subclass. I believe this is how respectJavaAccessibility has always worked, so I'm going this way to
keep from rocking the boat.
In any case, I advise getting away from respectJavaAccessibility if possible. Its effects on code can be non-obvious,
it's far less solid than the regular Java integration, and it may go away in a future version.
|
|
Date |
User |
Action |
Args |
2009-04-27 05:07:58 | cgroves | set | status: open -> closed resolution: fixed messages:
+ msg4600 |
2009-04-24 14:21:34 | cgroves | set | assignee: cgroves |
2009-04-24 02:38:46 | kfitch42 | set | files:
+ test-for-javaAccessibility.tar.gz messages:
+ msg4586 |
2009-04-24 02:37:20 | kfitch42 | set | files:
+ fix-for-accessibility-bug-redo.patch messages:
+ msg4585 |
2009-04-23 10:45:58 | kfitch42 | set | messages:
+ msg4582 |
2009-04-23 04:15:33 | marcdownie | set | messages:
+ msg4577 |
2009-04-23 03:37:04 | kfitch42 | set | files:
+ fix-for-accessibility-bug.patch keywords:
+ patch messages:
+ msg4575 |
2009-04-17 15:47:05 | marcdownie | set | messages:
+ msg4526 |
2009-04-16 17:34:59 | kfitch42 | set | nosy:
+ kfitch42 messages:
+ msg4521 |
2009-04-04 02:08:37 | zyasoft | set | nosy:
+ cgroves, fwierzbicki, zyasoft |
2009-04-04 02:07:12 | zyasoft | set | priority: high |
2009-03-23 03:02:45 | marcdownie | create | |
|