Updated patch incorporating Thomas's feedback.
http://gwt-code-reviews.appspot.com/1467804/diff/1/user/build.xml
File user/build.xml (right):
http://gwt-code-reviews.appspot.com/1467804/diff/1/user/build.xml#newcode155
user/build.xml:155: <exclude
name="com/google/web/bindery/requestfactory/apt/**"/>
On 2011/06/23 16:07:24, tbroyer wrote:
Why is this needed? I can understand that it makes iterating on the
APT code
easier (changes you make to it aren't taken into account when
determining
whether to precompile modules –which is slow and resource consuming–,
when
launching, e.g., unit tests) but you probably meant to only have it a
s a
temporary measure?
It's temporary, will revert before submitting.
http://gwt-code-reviews.appspot.com/1467804/diff/1/user/src/com/google/web/bindery/requestfactory/apt/DomainChecker.java
File
user/src/com/google/web/bindery/requestfactory/apt/DomainChecker.java
(right):
http://gwt-code-reviews.appspot.com/1467804/diff/1/user/src/com/google/web/bindery/requestfactory/apt/DomainChecker.java#newcode50
user/src/com/google/web/bindery/requestfactory/apt/DomainChecker.java:50:
private ExecutableElement found;
On 2011/06/23 16:07:24, tbroyer wrote:
shouldn't they all be 'final' ?
Changed lookFor() to a constructor, since instances of a MethoFinder
were never recycled.
http://gwt-code-reviews.appspot.com/1467804/diff/1/user/src/com/google/web/bindery/requestfactory/apt/DomainChecker.java#newcode83
user/src/com/google/web/bindery/requestfactory/apt/DomainChecker.java:83:
TypeMirror paramType = maybeBox(paramElement.asType(), state);
On 2011/06/23 16:07:24, tbroyer wrote:
Correct me if I'm wrong, but this is a change from RFIV, where
argument types
weren't "boxed" before being compared. If this is a willful change,
then
ReflectiveServiceLayer would have to be updated as well when looking
up methods.
Similarly, return type is only boxed in RFIV for service methods, not
properties.
No longer looking for boxed parameters.
http://gwt-code-reviews.appspot.com/1467804/diff/1/user/src/com/google/web/bindery/requestfactory/apt/DomainChecker.java#newcode96
user/src/com/google/web/bindery/requestfactory/apt/DomainChecker.java:96:
|| state.types.isSubsignature((ExecutableType) x.asType(),
(ExecutableType) found
On 2011/06/23 16:07:24, tbroyer wrote:
That's great, as it seems to fix issue 5926. (unfortunately, because
RFIV is
still used, an interface that validates with RfValidator at
compile-time can
fail with RFIV at runtime)
There will be a follow-up patch to this that deletes
RequestFactoryInterfaceValidator (the code's mostly written in another
git branch).
http://gwt-code-reviews.appspot.com/1467804/diff/1/user/src/com/google/web/bindery/requestfactory/apt/DomainChecker.java#newcode111
user/src/com/google/web/bindery/requestfactory/apt/DomainChecker.java:111:
returnType = maybeBox(returnType, state);
On 2011/06/23 16:07:24, tbroyer wrote:
Why isn't this done in the constructor?
Done.
http://gwt-code-reviews.appspot.com/1467804/diff/1/user/src/com/google/web/bindery/requestfactory/apt/DomainChecker.java#newcode115
user/src/com/google/web/bindery/requestfactory/apt/DomainChecker.java:115:
return scanAllInheritedMethods(x, state);
On 2011/06/23 16:07:24, tbroyer wrote:
Isn't that more or less the default behavior? (default behavior would
visit
fields and nested types too)
The default behavior is to scan all enclosed (declared) methods. In this
case, the code needs to consider all inherited methods, since methods
may be mixed in from non-proxy superinterfaces.
http://gwt-code-reviews.appspot.com/1467804/diff/1/user/src/com/google/web/bindery/requestfactory/apt/DomainChecker.java#newcode149
user/src/com/google/web/bindery/requestfactory/apt/DomainChecker.java:149:
TypeMirror returnType = x.getReturnType().accept(new
ClientToDomainMapper(), state);
On 2011/06/23 16:07:24, tbroyer wrote:
x should probably be turned into an ExecutableType using
state.types.asMemberOf(domainType.asType(), x) to make sure we
preserve "actual
type variables" in the hierarchy (and similarly in MethodFinder).
I mean, if we have:
interface BaseFooProxy<T> {
T getT();
void setT(T);
}
and
interface FooProxy extends BaseFooProxy<BarProxy> { }
the ExecutableElement will have a "formal type variable" T and we only
know
about its bounds. What we'd like to check here is that the domain
object has
"BarProxy getT()" and "void setT(BarProxy)" methods, and only
ExecutableType
would give us the information.
There might be other places where that'd be beneficial.
...and that'd help fix issue 5926 in an elegant way:
http://code.google.com/p/google-web-toolkit/issues/detail?id=5926
Done.
http://gwt-code-reviews.appspot.com/1467804/diff/1/user/src/com/google/web/bindery/requestfactory/apt/Finder.java
File user/src/com/google/web/bindery/requestfactory/apt/Finder.java
(right):
http://gwt-code-reviews.appspot.com/1467804/diff/1/user/src/com/google/web/bindery/requestfactory/apt/Finder.java#newcode34
user/src/com/google/web/bindery/requestfactory/apt/Finder.java:34: *
Make this externally-configurable?
On 2011/06/23 16:07:24, tbroyer wrote:
That'd be easy, using ProcessingEnvironment#getOptions.
Removed the blacklist in favor to marking that test type with
@SkipInterfaceValidation. The test will be remove when RFIV is retired
in a subsequent patch.
http://gwt-code-reviews.appspot.com/1467804/diff/1/user/src/com/google/web/bindery/requestfactory/apt/Finder.java#newcode39
user/src/com/google/web/bindery/requestfactory/apt/Finder.java:39:
On 2011/06/23 17:16:15, pquitslund wrote:
It feels a little odd to bake a test reference in here. Is there an
easy way to
work around this? (E.g., a Finder subclass that does the blacklist
filtering?)
Done.
http://gwt-code-reviews.appspot.com/1467804/diff/1/user/src/com/google/web/bindery/requestfactory/apt/Finder.java#newcode46
user/src/com/google/web/bindery/requestfactory/apt/Finder.java:46: //
Ignore anything other than interfaces
On 2011/06/23 16:07:24, tbroyer wrote:
Shouldn't that comment be *above* the 'if' rather than within?
Done.
http://gwt-code-reviews.appspot.com/1467804/diff/1/user/src/com/google/web/bindery/requestfactory/apt/ProxyScanner.java
File
user/src/com/google/web/bindery/requestfactory/apt/ProxyScanner.java
(right):
http://gwt-code-reviews.appspot.com/1467804/diff/1/user/src/com/google/web/bindery/requestfactory/apt/ProxyScanner.java#newcode40
user/src/com/google/web/bindery/requestfactory/apt/ProxyScanner.java:40:
if (x.getSimpleName().contentEquals("stableId") &&
x.getParameters().isEmpty()) {
On 2011/06/23 16:07:24, tbroyer wrote:
How about overriding shouldIgnore and put this check in there?
Done.
http://gwt-code-reviews.appspot.com/1467804/diff/1/user/src/com/google/web/bindery/requestfactory/apt/ProxyScanner.java#newcode51
user/src/com/google/web/bindery/requestfactory/apt/ProxyScanner.java:51:
// Parameters checked by visitVariable
On 2011/06/23 16:07:24, tbroyer wrote:
Should that comment be outside the 'if' ?
Done.
http://gwt-code-reviews.appspot.com/1467804/diff/1/user/src/com/google/web/bindery/requestfactory/apt/ProxyScanner.java#newcode62
user/src/com/google/web/bindery/requestfactory/apt/ProxyScanner.java:62:
state.poison(x, "A proxy must be annotated with %s, %s, or %s",
ProxyFor.class
On 2011/06/23 16:07:24, tbroyer wrote:
Isn't it a breaking change? That is, in 2.3 (and 2.4) I can declare a
"BaseFooProxy extends EntityProxy" without ProxyFor/etc. annotation,
and then
define sub-interfaces with the annotation. With this check, I have to
move the
"extends EntityProxy" to the sub-interfaces (as you did in the unit
test).
I don't really mind, but it'll probably break a few people's code
(starting with
mine)
Otherwise, maybe the check could be moved to places referencing proxy
types,
i.e. places where ProxyScanner is called except RfValidator (and
similarly for
request context's @Service/ServiceName vs. RequestContextScanner).
Moved the check to TransportableTypeVisitor.
http://gwt-code-reviews.appspot.com/1467804/diff/1/user/src/com/google/web/bindery/requestfactory/apt/ProxyScanner.java#newcode68
user/src/com/google/web/bindery/requestfactory/apt/ProxyScanner.java:68:
// See javadoc on Element.getAnnotation() for why it works this way
On 2011/06/23 16:07:24, tbroyer wrote:
Just wondering, why isn't State#checkExtraTypes using this method? or
why isn't
this code using the same algorithm as State#checkExtraTypes? (in other
words:
why two ways of doing the same thing, in the same codebase?)
Changed State.checkExtraTypes() to use this pattern and correctly check
super-interfaces.
http://gwt-code-reviews.appspot.com/1467804/diff/1/user/src/com/google/web/bindery/requestfactory/apt/RequestContextScanner.java
File
user/src/com/google/web/bindery/requestfactory/apt/RequestContextScanner.java
(right):
http://gwt-code-reviews.appspot.com/1467804/diff/1/user/src/com/google/web/bindery/requestfactory/apt/RequestContextScanner.java#newcode71
user/src/com/google/web/bindery/requestfactory/apt/RequestContextScanner.java:71:
} else if (isSetter(x, state)) {
On 2011/06/23 16:07:24, tbroyer wrote:
Setters in a RequestContext? Did I miss some earlier change?
They're allowed in RequestContext subtypes when used with JSON-RPC apis
for optional parameters.
http://gwt-code-reviews.appspot.com/1467804/diff/1/user/src/com/google/web/bindery/requestfactory/apt/RfApt.java
File user/src/com/google/web/bindery/requestfactory/apt/RfApt.java
(right):
http://gwt-code-reviews.appspot.com/1467804/diff/1/user/src/com/google/web/bindery/requestfactory/apt/RfApt.java#newcode149
user/src/com/google/web/bindery/requestfactory/apt/RfApt.java:149: +
BinaryName.toSourceName(entry.getValue()) + ".class.getName());");
On 2011/06/23 16:07:24, tbroyer wrote:
Now there's another issue when applied to the gwt-user unit tests:
many test
cases declare non-public inner interfaces (generally
package-protected) and the
generated build cannot compile due to type visibility issues.
Looks like Class.forName is the only real fix. Maybe that should be
done in
TypeTokenResolver.Builder#addTypeToken (so it'll also be applied to
tokens
loaded from properties files), and this code simply generate string
literals?
Done.
http://gwt-code-reviews.appspot.com/1467804/diff/1/user/src/com/google/web/bindery/requestfactory/apt/ScannerBase.java
File user/src/com/google/web/bindery/requestfactory/apt/ScannerBase.java
(right):
http://gwt-code-reviews.appspot.com/1467804/diff/1/user/src/com/google/web/bindery/requestfactory/apt/ScannerBase.java#newcode116
user/src/com/google/web/bindery/requestfactory/apt/ScannerBase.java:116:
protected void poisonIfRedundant(State state, TypeElement x,
Annotation... annotations) {
On 2011/06/23 16:07:24, tbroyer wrote:
That method deserves a javadoc! I thought it was misnamed as it wasn't
checking
much things, but reading ProxyScanner I found that one annotation has
already be
"asserted" and the passed annotations are other possible annotations
on the same
element (a type, generally), that have been "resolved" and should then
be 'null'
(otherwise, it means they're present, thus redundant).
The javadoc may be just some sample code showing how it's supposed to
be used.
Renamed to poisonIfAnnotationPresent() and javadoc'ed.
http://gwt-code-reviews.appspot.com/1467804/diff/1/user/src/com/google/web/bindery/requestfactory/apt/State.java
File user/src/com/google/web/bindery/requestfactory/apt/State.java
(right):
http://gwt-code-reviews.appspot.com/1467804/diff/1/user/src/com/google/web/bindery/requestfactory/apt/State.java#newcode161
user/src/com/google/web/bindery/requestfactory/apt/State.java:161:
return clientToDomainMain;
On 2011/06/23 16:07:24, tbroyer wrote:
Collections.unmodifiableMap() ? (as a safeguard)
Done.
http://gwt-code-reviews.appspot.com/1467804/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
