Moves method verification after extensions have had the chance to consume properties. #299

rharter · 2015-12-30T06:01:18Z

Since compile-testing doesn't support checking warnings, I've simply tested for the inverse here, ensuring that invalid properties fail with the appropriate error. This move should also remove the warning for "abstract methods other than property getters and builder converters" for consumed methods.

rharter · 2015-12-30T15:53:15Z

@eamonnmcmanus This is failing the build on OpenJDK in some unrelated tests, with a buffer overflow in libnet.so. Any idea what that's all about?

tbroyer · 2015-12-30T18:49:40Z

@rharter I've seen crashes during tests on Travis CI with OpenJDK 7 too: https://travis-ci.org/tbroyer/gradle-errorprone-plugin/builds/99365020
Switched from the "legacy precise" to the container-based infra and it went back to green: https://travis-ci.org/tbroyer/gradle-errorprone-plugin/builds/99366548

cgruber · 2016-01-10T19:36:23Z

We have some confirmation that this is a bug in the JDK which was fixed in 8 and backported to Oracle's JDK7, but not into OpenJDK7. I'm looking at a different way to fix this, as a different htmlunit runstyle could override things, but that would involve a new custom library just to run gwt tests via htmlunit on OpenJDK7, and given the real-world prevalence of 8, the non-crashiness of Oracle's jdk 7, and the container workaround I think I'm split between pushing for container use on Travis, or just dropping OpenJDK7 from the test rig (or disabling gwt tests on that jvm)

cgruber · 2016-01-10T19:36:40Z

Note - we're having this issue on Truth now too.

cgruber · 2016-01-10T19:37:15Z

@rharter - I believe warnings are in, or about to be in Compile-Testing, and i'll sync it out soon.

rharter · 2016-01-10T21:07:29Z

@cgruber Great, I'll look into adding warning tests once that's done.

cgruber · 2016-01-10T23:21:01Z

Crap... I found that that change (warnings API on compile-testing) is still pending... but "real soon now" (next day or two, just resolving some final review issues).

…sume properties. Fixes google#286

rharter · 2016-02-16T17:23:19Z

@cgruber Did this ever get merged? I don't see anything in the JavaSourcesSubject that would indicate warning validation.

cgruber · 2016-02-17T01:42:06Z

Hmm. I was under the impression that it had, and that we'd synced out more recently than it seems we have... I know it went in internally, but apparently we're behind on syncing out. Let me check on what's going on. :/

cgruber · 2016-02-17T01:49:09Z

Yep. Sorry, it needs a sync, and I'll make sure we get a SNAPSHOT out shortly (this week), and probably a stable minor release soon after.

rharter · 2016-02-17T03:12:01Z

That sounds great. Thanks!

eamonnmcmanus · 2016-02-18T15:32:03Z

@cgruber I don't think this change has been pulled in to the Google version.

rharter · 2016-02-29T17:04:24Z

@cgruber It looks like the changes have been merged into compile testing. Do you currently have a plan for a new maven release? I can't use this it here until you release it.

rharter · 2016-03-07T20:47:48Z

@cgruber Any update on this? ^^

JakeWharton · 2016-03-09T18:14:32Z

@rharter Looks like compile-testing 0.9 is out

rharter · 2016-03-09T19:04:31Z

nice, I'll get this updated.

rharter · 2016-03-10T23:27:50Z

I've tried updating the tests to use the new compilesWithoutWarnings() method from compile-testing, but we actually always compile with at least one warning, No processor claimed any of these annotations: com.google.auto.value.AutoValue. This is apparently intentional as documented in the code. The compilesWithoutWarnings method doesn't allow me to access the actual warnings to filter out known, expected warnings, so I can't filter out that warning and ensure it doesn't contain extra warnings.

I think this means that the current negative test, checking that the compilation fails for unconsumed properties will have to suffice.

eamonnmcmanus · 2016-03-10T23:40:55Z

If you know that there will always be this one warning, could you use withWarningCount(1)?

Alternatively we could have a test-only way of invoking AutoValue that causes it to return true instead of false from the process method.

rharter · 2016-03-11T00:49:00Z

Ahh, didn't realize that that was added too, I'll look into that.

tbroyer · 2016-03-11T03:20:56Z

Is it possible to pass -Xlint:-processing? That would remove that warning.

rharter · 2016-03-11T18:41:57Z

@eamonnmcmanus I believe this is ready to go, now.

menny · 2016-03-28T16:50:03Z

I would LOVE to see this in!
Our project has -Werror compiler flag, and without this PR in, we can not use AutoValue (and auto-value-parcel) in our source code.

eamonnmcmanus · 2016-04-18T21:17:22Z

value/src/test/java/com/google/auto/value/processor/ExtensionTest.java

+        .processedWith(new AutoValueProcessor(ImmutableList.of(new FooExtension())))
+        .compilesWithoutError()
+        // expected warnings: @AutoValue and @Generated annotations not consumed
+        .withWarningCount(2);


I've had success passing .withCompilerOptions("-Xlint:-processing") to turn off the warnings about @AutoValue and @Generated not being consumed by any processor. However, when I do that in this test, it passes without any other changes to the code. I think we need a test that fails before the rest of the modifications in this PR are made and passes afterwards.

I would expect that would pass since you're suppressing all processor warnings. The point of this test is to ensure that those two expected warnings, and no others, are thrown. In the error case, a warning is thrown for every property, but .withCompilerOptions("-Xlint:-processing") would suppress those.

Er, no, -Xlint:-processing is only about the warning wrt annotations that haven't been claimed by a processor (and most processors, including Auto ones, don't claim annotations at all).
https://docs.oracle.com/javase/7/docs/technotes/tools/solaris/javac.html#xlintwarnings

ahh, yes, good point. My misunderstanding.

eamonnmcmanus · 2016-04-18T23:13:51Z

value/src/main/java/com/google/auto/value/extension/AutoValueExtension.java

@@ -18,7 +18,8 @@
 *
 * <p>Extensions can extend the AutoValue implementation by generating subclasses of the AutoValue
 * generated class. It is not guaranteed that an Extension's generated class will be the final
- * class in the inheritance hierarchy, unless its {@link #mustBeFinal(Context)} method returns true.
+ * class in the inheritance hierarchy, unless its
+ * {@link com.google.auto.value.extension.AutoValueExtension#mustBeFinal(Context)} method returns true.


I'm not seeing the purpose of these {@link} rewrites. The previous versions work for me. Is there a context where they don't?

Yeah, I'm not sure why those got in there. Works fine for me, too. I've reverted them.

eamonnmcmanus · 2016-04-19T01:09:26Z

Sorry about the delay in getting to this. I've pulled it into our local repository but before committing it I want to restructure the code somewhat. The AutoValueProcessor.processType method in particular is getting a bit unwieldy. I'd also like to get rid of the warning for abstract methods like writeToParcel(Parcel, int), which implies generalizing the support for consuming properties so that you can also consume other abstract methods.

Rainer-Lang · 2016-05-03T08:50:12Z

Would be good to have this merged.

eamonnmcmanus · 2016-05-03T16:07:26Z

I'm going to close this PR because our processes mean that we incorporate changes internally at Google rather than on github. I've made a chunk of changes to extensions, partly based on this PR, which will appear at our next push.

@rharter

…his involves moving a fair amount of code around, but the resultant organization for method identification should be clearer. The principal purpose of the restructuring is to get rid of warnings about abstract methods when those methods are going to be implemented by an extension. A second purpose is to fix a bug where extensions would not work right if there was a toBuilder() method. Also improve the test coverage in ExtensionTest, which was fairly minimal. Some of the code in this change is based on google#299 by @rharter. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=120485500

googlebot added the cla: yes label Dec 30, 2015

rharter force-pushed the rh/warning branch from 5130f5e to 3da0004 Compare December 30, 2015 15:39

rharter added 2 commits January 19, 2016 09:32

Fixes javadoc reference errors after refactor.

bfcdcde

Moves method verification after extensions have had the chance to con…

8860489

…sume properties. Fixes google#286

Adds warning test to extension test.

55683c3

rharter force-pushed the rh/warning branch from 3da0004 to 55683c3 Compare March 11, 2016 18:37

rharter mentioned this pull request Mar 28, 2016

Warning when compiling Parcelable classes rharter/auto-value-parcel#40

Closed

eamonnmcmanus reviewed Apr 18, 2016
View reviewed changes

Reverts unnecessary javadoc @link overwrites.

b0df868

eamonnmcmanus closed this May 3, 2016

eamonnmcmanus mentioned this pull request May 10, 2016

Merge Google changes as of 2016-05-10 #328

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Moves method verification after extensions have had the chance to consume properties. #299

Moves method verification after extensions have had the chance to consume properties. #299

rharter commented Dec 30, 2015

rharter commented Dec 30, 2015

tbroyer commented Dec 30, 2015

cgruber commented Jan 10, 2016

cgruber commented Jan 10, 2016

cgruber commented Jan 10, 2016

rharter commented Jan 10, 2016

cgruber commented Jan 10, 2016

rharter commented Feb 16, 2016

cgruber commented Feb 17, 2016

cgruber commented Feb 17, 2016

rharter commented Feb 17, 2016

eamonnmcmanus commented Feb 18, 2016

rharter commented Feb 29, 2016

rharter commented Mar 7, 2016

JakeWharton commented Mar 9, 2016

rharter commented Mar 9, 2016

rharter commented Mar 10, 2016

eamonnmcmanus commented Mar 10, 2016

rharter commented Mar 11, 2016

tbroyer commented Mar 11, 2016

rharter commented Mar 11, 2016

menny commented Mar 28, 2016

eamonnmcmanus Apr 18, 2016

rharter May 3, 2016

tbroyer May 3, 2016

rharter May 3, 2016

eamonnmcmanus Apr 18, 2016

rharter May 3, 2016

eamonnmcmanus commented Apr 19, 2016

Rainer-Lang commented May 3, 2016

eamonnmcmanus commented May 3, 2016

Moves method verification after extensions have had the chance to consume properties. #299

Moves method verification after extensions have had the chance to consume properties. #299

Conversation

rharter commented Dec 30, 2015

rharter commented Dec 30, 2015

tbroyer commented Dec 30, 2015

cgruber commented Jan 10, 2016

cgruber commented Jan 10, 2016

cgruber commented Jan 10, 2016

rharter commented Jan 10, 2016

cgruber commented Jan 10, 2016

rharter commented Feb 16, 2016

cgruber commented Feb 17, 2016

cgruber commented Feb 17, 2016

rharter commented Feb 17, 2016

eamonnmcmanus commented Feb 18, 2016

rharter commented Feb 29, 2016

rharter commented Mar 7, 2016

JakeWharton commented Mar 9, 2016

rharter commented Mar 9, 2016

rharter commented Mar 10, 2016

eamonnmcmanus commented Mar 10, 2016

rharter commented Mar 11, 2016

tbroyer commented Mar 11, 2016

rharter commented Mar 11, 2016

menny commented Mar 28, 2016

eamonnmcmanus Apr 18, 2016

Choose a reason for hiding this comment

rharter May 3, 2016

Choose a reason for hiding this comment

tbroyer May 3, 2016

Choose a reason for hiding this comment

rharter May 3, 2016

Choose a reason for hiding this comment

eamonnmcmanus Apr 18, 2016

Choose a reason for hiding this comment

rharter May 3, 2016

Choose a reason for hiding this comment

eamonnmcmanus commented Apr 19, 2016

Rainer-Lang commented May 3, 2016

eamonnmcmanus commented May 3, 2016