JSNI references with ?? as the parameter list

[sp...@google.com]

Reviewers: bobv,

Description:
This patch allows JSNI references to non-overloaded methods to use ?? as
the parameter list. This saves a typing for the most common references
to methods.

Implementation notes:

The best place to look first is JavaAccessFromJavaScriptTest.java .
This adds tests for JSNI refs, including JSNI refs that use ?? .

The other tests in there test individual classes in the implementation.

In the implemantation, the updates are:

Tokenizer -- for tokenizing JSNI refs that use ??
JsniRef -- for parsing them
JsniRefLookup -- for looking them up in prod mode
ClassDispatchInfo -- for looking them up in dev mode

Please review this at http://gwt-code-reviews.appspot.com/126817

Affected files:
dev/core/src/com/google/gwt/dev/jjs/impl/JsniRefLookup.java
dev/core/src/com/google/gwt/dev/js/rhino/TokenStream.java
dev/core/src/com/google/gwt/dev/shell/DispatchClassInfo.java
dev/core/src/com/google/gwt/dev/util/JsniRef.java
dev/core/test/com/google/gwt/dev/jjs/impl/JsniRefLookupTest.java
dev/core/test/com/google/gwt/dev/js/TokenStreamTest.java
dev/core/test/com/google/gwt/dev/shell/DispatchClassInfoTest.java
dev/core/test/com/google/gwt/dev/util/JsniRefTest.java
user/test/com/google/gwt/dev/jjs/CompilerSuite.java
user/test/com/google/gwt/dev/jjs/test/JavaAccessFromJavaScriptTest.java

[cromw...@gmail.com]

On 2009/12/22 19:20:36, Lex wrote:

Very cool. Lex, wasn't there also a proposal a while back to allow the
current class to be omitted if you're referring to a method in the same
class, e.g.

@this::someMethod(??)(a,b)

or

@class::someMethod(??)(a,b) ?

or maybe just

@::someMethod(??)(a,b)

Maybe we should roll that one in too.

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

[John Tamplin]

On Tue, Dec 22, 2009 at 2:24 PM, <cromw...@gmail.com> wrote:

Very cool. Lex, wasn't there also a proposal a while back to allow the
current class to be omitted if you're referring to a method in the same
class, e.g.

@this::someMethod(??)(a,b)

or

@class::someMethod(??)(a,b) ?

or maybe just

@::someMethod(??)(a,b)

I believe this last one was the syntax we settled on (though at the time we discussed * to mean any arguments).  Also, we discussed using imports for class resolution, though I am not sure how hard it is to get at them for JDT (and it doesn't impact IHM, since we already have to read the source to get JSNI source anyway). 

--
John A. Tamplin
Software Engineer (GWT), Google

[Lex Spoon]

On Tue, Dec 22, 2009 at 2:29 PM, John Tamplin <j...@google.com> wrote:
> On Tue, Dec 22, 2009 at 2:24 PM, <cromw...@gmail.com> wrote:
>>
>> Very cool. Lex, wasn't there also a proposal a while back to allow the
>> current class to be omitted if you're referring to a method in the same
>> class, e.g.
>>
>> @this::someMethod(??)(a,b)
>>
>> or
>>
>> @class::someMethod(??)(a,b) ?
>>
>> or maybe just
>>
>> @::someMethod(??)(a,b)
>
> I believe this last one was the syntax we settled on (though at the time we
> discussed * to mean any arguments).

I would like being able to leave out the class type. It's simply
extra work and somebody has to get around to it. Already this patch
has been implemented for months now.

Do you remember, John, if we included the :: if the class type was
left off? I was thinking we could do @someMethod(??)(a,b), which is
more concise.

Regording * vs. ?, I used * to begin with, and there was a request to
change it to ? at the meeting. I changed it to ?? in this patch
because potentially we'd want to use a single ? as an individual
wildcard type rather than a sequence of them.

> Also, we discussed using imports for
> class resolution, though I am not sure how hard it is to get at them for JDT
> (and it doesn't impact IHM, since we already have to read the source to get
> JSNI source anyway).

Yes, that would be good, too. This improvement is the hardest one of
the bunch. In theory, the JDT should provide the information via its
"scope" and/or "environment" objects. That's good, because then we
can use the true Java naming rules including accessing inner types
from parent classes.

Lex

[John Tamplin]

On Tue, Dec 22, 2009 at 2:35 PM, Lex Spoon <sp...@google.com> wrote:

Do you remember, John, if we included the :: if the class type was

left off?  I was thinking we could do @someMethod(??)(a,b), which is
more concise.

I think we discussed leaving it as @::method, but I don't remember if there was any reason we needed the ::, other than clearly identifying that method is a method name and not some class.

[BobV]

One quick note while I'm looking through the code is that using "??"
as the wildcard is confusing. Is that one question-mark for each
parameter, meaning up to two parameters? Using a asterisk would be
more in keeping with regex-esque syntax.

--
Bob Vawter
Google Web Toolkit Team

[bo...@google.com]

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

http://gwt-code-reviews.appspot.com/126817/diff/1/6#newcode109
Line 109: Set<JMethod> almostMatches = new HashSet<JMethod>();
The unordered set will produce an unstable error message below.

http://gwt-code-reviews.appspot.com/126817/diff/1/6#newcode154
Line 154: TreeSet<String> almostMatchSigs = new TreeSet<String>();
This set isn't used.

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

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

http://gwt-code-reviews.appspot.com/126817/diff/1/8#newcode1381
Line 1381: reportSyntaxError("msg.jsni.expected.char", new String[] {
")" });
Isn't the expected character another question-mark?

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

http://gwt-code-reviews.appspot.com/126817/diff/1/7#newcode37
Line 37: * <code>memberById</code>, but there are other instances that
also match.
Provide an example in this javadoc.

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

http://gwt-code-reviews.appspot.com/126817/diff/1/9#newcode174
Line 174: * {@link #matchesAnyOverload()} is false.
Add assertions to the getter methods that enforce what's in the javadoc.

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

http://gwt-code-reviews.appspot.com/126817/diff/1/2#newcode129
Line 129: public void testWildcardLookups() {
What about

class A {
void method1();
}

class B {
void method1(int a);
}

What should @test.B::method1(??) resolve to?

http://gwt-code-reviews.appspot.com/126817/diff/1/3
File dev/core/test/com/google/gwt/dev/shell/DispatchClassInfoTest.java
(right):

http://gwt-code-reviews.appspot.com/126817/diff/1/3#newcode30
Line 30: class Foo {
This test should also be updated to test for inherited and possibly
ambiguous method names.

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

[Lex Spoon]

On Wed, Dec 23, 2009 at 9:35 AM, BobV <bo...@google.com> wrote:
> One quick note while I'm looking through the code is that using "??"
> as the wildcard is confusing.  Is that one question-mark for each
> parameter, meaning up to two parameters?  Using a asterisk would be
> more in keeping with regex-esque syntax.

Okay, that's three votes for *, if you count my initial patch. I'll
change it back. -Lex

[sp...@google.com]

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

http://gwt-code-reviews.appspot.com/126817/diff/1/6#newcode109
Line 109: Set<JMethod> almostMatches = new HashSet<JMethod>();

Track methods not strings, because strings are potentially ambiguous.

http://gwt-code-reviews.appspot.com/126817/diff/1/6#newcode109
Line 109: Set<JMethod> almostMatches = new HashSet<JMethod>();

Ah yes, that's why there was a treeset below. I'll change it back to
sorting the matches using a separate TreeSet with just the match names.

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

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

OK.

http://gwt-code-reviews.appspot.com/126817/diff/1/8#newcode1381
Line 1381: reportSyntaxError("msg.jsni.expected.char", new String[] {
")" });

This is the case where one '?' is seen but not a second one.

Anyway, this code will be redone when switching to '*' ....

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

http://gwt-code-reviews.appspot.com/126817/diff/1/7#newcode37
Line 37: * <code>memberById</code>, but there are other instances that
also match.

OK.

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

http://gwt-code-reviews.appspot.com/126817/diff/1/9#newcode174
Line 174: * {@link #matchesAnyOverload()} is false.

OK.

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

http://gwt-code-reviews.appspot.com/126817/diff/1/2#newcode129
Line 129: public void testWildcardLookups() {

Did you mean B to extend A?

I was thinking that the semantics would be to look in the most derived
class that has any method of the specified name. If there's one method
of the requested name, then it can be referenced using a wildcard.

I'll add some tests like that. I believe JsniRefLookup would currently
claim such a reference to be ambiguous, so I'll update it, too.

http://gwt-code-reviews.appspot.com/126817/diff/1/3
File dev/core/test/com/google/gwt/dev/shell/DispatchClassInfoTest.java
(right):

http://gwt-code-reviews.appspot.com/126817/diff/1/3#newcode30
Line 30: class Foo {

Will do.

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