Moe Sync #1294

ronshapiro · 2019-06-12T03:03:15Z

This code has been reviewed and submitted internally. Feel free to discuss on the PR and we can submit follow-up changes as necessary.

Commits:

Replace google.github.io/dagger with dagger.dev

bb00ae1

Flag instances of Duration.of(long, ChronoUnit) where the ChronoUnit is not an exact duration.

#badtime #goodtime

RELNOTES: Flag instances of Duration.of(long, ChronoUnit) where the ChronoUnit is not an exact duration.

221fde7

Fix an incorrect UnnecessaryAnonymousClass refactoring

don't refactor anonymous classes that implement the FI via a superclass.

0faf698

Fix NPE on JDK13+ when InconsistentHashCode visits a break statement with a label.

Fixes #1287

RELNOTES: n/a

7bb2a77

Expand SelfAssignment to cover the Precondition-style APIs in c.g.c.time.Durations,c.g.protobuf.util.{Durations,Timestamps}.

5ddc53f

Allow ChronoUnit.DAYS explicitly in Duration.of(long, TemporalUnit).

(It's explicitly called out in the javadocs, but I missed it the first read.)

Also turn off the checker for desugar'ed java.time.

254f3f2

Rename DurationOfLongTemporalUnit to DurationTemporalUnit and cover duration.plus(long, TemporalUnit) and duration.minus(long, TemporalUnit).

Also ban calls to Instant APIs where the TemporalUnit is not supported.

beef3d8

Provide an example of using proto3's primitive wrappers.

e2cb9fe

Migrate org.mockito.Matchers#any* to org.mockito.ArgumentMatchers

The former is deprecated and replaced by the latter in Mockito 2. However, there is a
functional difference: ArgumentMatchers will reject null and check the type
if the matcher specified a type (e.g. any(Class) or anyInt()). any() will
remain to accept anything.

c875077

Optimize VisitorState.inferBinaryName() by iterating directly over the chars of the class name.

This saves the need to create temporary strings for each piece of a package/each simple name of a type, uses StringBuilder.append(char) instead of append(String), and avoids some of the Iterable/Iterator/CharMatcher overhead. It also lends itself to easily checking if any replacement is necessary, which is one of the biggest savings this change brings.

RELNOTES: Performance improvements

8e131a2

Replace google.github.io/dagger with dagger.dev

fc36200

Cache references to "JavacSingletonType.instance(context)" references within ErrorProne. Expose them as methods on VisitorState and only request them once

On their own, these seem insignificant, but in aggregate they're timing is noticeable.

266ffcc

Update docs for the Truth migration from an `actual()` method to an `actual` field.

9dc8eaf

Use the latest checker framework dataflow version

567b765

Remove the custom link from UnusedException.

This already had documentation, which I guess wasn't being linked to. Winning.

ced971d

Add valueOf to list of BadImport static imports

#1284

21599b7

Make LocalDate.plus/minus(Duration) a compile-time error. #badtime #goodtime

d013d6a

Add Class to list of bad imports.

We have nested protobuf classes that mask java.lang.Class.

64bf016

Update to Truth 0.45, and address deprecations.

Renames may include:

containsAllOf => containsAtLeast
containsAllIn => containsAtLeastElementsIn
isSameAs => isSameInstanceAs
isOrdered => isInOrder
isStrictlyOrdered => isInStrictOrder

The other major change is to change custom subjects to extend raw Subject instead of supplying type parameters. The type parameters are being removed from Subject. This CL will temporarily produce rawtypes warnings, which will go away when I remove the type parameters (as soon as this batch of CLs is submitted).

Some CLs in this batch also migrate calls away from actualAsString(). Its literal replacement is "<" + actual + ">" (unless an object overrides actualCustomStringRepresentation()), but usually I've made a larger change, such as switching from an old-style "Not true that..." failure message to one generated with the Fact API. In that case, the new code usually contains a direct reference to this.actual (a field that I occasionally had to create). Another larger change I sometimes made is to switch from a manual check-and-fail approach to instead use check(...). And sometimes I just remove a withMessage() call that's no longer necessary now that the code uses check(...), or I introduce a check(...) call. (An assertion made with check(...) automatically includes the actual value from the original subject, so there's no need to set it again with withMessage().)

Finally, there's one CL in this batch in which I migrate a Correspondence subclass to instead use Correspondence.from.

1a8c868

Remove obsolete todo

3cdc1fd

Recommend Period in the LocalDateTemporalAmount summary.

d1aad72

Replace some for loops with stream operatoins

It reads more nicely without the builders, and I don't think it's in some performance-sensitive area where we should worry about the slight cost of a stream.

eae7d4d

Cleanups in CheckReturnValue

Anonymous class to lambda
Guava Optional to j.u.Optional
- Replace ifPresent/get pairs with orElseGet chain
Fix spelling mistakes in comments
Remove an UnnecessaryLambda(TM)
Remove some unused parameters

a716544

Move @CheckReturnValue to the class level

c0e3539

Clean up EmptyIfStatement

only get the parent once, remove an unnecessary assert.

205ad1a

Don't repeat a bunch of work in MethodMatcher usage

bd513dd

Remove an unused parameter

b530fca

Add the TemporalAmount implementations from org.threeten.extra to the list of well known immutable types.

All of them clearly document immutability in their contract, e.g.: https://www.threeten.org/threeten-extra/apidocs/org.threeten.extra/org/threeten/extra/Days.html

RELNOTES: N/A

fc972db

Fix open-source breakages caused by my recent update to Truth 0.45:

Update ImplementAssertionUsingChaining to operate on the new failWithActual and failWithoutActual methods instead of hardcoding the legacy methods. (The legacy methods have been removed externally.)
Update TruthSelfEquals to check isSameInstanceAs instead of isSameAs. (isSameAs has been removed externally.)
Update UseCorrectAssertInTests to generate isSameInstanceAs instead of isSameAs. (isSameAs has been removed externally.)
Upgrade Compile-Testing to 0.18. (0.16 isn't compatible with the new Truth.)

a63e46f

Make Matchers.parentNode type-safer

47dfcb4

Add a TODO to revisit nextStatement's handling of the last statement in a block

e3766e4

Make methodName a String

contentEquals() will do that anyway, so this saves us 3 String creations.

Alternatively, we could use Names to cache the names and then use equals() on those. I'm open to that if we'd like.

163a2b7

Discourage calling #toString on lite protos.

Ignore cases which are clearly being called within a debug/verbose log method, or in testonly code.

4ed49ef

Add a check for calling checkNotNull twice on the same variable.

0fbefb0

Migrate AbstractToString onto java.util.Optional

7052474

Fix typo in UnsafeFinalization.md

231d8b6

Turn off ErrorProne for large code generators.

Currently only protoc and Dagger-generated files are supported. Security checks (@BugPattern(disableable = false)) are still invoked to make sure the generators don't generate any dangerous code.

RELNOTES: N/A

03f19c9

------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=250277687

…is not an exact duration. #badtime #goodtime RELNOTES: Flag instances of Duration.of(long, ChronoUnit) where the ChronoUnit is not an exact duration. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=250339938

don't refactor anonymous classes that implement the FI via a superclass. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=250349768

…with a label. Fixes #1287 RELNOTES: n/a ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=250371701

…ime.Durations,c.g.protobuf.util.{Durations,Timestamps}. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=250394166

(It's explicitly called out in the javadocs, but I missed it the first read.) Also turn off the checker for desugar'ed java.time. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=250398318

…uration.plus(long, TemporalUnit) and duration.minus(long, TemporalUnit). Also ban calls to Instant APIs where the TemporalUnit is not supported. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=250547375

------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=250656647

The former is deprecated and replaced by the latter in Mockito 2. However, there is a functional difference: ArgumentMatchers will reject `null` and check the type if the matcher specified a type (e.g. `any(Class)` or `anyInt()`). `any()` will remain to accept anything. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=250661666

…e chars of the class name. This saves the need to create temporary strings for each piece of a package/each simple name of a type, uses StringBuilder.append(char) instead of append(String), and avoids some of the Iterable/Iterator/CharMatcher overhead. It also lends itself to easily checking if any replacement is necessary, which is one of the biggest savings this change brings. RELNOTES: Performance improvements ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=250691079

------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=250901613

… within ErrorProne. Expose them as methods on VisitorState and only request them once On their own, these seem insignificant, but in aggregate they're timing is noticeable. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=250954754

…actual` field. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=250982073

------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=251170093

This already had documentation, which I guess wasn't being linked to. Winning. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=251256625

#1284 ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=251294027

#badtime #goodtime ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=251474206

We have nested protobuf classes that mask java.lang.Class. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=251482745

Renames may include: - containsAllOf => containsAtLeast - containsAllIn => containsAtLeastElementsIn - isSameAs => isSameInstanceAs - isOrdered => isInOrder - isStrictlyOrdered => isInStrictOrder The other major change is to change custom subjects to extend raw Subject instead of supplying type parameters. The type parameters are being removed from Subject. This CL will temporarily produce rawtypes warnings, which will go away when I remove the type parameters (as soon as this batch of CLs is submitted). Some CLs in this batch also migrate calls away from actualAsString(). Its literal replacement is `"<" + actual + ">"` (unless an object overrides actualCustomStringRepresentation()), but usually I've made a larger change, such as switching from an old-style "Not true that..." failure message to one generated with the Fact API. In that case, the new code usually contains a direct reference to this.actual (a field that I occasionally had to create). Another larger change I sometimes made is to switch from a manual check-and-fail approach to instead use check(...). And sometimes I just remove a withMessage() call that's no longer necessary now that the code uses check(...), or I introduce a check(...) call. (An assertion made with check(...) automatically includes the actual value from the original subject, so there's no need to set it again with withMessage().) Finally, there's one CL in this batch in which I migrate a Correspondence subclass to instead use Correspondence.from. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=251498323

------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=251504805

------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=251691890

It reads more nicely without the builders, and I don't think it's in some performance-sensitive area where we should worry about the slight cost of a stream. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=251718707

- Anonymous class to lambda - Guava Optional to j.u.Optional - Replace ifPresent/get pairs with orElseGet chain - Fix spelling mistakes in comments - Remove an UnnecessaryLambda(TM) - Remove some unused parameters ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=251752031

------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=251756849

only get the parent once, remove an unnecessary assert. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=251875666

------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=251875973

------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=251911580

… list of well known immutable types. All of them clearly document immutability in their contract, e.g.: https://www.threeten.org/threeten-extra/apidocs/org.threeten.extra/org/threeten/extra/Days.html RELNOTES: N/A ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=251912949

- Update ImplementAssertionUsingChaining to operate on the new failWithActual and failWithoutActual methods instead of hardcoding the legacy methods. (The legacy methods have been removed externally.) - Update TruthSelfEquals to check isSameInstanceAs instead of isSameAs. (isSameAs has been removed externally.) - Update UseCorrectAssertInTests to generate isSameInstanceAs instead of isSameAs. (isSameAs has been removed externally.) - Upgrade Compile-Testing to 0.18. (0.16 isn't compatible with the new Truth.) ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=251934666

------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=251952914

…in a block ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=252064017

contentEquals() will do that anyway, so this saves us 3 String creations. Alternatively, we could use Names to cache the names and then use equals() on those. I'm open to that if we'd like. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=252093952

Ignore cases which are clearly being called within a debug/verbose log method, or in testonly code. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=252367708

------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=252570059

------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=252572232

------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=252673610

Currently only protoc and Dagger-generated files are supported. Security checks (@BugPattern(disableable = false)) are still invoked to make sure the generators don't generate any dangerous code. RELNOTES: N/A ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=252688417

check_api/src/main/java/com/google/errorprone/ErrorProneOptions.java

Stephan202 · 2019-06-12T07:15:56Z

core/src/main/java/com/google/errorprone/bugpatterns/time/DurationTemporalUnit.java

+
+/**
+ * Bans calls to {@code Duration} APIs where the {@link java.time.temporal.TemporalUnit} is not
+ * {@link ChronoUnit#DAYS} or it has an estimated duration (which is guaranteed to throw an {@code


Suggested change

* {@link ChronoUnit#DAYS} or it has an estimated duration (which is guaranteed to throw an {@code

* {@link ChronoUnit#DAYS} and has an estimated duration (which is guaranteed to throw an {@code

Repeated this comment just so it doesn't get lost following closure of #1291.

jbduncan · 2019-06-12T17:03:01Z

docs/bugpattern/ChainedAssertionLosesContext.md

@@ -6,7 +6,7 @@ Before:
 ```
 class MyProtoSubject {
  public void hasFoo(Foo expected) {
-    assertThat(actual().foo()).isEqualTo(expected);
+    assertThat(actual.foo()).isEqualTo(expected);


AFAICT, MyProtoSubject lacks an actual variable or field, so I wonder if it should be updated? 🤔

The class is missing a constructor and lots of other things, too. I don't want there to be too much noise around the actual bug, but I should probably insert a "..." in there to make that clear. Hmm, and extends Subject, too. Will do. thanks

jbduncan · 2019-06-12T17:04:40Z

docs/bugpattern/ChainedAssertionLosesContext.md

@@ -16,7 +16,7 @@ After:
 ```
 class MyProtoSubject {
  public void hasFoo(Foo expected) {
-    check("foo()").that(actual().foo()).isEqualTo(expected);
+    check("foo()").that(actual.foo()).isEqualTo(expected);


This example lacks an actual variable or field, so I wonder if it should be updated?

ronshapiro · 2019-06-12T19:58:19Z

@Stephan202 sorry for resolving your comment, I meant to reply. So here's my response:

We are currently evaluating that flag in an experiment. It's possible that we'll decide to opensource that logic after we have a better handle on it and if it's the right approach to what we want to be doing.

The reason that some of the methods were opensourced is primarily because it was expansive enough that it was a lot to try to strip it all, so I figured that it stripping the ability to set it was sufficient for the time being

Stephan202 · 2019-06-12T20:01:11Z

@ronshapiro clear, thanks!

in response to: #1294 (comment) ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=253066609

ronshapiro and others added 30 commits June 11, 2019 23:01

Replace google.github.io/dagger with dagger.dev

bb00ae1

------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=250277687

Fix an incorrect UnnecessaryAnonymousClass refactoring

0faf698

don't refactor anonymous classes that implement the FI via a superclass. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=250349768

Fix NPE on JDK13+ when InconsistentHashCode visits a break statement …

7bb2a77

…with a label. Fixes #1287 RELNOTES: n/a ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=250371701

Expand SelfAssignment to cover the Precondition-style APIs in c.g.c.t…

5ddc53f

…ime.Durations,c.g.protobuf.util.{Durations,Timestamps}. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=250394166

Provide an example of using proto3's primitive wrappers.

e2cb9fe

------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=250656647

Replace google.github.io/dagger with dagger.dev

fc36200

------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=250901613

Update docs for the Truth migration from an actual() method to an `…

9dc8eaf

…actual` field. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=250982073

Use the latest checker framework dataflow version

567b765

------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=251170093

Remove the custom link from UnusedException.

ced971d

This already had documentation, which I guess wasn't being linked to. Winning. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=251256625

Add valueOf to list of BadImport static imports

21599b7

#1284 ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=251294027

Make LocalDate.plus/minus(Duration) a compile-time error.

d013d6a

#badtime #goodtime ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=251474206

Add Class to list of bad imports.

64bf016

We have nested protobuf classes that mask java.lang.Class. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=251482745

Remove obsolete todo

3cdc1fd

------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=251504805

Recommend Period in the LocalDateTemporalAmount summary.

d1aad72

------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=251691890

Move @CheckReturnValue to the class level

c0e3539

------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=251756849

Clean up EmptyIfStatement

205ad1a

only get the parent once, remove an unnecessary assert. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=251875666

Don't repeat a bunch of work in MethodMatcher usage

bd513dd

------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=251875973

Remove an unused parameter

b530fca

------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=251911580

Make Matchers.parentNode type-safer

47dfcb4

------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=251952914

cushon and others added 7 commits June 11, 2019 23:02

Add a TODO to revisit nextStatement's handling of the last statement …

e3766e4

…in a block ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=252064017

Discourage calling #toString on lite protos.

4ed49ef

Ignore cases which are clearly being called within a debug/verbose log method, or in testonly code. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=252367708

Add a check for calling checkNotNull twice on the same variable.

0fbefb0

------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=252570059

Migrate AbstractToString onto java.util.Optional

7052474

------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=252572232

Fix typo in UnsafeFinalization.md

231d8b6

------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=252673610

ronshapiro added the internal-sync label Jun 12, 2019

googlebot added the cla: yes label Jun 12, 2019

Stephan202 reviewed Jun 12, 2019

View reviewed changes

jbduncan reviewed Jun 12, 2019

View reviewed changes

ronshapiro merged commit 88cf5c6 into master Jun 13, 2019

ronshapiro deleted the sync-master-2019/06/11 branch June 13, 2019 21:40

ronshapiro mentioned this pull request Jun 18, 2019

Moe Sync #1296

Closed

ronshapiro pushed a commit that referenced this pull request Jun 18, 2019

Make example code look more like a real Subject.

a4e476f

in response to: #1294 (comment) ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=253066609

ronshapiro mentioned this pull request Jun 20, 2019

Moe Sync #1299

Merged

ronshapiro pushed a commit that referenced this pull request Jun 20, 2019

Make example code look more like a real Subject.

7dbad8c

in response to: #1294 (comment) ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=253066609

ronshapiro pushed a commit that referenced this pull request Jun 21, 2019

Make example code look more like a real Subject.

af3d806

in response to: #1294 (comment) ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=253066609

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Moe Sync #1294

Moe Sync #1294

ronshapiro commented Jun 12, 2019

Stephan202 Jun 12, 2019

jbduncan Jun 12, 2019

cpovirk Jun 13, 2019

jbduncan Jun 12, 2019

cpovirk Jun 13, 2019

ronshapiro commented Jun 12, 2019

Stephan202 commented Jun 12, 2019

	* {@link ChronoUnit#DAYS} or it has an estimated duration (which is guaranteed to throw an {@code
	* {@link ChronoUnit#DAYS} and has an estimated duration (which is guaranteed to throw an {@code

Moe Sync #1294

Moe Sync #1294

Conversation

ronshapiro commented Jun 12, 2019

Commits:

Stephan202 Jun 12, 2019

Choose a reason for hiding this comment

jbduncan Jun 12, 2019

Choose a reason for hiding this comment

cpovirk Jun 13, 2019

Choose a reason for hiding this comment

jbduncan Jun 12, 2019

Choose a reason for hiding this comment

cpovirk Jun 13, 2019

Choose a reason for hiding this comment

ronshapiro commented Jun 12, 2019

Stephan202 commented Jun 12, 2019