Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Moe Sync #1117

Closed
wants to merge 26 commits into from
Closed

Moe Sync #1117

wants to merge 26 commits into from

Conversation

ronshapiro
Copy link
Contributor

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:

Stop passing unused VisitorState to SuppressionHelper.

RELNOTES: None

84f3bbc


Ignore super(...) invocations in MustBeClosedChecker

RELNOTES: Ignore super(...) invocations in MustBeClosedChecker

75b0055


OrphanedFormatString: Only consider StringBuilder.append a likely error if the append(CharSequence, int, int) or append(char[], int, int) overloads are invoked.

RELNOTES: OrphanedFormatString: only consider StringBuilder.append a likely error for some overloads (#1021)

233651e


Fix flagging serialization methods in Unused.

Extract the test so it's a bit clearer.

RELNOTES: N/A

ccdc625


Make ant example's path handling work on Windows

3f2ca43#commitcomment-30383244

RELNOTES: N/A

2e89884


Remove ProtoFieldPreconditionsCheckNotNull, as it is subsumed into ProtoFieldNullComparison.

RELNOTES: N/A

45cc93d


Fix treatment of non-trivial receivers of method selects in Unused.

RELNOTES: Fix a false positive in Unused.

c6c010f


Provide a marginally better description in BadInstanceof.

It seems slightly friendlier to also include the actual type of the expression to make the issue a bit clearer.

RELNOTES: N/A

65fe799


Provide a different description when removing the `default` case in UnnecessaryDefaultInEnumSwitch.

RELNOTES: N/A

5b094ec


Remove altName of BadNestedImport from BadImport checker.

There is no need to continue to support both names.

RELNOTES: Remove altName of BadNestedImport from BadImport checker.

135c920


Add support for suppression of FieldCanBeFinal

Fixes #1038

RELNOTES: N/A

e5eb00a


Add a bug pattern to discourage writing #equals methods by just calling hashCode.

I'm kinda surprised this is a thing. We only match where the hashCode is the last thing compared within a `return, as I think this is a fairly sane pattern, especially with memoized hashCodes:

return hashCode() == o.hashCode() && ;

RELNOTES: [EqualsUsingHashCode] add a bug pattern to discourage writing equals using hashCode.

4fc4181


Add a check for obviously inconsistent hashCode implementations.

This will have plenty of false negatives for classes which compare fields via complex getters, but should work fairly well for simple data classes.

Fixes #35

RELNOTES: [InconsistentHashCode] Add a check to flag inconsistent implementations of hashCode.

e8cf0b5


Make BooleanParameter match constructors too.

And add a couple of fairly common terms to the list of parameter names to ignore.

RELNOTES: Make BooleanParameter match constructors.

36642fa


In BooleanParameter, don't refactor non-boolean parameters, e.g. generic parameters or Object.

RELNOTES: BooleanParameter now skips non-boolean parameters, e.g. generics.

18c54a0


Revise docs to try to make them clearer.

4667101


Fix ParcelableCreator check for raw creator

RELNOTES: Fix ParcelableCreator check for raw creator

f8d8e93


Create namedAnyOf(String... varargs) convenience MethodMatchers method.

RELNOTES: N/A

3926cde


Show more Error Prone diagnostics at once

RELNOTES: Report errors for all files with Error Prone diagnostics, not just the first one.

435c6ba


Make TypeParameterShadowing rename Javadoc references to type parameters too.

RELNOTES: Make TypeParameterShadowing fix Javadocs too.

93b2a7e


Make ParameterComment match non-parameter-comments before arguments too.

Previously it wouldn't match the non-standard form foo(/* bar */ baz).

RELNOTES: Make ParameterComment match before comments too.

09b08e9


Make insertion coalescing in Replacements#add explicit

Instead of silently concatenating overlapping insertions together, require
callers of Replacements#add to explicitly requires a policy for concatenating
them (either existing-first, or new-first). The default behaviour now throws
an exception for overlapping insertions.

RELNOTES: N/A

f6d7c53


Call out CharSequence in UndefinedEquals docs RELNOTES: N/A

958c0df


Fix breakage caused by f6d7c53

f9c9e41


Add a qualifyType overload which takes a String of the type name.

A common pattern is qualifyType(..., state.getTypeFromString(typeName)) which stands a fair chance of NPEing if typeName isn't available.

RELNOTES: Add String-taking qualifyType overload.

803cf50


Exempt constructors for classes containing the term "boolean" (aimed at AtomicBoolean).

These are an obvious exception to my claim that "constructors accepting a single boolean are rarely self documenting".

(Also include 'defaultValue'.)

RELNOTES: N/A

ebe4d09

awturner and others added 26 commits September 12, 2018 15:40
RELNOTES: None

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=211489937
RELNOTES: Ignore super(...) invocations in MustBeClosedChecker

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=211490289
…or if the append(CharSequence, int, int) or append(char[], int, int) overloads are invoked.

RELNOTES: OrphanedFormatString: only consider StringBuilder.append a likely error for some overloads (#1021)

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=211491078
Extract the test so it's a bit clearer.

RELNOTES: N/A

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=211495096
3f2ca43#commitcomment-30383244

RELNOTES: N/A

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=211572444
…otoFieldNullComparison.

RELNOTES: N/A

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=211590847
RELNOTES: Fix a false positive in Unused.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=211768635
It seems slightly friendlier to also include the actual type of the expression to make the issue a bit clearer.

RELNOTES: N/A

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=211778737
…nnecessaryDefaultInEnumSwitch.

RELNOTES: N/A

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=211780381
There is no need to continue to support both names.

RELNOTES: Remove altName of BadNestedImport from BadImport checker.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=211803328
Fixes #1038

RELNOTES: N/A

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=211867098
…ng hashCode.

I'm kinda surprised this is a thing. We only match where the hashCode is the last thing compared within a `return, as I think this is a fairly sane pattern, especially with memoized hashCodes:

  return hashCode() == o.hashCode() && <expensive comparison>;

RELNOTES: [EqualsUsingHashCode] add a bug pattern to discourage writing equals using hashCode.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=211944934
This will have plenty of false negatives for classes which compare fields via complex getters, but should work fairly well for simple data classes.

Fixes #35

RELNOTES: [InconsistentHashCode] Add a check to flag inconsistent implementations of hashCode.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=211950941
And add a couple of fairly common terms to the list of parameter names to ignore.

RELNOTES: Make BooleanParameter match constructors.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=211982804
…ric parameters or Object.

RELNOTES: BooleanParameter now skips non-boolean parameters, e.g. generics.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=212035958
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=212059300
RELNOTES: Fix ParcelableCreator check for raw creator

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=212136660
RELNOTES: N/A

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=212299476
RELNOTES: Report errors for all files with Error Prone diagnostics, not just the first one.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=212341750
…ers too.

RELNOTES: Make TypeParameterShadowing fix Javadocs too.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=212435931
Previously it wouldn't match the non-standard form `foo(/* bar */ baz)`.

RELNOTES: Make ParameterComment match before comments too.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=212441506
Instead of silently concatenating overlapping insertions together, require
callers of Replacements#add to explicitly requires a policy for concatenating
them (either existing-first, or new-first). The default behaviour now throws
an exception for overlapping insertions.

RELNOTES: N/A

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=212536627
RELNOTES: N/A

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=212574017
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=212579964
A common pattern is qualifyType(..., state.getTypeFromString(typeName)) which stands a fair chance of NPEing if typeName isn't available.

RELNOTES: Add String-taking qualifyType overload.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=212593761
…at AtomicBoolean).

These are an obvious exception to my claim that "constructors accepting a single boolean are rarely self documenting".

(Also include 'defaultValue'.)

RELNOTES: N/A

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=212650040
@googlebot
Copy link
Collaborator

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

@googlebot
Copy link
Collaborator

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

@eaftan
Copy link
Contributor

eaftan commented Sep 13, 2018

It looks like commit 435c6ba broke the JDK 11 build. @cushon can you PTAL?

cushon added a commit that referenced this pull request Sep 13, 2018
MOE_MIGRATED_REVID=212738899
@cushon cushon closed this Sep 13, 2018
@ronshapiro ronshapiro deleted the sync-master-2018/09/12 branch December 24, 2018 04:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Check that equals() and hashCode() read the same fields
9 participants