Re: JSNI references with * as the parameter list

[bo...@google.com]

http://gwt-code-reviews.appspot.com/126817/diff/2006/2007
File dev/core/test/com/google/gwt/dev/jjs/impl/JsniRefLookupTest.java
(right):

http://gwt-code-reviews.appspot.com/126817/diff/2006/2007#newcode234
Line 234: JMethod res = (JMethod) lookup("test.Bar::foo(*)", errors);
It seems weird that this should work. If Bar didn't declare foo() (and
the lookup went up to the supertype Foo), this would fail to compile.
It breaks the otherwise-Java style of name lookups used by JSNI to care
about where the method is declared. It also rewards the unnecessary
(and ugly) practice of re-defining a method overload just to make it the
"default".

It's simplest to explain to users that the @class::method(*) wildcard
syntax selects the one method out of the entire supertype/superinterface
hierarchy whose name is "method" without having to get into discussions
about which type a method is declared on.

Also, on that vein, add test code for looking up wildcards on
interfaces.

http://gwt-code-reviews.appspot.com/126817

[Lex Spoon]

On Mon, Jan 4, 2010 at 1:26 PM, <bo...@google.com> wrote:

http://gwt-code-reviews.appspot.com/126817/diff/2006/2007
File dev/core/test/com/google/gwt/dev/jjs/impl/JsniRefLookupTest.java
(right):

http://gwt-code-reviews.appspot.com/126817/diff/2006/2007#newcode234
Line 234: JMethod res = (JMethod) lookup("test.Bar::foo(*)", errors);
It seems weird that this should work. If Bar didn't declare foo() (and
the lookup went up to the supertype Foo), this would fail to compile.
It breaks the otherwise-Java style of name lookups used by JSNI to care
about where the method is declared.  It also rewards the unnecessary
(and ugly) practice of re-defining a method overload just to make it the
"default". 

It's simplest to explain to users that the @class::method(*) wildcard
syntax selects the one method out of the entire supertype/superinterface
hierarchy whose name is "method" without having to get into discussions
about which type a method is declared on.

As would be expected, the trickiest part of this patch has to do with overloading and inheritance.  These are dark corners that good code will not rely on, but our tools have to do something with them.

I'm not in love with the current algorithm, but what precisely shall we do?  It's certainly important, but let's be careful not to bike shed the corner cases.

For this particular test case, I thought that if someone defines Bar and Bar has exactly one foo method, then Bar::foo(*) should be allowed, regardless of what Bar inherits.  It's easy enough to disallow such a reference, but should we?  We have a choice here between supporting good code and eliminating bad code.  I fear that if we try to eliminate all the bad code programmers could write, we will never be able to accept any code at all.  It's just a design guideline, though.  Does anyone else have an opinion about this question?

If we want to rule that case out, then my next suggestion would be to count overloads exactly as a Java compiler would.  That would mean that methods overriding each other would not count as extra overloads.  The main trickiness would be dealing with inheritance that includes bridge methods.  That wouldn't be insurmountable, but it's more complicated than the current solution.

FWIW, the current algorithm is as follows.  I think it's pretty easy to work with.  If the user asks for Foo::bar(*), then start in class Foo and look for methods named bar.  If you see exactly one, then that's the one they mean.  If you see more than one, then the reference is ambiguous.  If you see zero, then recurse into inherited superclasses and methods.

Also, on that vein, add test code for looking up wildcards on
interfaces.

Good idea, I will.

Lex

[sco...@google.com]

I haven't looked at this, but should JsniCheckerTest get a new case as
well?

http://gwt-code-reviews.appspot.com/126817

[BobV]

> I'm not in love with the current algorithm, but what precisely shall we do?

For a given type, collect all method signatures that could conceivably
be invoked via that type (and its superclasses or superinterfaces) and
succeed only if there is exactly one unique signature with the given
method name.

As an implementation optimization, lookups on a concrete class can
just crawl the supertype chain, but lookups on abstract type would
have to consider all superinterfaces as well.

--
Bob Vawter
Google Web Toolkit Team

[Lex Spoon]

On Tue, Jan 5, 2010 at 12:20 PM, <sco...@google.com> wrote:

I haven't looked at this, but should JsniCheckerTest get a new case as
well?

Sure, I'll add one.  -Lex

[Lex Spoon]

On Tue, Jan 5, 2010 at 12:40 PM, BobV <bo...@google.com> wrote:

> I'm not in love with the current algorithm, but what precisely shall we do?

For a given type, collect all method signatures that could conceivably
be invoked via that type (and its superclasses or superinterfaces) and
succeed only if there is exactly one unique signature with the given
method name.

Okay, John T. wants the same algorithm.  I like it myself, anyway.  New patch in just a minute.

The main trick is in dealing with bridge methods.  I believe they should be ignored, because you can't normally call them in Java.  However, we unfortunately currently allow them.  So, as a compromise, the latest patch allows *direct* access to bridge methods, but it ignores them when resolving a wildcard.

Lex Spoon

[Lex Spoon]

On Tue, Jan 5, 2010 at 4:38 PM, Lex Spoon <sp...@google.com> wrote:

The main trick is in dealing with bridge methods.  I believe they should be ignored, because you can't normally call them in Java.  However, we unfortunately currently allow them.  So, as a compromise, the latest patch allows *direct* access to bridge methods, but it ignores them when resolving a wildcard.

To give an example, suppose StringHolder extends Settable<String>, like this:

abstract class Settable<T> {

  abstract void set(T x);

}

class StringHolder {

  void set(String x) { /*...*/ }

}

From the point of view of source code, StringHolder has only a single set() method, and it takes a String as an argument.  However, in the Java bytecode, it will have two set methods: it will have the String one, and a bridge method that takes an Object as an argument.  The Object one just forwards to the String one.  The Object one would be used in code like this:

  void setToNull(Settable<T> settable) {

    settable.set(null);  // calls the bridge method

  }

The current patch considers @StringHolder::set(*) to be unambiguous, because it ignores bridge methods.

Lex

 

[bo...@google.com]

LGTM, just some nits

http://gwt-code-reviews.appspot.com/126817/diff/2024/2038
File dev/core/src/com/google/gwt/dev/jjs/impl/JsniRefLookup.java
(right):

http://gwt-code-reviews.appspot.com/126817/diff/2024/2038#newcode118
Line 118: // Backward compatibility: allow accessing bridge methods with
full
Use block quote.

http://gwt-code-reviews.appspot.com/126817/diff/2024/2040
File dev/core/src/com/google/gwt/dev/js/rhino/TokenStream.java (right):

http://gwt-code-reviews.appspot.com/126817/diff/2024/2040#newcode1377
Line 1377: // First check for the special case of ?? as the parameter
list, indicating
Update the comment with *

http://gwt-code-reviews.appspot.com/126817/diff/2024/2039
File dev/core/src/com/google/gwt/dev/shell/DispatchClassInfo.java
(right):

http://gwt-code-reviews.appspot.com/126817/diff/2024/2039#newcode236
Line 236: private List<Member> removeSyntheticMembers(Collection<Member>
members) {
Since this doesn't mutate the input collection, rename it to "filter"

http://gwt-code-reviews.appspot.com/126817/diff/2024/2041
File dev/core/src/com/google/gwt/dev/util/JsniRef.java (right):

http://gwt-code-reviews.appspot.com/126817/diff/2024/2041#newcode128
Line 128:
Unnecessary blank lines.

http://gwt-code-reviews.appspot.com/126817/diff/2024/2026
File dev/core/test/com/google/gwt/dev/jjs/impl/JsniRefLookupTest.java
(right):

http://gwt-code-reviews.appspot.com/126817/diff/2024/2026#newcode50
Line 50: public void testConstructors() {
Sort order.

http://gwt-code-reviews.appspot.com/126817

[sp...@google.com]

Thanks for the review, Bob, and for the helpful iteration, Scott, Ray
C., and John T.

It's committed at r7368.

http://gwt-code-reviews.appspot.com/126817/diff/2024/2038
File dev/core/src/com/google/gwt/dev/jjs/impl/JsniRefLookup.java
(right):

http://gwt-code-reviews.appspot.com/126817/diff/2024/2038#newcode118
Line 118: // Backward compatibility: allow accessing bridge methods with
full

On 2010/01/06 23:02:41, bobv wrote:
> Use block quote.

Done.

http://gwt-code-reviews.appspot.com/126817/diff/2024/2040
File dev/core/src/com/google/gwt/dev/js/rhino/TokenStream.java (right):

http://gwt-code-reviews.appspot.com/126817/diff/2024/2040#newcode1377
Line 1377: // First check for the special case of ?? as the parameter
list, indicating

On 2010/01/06 23:02:41, bobv wrote:
> Update the comment with *

Done.

http://gwt-code-reviews.appspot.com/126817/diff/2024/2039
File dev/core/src/com/google/gwt/dev/shell/DispatchClassInfo.java
(right):

http://gwt-code-reviews.appspot.com/126817/diff/2024/2039#newcode236
Line 236: private List<Member> removeSyntheticMembers(Collection<Member>
members) {

Done. "filterOutSyntheticMembers".

http://gwt-code-reviews.appspot.com/126817/diff/2024/2041
File dev/core/src/com/google/gwt/dev/util/JsniRef.java (right):

http://gwt-code-reviews.appspot.com/126817/diff/2024/2041#newcode128
Line 128:
Done. Auto-sorting must have produced them.

http://gwt-code-reviews.appspot.com/126817