-
Notifications
You must be signed in to change notification settings - Fork 330
Migrate AutoValue to Records #1415
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
Changes from 14 commits
1b2483b
9c7e5d3
ed33a43
75a2d78
c43b083
76cf410
9ebfd92
506e4c4
a14c9ce
9de1799
32806d6
5dd8b69
f7fd4bd
21d6359
3000a24
f8671ff
fa305c1
91f0cdc
b449530
d85b929
06ccd25
c2e6bda
c67a209
158fbd2
daec884
99049cb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,24 +1,22 @@ | ||
| package com.uber.nullaway.libmodel; | ||
|
|
||
| import com.google.auto.value.AutoValue; | ||
| import com.google.common.collect.ImmutableMap; | ||
| import com.google.common.collect.ImmutableSet; | ||
|
|
||
| /** A record describing the annotations associated with a java method and its arguments. */ | ||
| @AutoValue | ||
| public abstract class MethodAnnotationsRecord { | ||
| /** A record describing the annotations associated with a java method and its arguments. | ||
| * Generates by Record | ||
| */ | ||
|
|
||
| public record MethodAnnotationsRecord(ImmutableSet<String> methodAnnotations, | ||
| ImmutableSet<Integer> typeParamNullableUpperbounds, | ||
| ImmutableMap<Integer, ImmutableSet<String>> argumentAnnotations) { | ||
|
|
||
| public static MethodAnnotationsRecord create( | ||
| ImmutableSet<String> methodAnnotations, | ||
| ImmutableSet<Integer> typeParamNullableUpperbounds, | ||
| ImmutableMap<Integer, ImmutableSet<String>> argumentAnnotations) { | ||
| return new AutoValue_MethodAnnotationsRecord( | ||
| return new MethodAnnotationsRecord( | ||
| methodAnnotations, typeParamNullableUpperbounds, argumentAnnotations); | ||
| } | ||
coderabbitai[bot] marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
msridhar marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| abstract ImmutableSet<String> methodAnnotations(); | ||
|
|
||
| abstract ImmutableSet<Integer> typeParamNullableUpperbounds(); | ||
|
|
||
| abstract ImmutableMap<Integer, ImmutableSet<String>> argumentAnnotations(); | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -22,7 +22,6 @@ | |
|
|
||
| package com.uber.nullaway; | ||
|
|
||
| import com.google.auto.value.AutoValue; | ||
| import com.google.common.base.Joiner; | ||
| import com.google.common.base.Preconditions; | ||
| import com.google.common.collect.ImmutableSet; | ||
|
|
@@ -609,11 +608,11 @@ public boolean warnOnGenericInferenceFailure() { | |
| return warnOnInferenceFailure; | ||
| } | ||
|
|
||
| @AutoValue | ||
| abstract static class MethodClassAndName { | ||
| record MethodClassAndName(String enclosingClass, | ||
| String methodName) { | ||
|
|
||
| static MethodClassAndName create(String enclosingClass, String methodName) { | ||
| return new AutoValue_ErrorProneCLIFlagsConfig_MethodClassAndName(enclosingClass, methodName); | ||
| return new MethodClassAndName(enclosingClass, methodName); | ||
| } | ||
|
||
|
|
||
| static MethodClassAndName fromClassDotMethod(String classDotMethod) { | ||
|
|
@@ -623,8 +622,5 @@ static MethodClassAndName fromClassDotMethod(String classDotMethod) { | |
| return MethodClassAndName.create(className, methodName); | ||
| } | ||
|
|
||
| abstract String enclosingClass(); | ||
|
|
||
| abstract String methodName(); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -21,7 +21,6 @@ | |
| import static com.uber.nullaway.NullabilityUtil.castToNonNull; | ||
| import static com.uber.nullaway.NullabilityUtil.findEnclosingMethodOrLambdaOrInitializer; | ||
|
|
||
| import com.google.auto.value.AutoValue; | ||
| import com.google.common.base.Preconditions; | ||
| import com.google.common.base.Verify; | ||
| import com.google.common.cache.CacheBuilder; | ||
|
|
@@ -352,13 +351,21 @@ public boolean isRunning(TreePath path, Context context, AccessPathNullnessPropa | |
| return analysis.isRunning(); | ||
| } | ||
|
|
||
| @AutoValue | ||
| abstract static class CfgParams { | ||
| // Should not be used for hashCode or equals | ||
| static final class CfgParams { | ||
| /** | ||
| * Records cannot declare extra instance fields (environment). | ||
| * Used direct replacement approach to migrate from AutoValue | ||
| */ | ||
| private final TreePath codePath; | ||
|
|
||
| private @Nullable ProcessingEnvironment environment; | ||
|
|
||
| private static CfgParams create(TreePath codePath, ProcessingEnvironment environment) { | ||
| CfgParams cp = new AutoValue_DataFlow_CfgParams(codePath); | ||
| private CfgParams(TreePath codePath) { | ||
| this.codePath = codePath; | ||
| } | ||
|
|
||
| static CfgParams create(TreePath codePath, ProcessingEnvironment environment) { | ||
| CfgParams cp = new CfgParams(codePath); | ||
| cp.environment = environment; | ||
| return cp; | ||
| } | ||
|
|
@@ -367,21 +374,26 @@ ProcessingEnvironment environment() { | |
| return castToNonNull(environment); | ||
| } | ||
|
|
||
| abstract TreePath codePath(); | ||
| TreePath codePath() { | ||
| return codePath; | ||
| } | ||
| /* | ||
| * If needed, need to override these methods | ||
| * @Override | ||
| * public boolean equals(Object o) { ...codePath... } | ||
| * | ||
| * @Override | ||
| * public int hashCode() { ...codePath... } | ||
| */ | ||
|
||
| } | ||
|
Comment on lines
355
to
473
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick | 🔵 Trivial 🧩 Analysis chain🏁 Script executed: cd nullaway && find . -name "DataFlow.java" -type fRepository: uber/NullAway Length of output: 113 🏁 Script executed: wc -l nullaway/src/main/java/com/uber/nullaway/dataflow/DataFlow.javaRepository: uber/NullAway Length of output: 124 🏁 Script executed: sed -n '355,477p' nullaway/src/main/java/com/uber/nullaway/dataflow/DataFlow.javaRepository: uber/NullAway Length of output: 4056 🏁 Script executed: # Check for CfgParams usage throughout the file
rg -n "CfgParams" nullaway/src/main/java/com/uber/nullaway/dataflow/DataFlow.javaRepository: uber/NullAway Length of output: 1063 🏁 Script executed: # Check for LoadingCache and synchronization in the file
rg -n "LoadingCache|synchronized|volatile" nullaway/src/main/java/com/uber/nullaway/dataflow/DataFlow.java | head -20Repository: uber/NullAway Length of output: 277 🏁 Script executed: # Check imports to see what caching framework is used
head -50 nullaway/src/main/java/com/uber/nullaway/dataflow/DataFlow.java | grep -E "^import"Repository: uber/NullAway Length of output: 1545 🏁 Script executed: sed -n '93,155p' nullaway/src/main/java/com/uber/nullaway/dataflow/DataFlow.javaRepository: uber/NullAway Length of output: 3507 🏁 Script executed: # Check what happens in the CacheLoader.load() method
sed -n '97,155p' nullaway/src/main/java/com/uber/nullaway/dataflow/DataFlow.javaRepository: uber/NullAway Length of output: 3349 🏁 Script executed: # Check for all accesses to environment field or environment() method
rg -n "\.environment|environment\(\)" nullaway/src/main/java/com/uber/nullaway/dataflow/DataFlow.javaRepository: uber/NullAway Length of output: 211 🏁 Script executed: # Check if CfgParams instances are stored/retrieved outside of immediate cache usage
rg -n "CfgParams" nullaway/src/main/java/com/uber/nullaway/dataflow/DataFlow.java -A 2 -B 2Repository: uber/NullAway Length of output: 2940 Simplify CfgParams by initializing environment in the constructor. The That said, the post-construction initialization pattern (lines 402-406) adds unnecessary complexity. Passing 🤖 Prompt for AI Agents |
||
|
|
||
| @AutoValue | ||
| abstract static class AnalysisParams { | ||
| record AnalysisParams(ForwardTransferFunction<?, ?> transferFunction, ControlFlowGraph cfg) { | ||
|
|
||
| private static AnalysisParams create( | ||
| ForwardTransferFunction<?, ?> transferFunction, ControlFlowGraph cfg) { | ||
| AnalysisParams ap = new AutoValue_DataFlow_AnalysisParams(transferFunction, cfg); | ||
| return ap; | ||
| return new AnalysisParams(transferFunction, cfg); | ||
| } | ||
|
|
||
| abstract ForwardTransferFunction<?, ?> transferFunction(); | ||
|
|
||
| abstract ControlFlowGraph cfg(); | ||
| } | ||
|
Comment on lines
475
to
481
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick | 🔵 Trivial Consider removing the redundant factory method. The ♻️ Optional: Call-sites can use the record constructor directlyAt lines 158 and 350, replace: -AnalysisParams aparams = AnalysisParams.create(transfer, cfg);
+AnalysisParams aparams = new AnalysisParams(transfer, cfg);Then remove the record AnalysisParams(ForwardTransferFunction<?, ?> transferFunction, ControlFlowGraph cfg) {
-
- private static AnalysisParams create(
- ForwardTransferFunction<?, ?> transferFunction, ControlFlowGraph cfg) {
- return new AnalysisParams(transferFunction, cfg);
- }
-
}🤖 Prompt for AI Agents |
||
|
|
||
| /** A pair of Analysis and ControlFlowGraph. */ | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -24,7 +24,6 @@ | |
|
|
||
| import static com.uber.nullaway.NullabilityUtil.castToNonNull; | ||
|
|
||
| import com.google.auto.value.AutoValue; | ||
| import com.google.common.collect.ImmutableList; | ||
| import com.google.common.collect.ImmutableMap; | ||
| import com.google.common.collect.LinkedHashMultimap; | ||
|
|
@@ -128,18 +127,15 @@ class StreamNullabilityPropagator implements Handler { | |
| private final Map<MethodInvocationTree, Tree> observableCallToInnerMethodOrLambda = | ||
| new LinkedHashMap<>(); | ||
|
|
||
| @AutoValue | ||
| abstract static class CollectRecordAndInnerMethod { | ||
| record CollectRecordAndInnerMethod(CollectLikeMethodRecord collectlikeMethodRecord, | ||
| Tree innerMethodOrLambda) { | ||
|
|
||
| static CollectRecordAndInnerMethod create( | ||
| CollectLikeMethodRecord collectlikeMethodRecord, Tree innerMethodOrLambda) { | ||
| return new AutoValue_StreamNullabilityPropagator_CollectRecordAndInnerMethod( | ||
| return new CollectRecordAndInnerMethod( | ||
| collectlikeMethodRecord, innerMethodOrLambda); | ||
| } | ||
|
||
|
|
||
| abstract CollectLikeMethodRecord getCollectLikeMethodRecord(); | ||
|
|
||
| abstract Tree getInnerMethodOrLambda(); | ||
| } | ||
|
|
||
| // Maps collect calls in the observable call chain to the relevant (collect record, inner method | ||
|
|
@@ -380,9 +376,9 @@ private void handleChainFromFilter( | |
| collectCallToRecordsAndInnerMethodsOrLambdas.get(castToNonNull(outerCallInChain))) { | ||
| MapOrCollectMethodToFilterInstanceRecord record = | ||
| new MapOrCollectMethodToFilterInstanceRecord( | ||
| collectRecordAndInnerMethod.getCollectLikeMethodRecord(), filterMethodOrLambda); | ||
| collectRecordAndInnerMethod.collectlikeMethodRecord(), filterMethodOrLambda); | ||
| mapOrCollectRecordToFilterMap.put( | ||
| collectRecordAndInnerMethod.getInnerMethodOrLambda(), record); | ||
| collectRecordAndInnerMethod.innerMethodOrLambda(), record); | ||
| } | ||
| } | ||
| } while (outerCallInChain != null | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably we can just get rid of this line now?