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

Replace occurrences of which with that in defining clauses #259

Merged
merged 4 commits into from
Sep 25, 2022
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ Some pointers:
- Checks should be _topical_: ideally they address a single concern.
- Where possible checks should provide _fixes_, and ideally these are
completely behavior-preserving. In order for a check to be adopted by users
it must not "get in the way". So for a check which addresses a relatively
it must not "get in the way". So for a check that addresses a relatively
trivial stylistic concern it is doubly important that the violations it
detects can be auto-patched.
- Make sure you have read Error Prone's [criteria for new
Expand Down
98 changes: 49 additions & 49 deletions error-prone-contrib/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -64,129 +64,129 @@ The following is a list of checks we'd like to see implemented:
functionality.
- A subset of the refactor operations provided by the Eclipse-specific
[AutoRefactor][autorefactor] plugin.
- A check which replaces fully qualified types with simple types in contexts
- A check that replaces fully qualified types with simple types in contexts
where this does not introduce ambiguity. Should consider both actual Java
code and Javadoc `@link` references.
- A check which simplifies array expressions. It would replace empty array
- A check that simplifies array expressions. It would replace empty array
expressions of the form `new int[] {}` with `new int[0]`. Statements of the
form `byte[] arr = new byte[] {'c'};` would be shortened to
`byte[] arr = {'c'};`.
- A check which replaces expressions of the form
- A check that replaces expressions of the form
`String.format("some prefix %s", arg)` with `"some prefix " + arg`, and
similar for simple suffixes. Can perhaps be generalized further, though it's
unclear how far. (Well, a `String.format` call without arguments can
certainly be simplified, too.)
- A check which replaces single-character strings with `char`s where possible.
- A check that replaces single-character strings with `char`s where possible.
For example as argument to `StringBuilder.append` and in string
concatenations.
- A check which adds or removes the first `Locale` argument to `String.format`
- A check that adds or removes the first `Locale` argument to `String.format`
and similar calls as necessary. (For example, a format string containing only
`%s` placeholders is locale-insensitive unless any of the arguments is a
`Formattable`, while `%f` placeholders _are_ locale-sensitive.)
- A check which replaces `String.replaceAll` with `String.replace` if the first
- A check that replaces `String.replaceAll` with `String.replace` if the first
argument is certainly not a regular expression. And if both arguments are
single-character strings then the `(char, char)` overload can be invoked
instead.
- A check which flags (and ideally, replaces) `try-finally` constructs with
- A check that flags (and ideally, replaces) `try-finally` constructs with
equivalent `try-with-resources` constructs.
- A check which drops exceptions declared in `throws` clauses if they are (a)
- A check that drops exceptions declared in `throws` clauses if they are (a)
not actually thrown and (b) the associated method cannot be overridden.
- A check which tries to statically import certain methods whenever used, if
- A check that tries to statically import certain methods whenever used, if
possible. The set of targeted methods should be configurable, but may default
to e.g. `java.util.Function.identity()`, the static methods exposed by
`java.util.stream.Collectors` and the various Guava collector factory
methods.
- A check which replaces `new Random().someMethod()` calls with
- A check that replaces `new Random().someMethod()` calls with
`ThreadLocalRandom.current().someMethod()` calls, to avoid unnecessary
synchronization.
- A check which drops `this.` from `this.someMethod()` calls and which
- A check that drops `this.` from `this.someMethod()` calls and which
optionally does the same for fields, if no ambiguity arises.
- A check which replaces `Integer.valueOf` calls with `Integer.parseInt` or
- A check that replaces `Integer.valueOf` calls with `Integer.parseInt` or
vice versa in order to prevent auto (un)boxing, and likewise for other number
types.
- A check which flags nullable collections.
- A check which flags `AutoCloseable` resources not managed by a
- A check that flags nullable collections.
- A check that flags `AutoCloseable` resources not managed by a
`try-with-resources` construct. Certain subtypes, such as jOOQ's `DSLContext`
should be excluded.
- A check which flags `java.time` methods which implicitly consult the system
- A check that flags `java.time` methods which implicitly consult the system
clock, suggesting that a passed-in `Clock` is used instead.
- A check which flags public methods on public classes which reference
- A check that flags public methods on public classes which reference
non-public types. This can cause `IllegalAccessError`s and
`BootstrapMethodError`s at runtime.
- A check which swaps the LHS and RHS in expressions of the form
- A check that swaps the LHS and RHS in expressions of the form
`nonConstant.equals(someNonNullConstant)`.
- A check which annotates methods which only throw an exception with
- A check that annotates methods which only throw an exception with
`@Deprecated` or ` @DoNotCall`.
- A check which flags imports from other test classes.
- A Guava-specific check which replaces `Joiner.join` calls with `String.join`
- A check that flags imports from other test classes.
- A Guava-specific check that replaces `Joiner.join` calls with `String.join`
calls in those cases where the latter is a proper substitute for the former.
- A Guava-specific check which flags `{Immutable,}Multimap` type usages where
- A Guava-specific check that flags `{Immutable,}Multimap` type usages where
`{Immutable,}{List,Set}Multimap` would be more appropriate.
- A Guava-specific check which rewrites
- A Guava-specific check that rewrites
`if (conditional) { throw new IllegalArgumentException(); }` and variants to
an equivalent `checkArgument` statement. Idem for other exception types.
- A Guava-specific check which replaces simple anonymous `CacheLoader` subclass
- A Guava-specific check that replaces simple anonymous `CacheLoader` subclass
declarations with `CacheLoader.from(someLambda)`.
- A Spring-specific check which enforces that methods with the `@Scheduled`
- A Spring-specific check that enforces that methods with the `@Scheduled`
annotation are also annotated with New Relic's `@Trace` annotation. Such
methods should ideally not also represent Spring MVC endpoints.
- A Spring-specific check which enforces that `@RequestMapping` annotations,
- A Spring-specific check that enforces that `@RequestMapping` annotations,
when applied to a method, explicitly specify one or more target HTTP methods.
- A Spring-specific check which looks for classes in which all
`@RequestMapping` annotations (and the various aliases) specify the same
`path`/`value` property and then moves that path to the class level.
- A Spring-specific check which flags `@Value("some.property")` annotations, as
- A Spring-specific check that looks for classes in which all `@RequestMapping`
annotations (and the various aliases) specify the same `path`/`value`
property and then moves that path to the class level.
- A Spring-specific check that flags `@Value("some.property")` annotations, as
these almost certainly should be `@Value("${some.property}")`.
- A Spring-specific check which drops the `required` attribute from
- A Spring-specific check that drops the `required` attribute from
`@RequestParam` annotations when the `defaultValue` attribute is also
specified.
- A Spring-specific check which rewrites a class which uses field injection to
- A Spring-specific check that rewrites a class which uses field injection to
one which uses constructor injection. This check wouldn't be strictly
behavior preserving, but could be used for a one-off code base migration.
- A Spring-specific check which disallows field injection, except in
- A Spring-specific check that disallows field injection, except in
`AbstractTestNGSpringContextTests` subclasses. (One known edge case:
self-injections so that a bean can call itself through an implicit proxy.)
- A Spring-specific check which verifies that public methods on all classes
- A Spring-specific check that verifies that public methods on all classes
whose name matches a certain pattern, e.g. `.*Service`, are annotated
`@Secured`.
- A Spring-specific check which verifies that annotations such as
- A Spring-specific check that verifies that annotations such as
`@RequestParam` are only present in `@RestController` classes.
- A Spring-specific check which disallows `@ResponseStatus` on MVC endpoint
- A Spring-specific check that disallows `@ResponseStatus` on MVC endpoint
methods, as this prevents communication of error status codes.
- A Hibernate Validator-specific check which looks for `@UnwrapValidatedValue`
- A Hibernate Validator-specific check that looks for `@UnwrapValidatedValue`
usages and migrates the associated constraint annotations to the generic type
argument to which they (are presumed to) apply.
- A TestNG-specific check which drops method-level `@Test` annotations if a
- A TestNG-specific check that drops method-level `@Test` annotations if a
matching/more specific annotation is already present at the class level.
- A TestNG-specific check which enforces that all tests are in a group.
- A TestNG-specific check which flags field assignments in
- A TestNG-specific check that enforces that all tests are in a group.
- A TestNG-specific check that flags field assignments in
`@BeforeMethod`-annotated methods unless the class is annotated
`@Test(singleThreaded = true)`.
- A TestNG-specific check which flags usages of the `expectedExceptions`
- A TestNG-specific check that flags usages of the `expectedExceptions`
attribute of the `@Test` annotation, pointing to `assertThrows`.
- A Jongo-specific check which disallows the creation of sparse indices, in
- A Jongo-specific check that disallows the creation of sparse indices, in
favour of partial indices.
- An Immutables-specific check which replaces
- An Immutables-specific check that replaces
`checkState`/`IllegalStateException` usages inside a `@Value.Check`-annotated
method with `checkArgument`/`IllegalArgument`, since the method is invoked
when a caller attempts to create an immutable instance.
- An Immutables-specific check which disallows references to collection types
- An Immutables-specific check that disallows references to collection types
other than the Guava immutable collections, including inside generic type
arguments.
- An SLF4J-specific check which drops or adds a trailing dot from log messages,
- An SLF4J-specific check that drops or adds a trailing dot from log messages,
as applicable.
- A Mockito-specific check which identifies sequences of statements which mock
a significant number of methods on a single object with "default data"; such
- A Mockito-specific check that identifies sequences of statements which mock a
significant number of methods on a single object with "default data"; such
constructions can often benefit from a different type of default answer, such
as `Answers.RETURNS_MOCKS`.
- An RxJava-specific check which flags `.toCompletable()` calls on expressions
- An RxJava-specific check that flags `.toCompletable()` calls on expressions
of type `Single<Completable>` etc., as most likely
`.flatMapCompletable(Functions.identity())` was meant instead. Idem for other
variations.
- An RxJava-specific check which flags `expr.firstOrError()` calls and suggests
- An RxJava-specific check that flags `expr.firstOrError()` calls and suggests
`expr.switchIfEmpty(Single.error(...))`, so that an application-specific
exception is thrown instead of `NoSuchElementException`.
- An RxJava-specific check which flags use of `#assertValueSet` without
- An RxJava-specific check that flags use of `#assertValueSet` without
`#assertValueCount`, as the former method doesn't do what one may intuitively
expect it to do. See ReactiveX/RxJava#6151.

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@
import java.util.Map;
import javax.lang.model.element.AnnotationValue;

/** A {@link BugChecker} which flags ambiguous {@code @JsonCreator}s in enums. */
/** A {@link BugChecker} that flags ambiguous {@code @JsonCreator}s in enums. */
@AutoService(BugChecker.class)
@BugPattern(
summary = "`JsonCreator.Mode` should be set for single-argument creators",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@
import com.sun.source.tree.MethodInvocationTree;

/**
* A {@link BugChecker} which flags AssertJ {@code isEqualTo(null)} checks for simplification.
* A {@link BugChecker} that flags AssertJ {@code isEqualTo(null)} checks for simplification.
*
* <p>This bug checker cannot be replaced with a simple Refaster template, as the Refaster approach
* would require that all overloads of {@link org.assertj.core.api.Assert#isEqualTo(Object)} (such
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@
import com.sun.source.tree.Tree;
import java.util.List;

/** A {@link BugChecker} which flags redundant {@code @Autowired} constructor annotations. */
/** A {@link BugChecker} that flags redundant {@code @Autowired} constructor annotations. */
@AutoService(BugChecker.class)
@BugPattern(
summary = "Omit `@Autowired` on a class' sole constructor, as it is redundant",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@
import java.util.regex.Pattern;
import tech.picnic.errorprone.bugpatterns.util.SourceCode;

/** A {@link BugChecker} which flags annotations that could be written more concisely. */
/** A {@link BugChecker} that flags annotations that could be written more concisely. */
@AutoService(BugChecker.class)
@BugPattern(
summary = "Omit redundant syntax from annotation declarations",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
import java.util.stream.Collector;

/**
* A {@link BugChecker} which flags {@link Collector Collectors} that don't clearly express
* A {@link BugChecker} that flags {@link Collector Collectors} that don't clearly express
* (im)mutability.
*
* <p>Replacing such collectors with alternatives that produce immutable collections is preferred.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@
import com.sun.source.tree.Tree;
import java.util.Optional;

/** A {@link BugChecker} which flags empty methods that seemingly can simply be deleted. */
/** A {@link BugChecker} that flags empty methods that seemingly can simply be deleted. */
@AutoService(BugChecker.class)
@BugPattern(
summary = "Empty method can likely be deleted",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@
import java.util.Optional;

/**
* A {@link BugChecker} which flags improperly formatted Error Prone test code.
* A {@link BugChecker} that flags improperly formatted Error Prone test code.
*
* <p>All test code should be formatted in accordance with Google Java Format's {@link Formatter}
* output, and imports should be ordered according to the {@link Style#GOOGLE Google} style.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@
import java.util.stream.Stream;

/**
* A {@link BugChecker} which flags {@link Ordering#explicit(Object, Object[])}} invocations listing
* A {@link BugChecker} that flags {@link Ordering#explicit(Object, Object[])}} invocations listing
* a subset of an enum type's values.
*/
@AutoService(BugChecker.class)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@
import reactor.core.publisher.Flux;

/**
* A {@link BugChecker} which flags usages of {@link Flux#flatMap(Function)} and {@link
* A {@link BugChecker} that flags usages of {@link Flux#flatMap(Function)} and {@link
* Flux#flatMapSequential(Function)}.
*
* <p>{@link Flux#flatMap(Function)} and {@link Flux#flatMapSequential(Function)} eagerly perform up
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,8 @@
import tech.picnic.errorprone.bugpatterns.util.SourceCode;

/**
* A {@link BugChecker} which flags string concatenations that produce a format string; in such
* cases the string concatenation should instead be deferred to the invoked method.
* A {@link BugChecker} that flags string concatenations that produce a format string; in such cases
* the string concatenation should instead be deferred to the invoked method.
*
* @implNote This checker is based on the implementation of {@link
* com.google.errorprone.bugpatterns.flogger.FloggerStringConcatenation}.
Expand All @@ -46,7 +46,7 @@
// should introduce special handling of `Formattable` arguments, as this check would replace a
// `Formattable#toString` invocation with a `Formattable#formatTo` invocation. But likely that
// should be considered a bug fix, too.
// XXX: Introduce a separate check which adds/removes the `Locale` parameter to `String.format`
// XXX: Introduce a separate check that adds/removes the `Locale` parameter to `String.format`
// invocations, as necessary.
@AutoService(BugChecker.class)
@BugPattern(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@
import javax.lang.model.element.Modifier;

/**
* A {@link BugChecker} which flags {@link SortedSet} property declarations inside
* A {@link BugChecker} that flags {@link SortedSet} property declarations inside
* {@code @Value.Immutable}- and {@code @Value.Modifiable}-annotated types that lack a
* {@code @Value.NaturalOrder} or {@code @Value.ReverseOrder} annotation.
*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,8 +38,8 @@
import javax.lang.model.element.Name;
import tech.picnic.errorprone.bugpatterns.util.SourceCode;

/** A {@link BugChecker} which flags non-canonical JUnit method declarations. */
// XXX: Consider introducing a class-level check which enforces that test classes:
/** A {@link BugChecker} that flags non-canonical JUnit method declarations. */
// XXX: Consider introducing a class-level check that enforces that test classes:
// 1. Are named `*Test` or `Abstract*TestCase`.
// 2. If not `abstract`, are package-private and don't have public methods and subclasses.
// 3. Only have private fields.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@
import tech.picnic.errorprone.bugpatterns.util.SourceCode;

/**
* A {@link BugChecker} which flags annotation array listings which aren't sorted lexicographically.
* A {@link BugChecker} that flags annotation array listings which aren't sorted lexicographically.
*
* <p>The idea behind this checker is that maintaining a sorted sequence simplifies conflict
* resolution, and can even avoid it if two branches add the same entry.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@
import javax.lang.model.element.Name;

/**
* A {@link BugChecker} which flags lambda expressions that can be replaced with method references.
* A {@link BugChecker} that flags lambda expressions that can be replaced with method references.
*/
// XXX: Other custom expressions we could rewrite:
// - `a -> "str" + a` to `"str"::concat`. But only if `str` is provably non-null.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
import tech.picnic.errorprone.bugpatterns.util.SourceCode;

/**
* A {@link BugChecker} which flags method invocations for which all arguments are wrapped using
* A {@link BugChecker} that flags method invocations for which all arguments are wrapped using
* {@link org.mockito.Mockito#eq}; this is redundant.
*/
@AutoService(BugChecker.class)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
import com.sun.tools.javac.util.List;
import java.util.Optional;

/** A {@link BugChecker} which flags nesting of {@link Optional Optionals}. */
/** A {@link BugChecker} that flags nesting of {@link Optional Optionals}. */
@AutoService(BugChecker.class)
@BugPattern(
summary =
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@
import tech.picnic.errorprone.bugpatterns.util.SourceCode;

/**
* A {@link BugChecker} which flags {@link Mono} operations that are known to be vacuous, given that
* A {@link BugChecker} that flags {@link Mono} operations that are known to be vacuous, given that
* they are invoked on a {@link Mono} that is known not to complete empty.
*/
@AutoService(BugChecker.class)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@
import tech.picnic.errorprone.bugpatterns.util.SourceCode;

/**
* A {@link BugChecker} which flags {@code Comparator#comparing*} invocations that can be replaced
* A {@link BugChecker} that flags {@code Comparator#comparing*} invocations that can be replaced
* with an equivalent alternative so as to avoid unnecessary (un)boxing.
*/
// XXX: Add more documentation. Explain how this is useful in the face of refactoring to more
Expand Down
Loading