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.

Sign up for GitHub

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

Refactored data clumps with the help of LLMs (research project) #960

Merged
merged 7 commits into from
Jun 17, 2024
Merged

Refactored data clumps with the help of LLMs (research project) #960

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
f7d8b62
refactor data clumps
compf May 28, 2024
c6c924e
simplified multiple getter calls, renaming of getters, and added equa…
compf Jun 11, 2024
4c0766c
Merge branch 'master' into refactor_data_clumps
msridhar Jun 14, 2024
8186a88
renamed visitorState to state and used better equals operation
compf Jun 17, 2024
d976096
applied better equal operation on other fields
compf Jun 17, 2024
48bd1dc
Merge branch 'master' into refactor_data_clumps
msridhar Jun 17, 2024
bbedf7b
javadoc
msridhar Jun 17, 2024
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
14 changes: 8 additions & 6 deletions nullaway/src/main/java/com/uber/nullaway/NullAway.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,7 @@
import com.uber.nullaway.generics.GenericsChecks;
import com.uber.nullaway.handlers.Handler;
import com.uber.nullaway.handlers.Handlers;
import com.uber.nullaway.handlers.MethodAnalysisContext;
import java.util.ArrayList;
import java.util.LinkedHashMap;
import java.util.LinkedHashSet;
Expand Down Expand Up @@ -402,7 +403,7 @@ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState
return Description.NO_MATCH;
}
Symbol.MethodSymbol methodSymbol = getSymbolForMethodInvocation(tree);
handler.onMatchMethodInvocation(this, tree, state, methodSymbol);
handler.onMatchMethodInvocation(tree, new MethodAnalysisContext(this, state, methodSymbol));
// assuming this list does not include the receiver
List<? extends ExpressionTree> actualParams = tree.getArguments();
return handleInvocation(tree, state, methodSymbol, actualParams);
Expand Down Expand Up @@ -671,7 +672,7 @@ public Description matchMethod(MethodTree tree, VisitorState state) {
// overridden method (if overridden method is in an annotated
// package)
Symbol.MethodSymbol methodSymbol = ASTHelpers.getSymbol(tree);
handler.onMatchMethod(this, tree, state, methodSymbol);
handler.onMatchMethod(tree, new MethodAnalysisContext(this, state, methodSymbol));
boolean isOverriding = ASTHelpers.hasAnnotation(methodSymbol, "java.lang.Override", state);
boolean exhaustiveOverride = config.exhaustiveOverride();
if (isOverriding || !exhaustiveOverride) {
Expand Down Expand Up @@ -979,7 +980,8 @@ public Description matchLambdaExpression(LambdaExpressionTree tree, VisitorState
// we need to update environment mapping before running the handler, as some handlers
// (like Rx nullability) run dataflow analysis
updateEnvironmentMapping(state.getPath(), state);
handler.onMatchLambdaExpression(this, tree, state, funcInterfaceMethod);
handler.onMatchLambdaExpression(
tree, new MethodAnalysisContext(this, state, funcInterfaceMethod));
if (codeAnnotationInfo.isSymbolUnannotated(funcInterfaceMethod, config, handler)) {
return Description.NO_MATCH;
}
Expand Down Expand Up @@ -1023,7 +1025,7 @@ public Description matchMemberReference(MemberReferenceTree tree, VisitorState s
Symbol.MethodSymbol referencedMethod = ASTHelpers.getSymbol(tree);
Symbol.MethodSymbol funcInterfaceSymbol =
NullabilityUtil.getFunctionalInterfaceMethod(tree, state.getTypes());
handler.onMatchMethodReference(this, tree, state, referencedMethod);
handler.onMatchMethodReference(tree, new MethodAnalysisContext(this, state, referencedMethod));
return checkOverriding(funcInterfaceSymbol, referencedMethod, tree, state);
}

Expand Down Expand Up @@ -1467,7 +1469,7 @@ private boolean okToReadBeforeInitialized(TreePath path, VisitorState state) {
} else {
castToNonNullArg =
handler.castToNonNullArgumentPositionsForMethod(
this, state, methodSymbol, arguments, null);
arguments, null, new MethodAnalysisContext(this, state, methodSymbol));
}
if (castToNonNullArg != null && leaf.equals(arguments.get(castToNonNullArg))) {
return true;
Expand Down Expand Up @@ -1840,7 +1842,7 @@ private Description checkCastToNonNullTakesNullable(
} else {
castToNonNullPosition =
handler.castToNonNullArgumentPositionsForMethod(
this, state, methodSymbol, actualParams, null);
actualParams, null, new MethodAnalysisContext(this, state, methodSymbol));
}
if (castToNonNullPosition != null) {
ExpressionTree actual = actualParams.get(castToNonNullPosition);
Expand Down
105 changes: 59 additions & 46 deletions nullaway/src/main/java/com/uber/nullaway/handlers/AbstractFieldContractHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,27 +60,26 @@ protected AbstractFieldContractHandler(String annotName) {
/**
* Verifies that the method being processed adheres to the annotation specifications.
*
* @param analysis NullAway instance.
* @param tree Method tree under processing.
* @param state Error Prone {@link VisitorState}.
* @param methodSymbol Processing method symbol.
* @param methodAnalysisContext The MethodAnalysisContext object
*/
@Override
public void onMatchMethod(
NullAway analysis, MethodTree tree, VisitorState state, Symbol.MethodSymbol methodSymbol) {
public void onMatchMethod(MethodTree tree, MethodAnalysisContext methodAnalysisContext) {
Set<String> annotationContent =
NullabilityUtil.getAnnotationValueArray(methodSymbol, annotName, false);
NullabilityUtil.getAnnotationValueArray(
methodAnalysisContext.getMethodSymbol(), annotName, false);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getMethodSymbol() used multiple times in this method and should be stored in a local variable

boolean isAnnotated = annotationContent != null;
boolean isValid =
isAnnotated
&& validateAnnotationSyntax(
castToNonNull(annotationContent), analysis, tree, state, methodSymbol)
&& validateAnnotationSemantics(analysis, state, tree, methodSymbol);
castToNonNull(annotationContent), tree, methodAnalysisContext)
&& validateAnnotationSemantics(tree, methodAnalysisContext);
if (isAnnotated && !isValid) {
return;
}
Symbol.MethodSymbol closestOverriddenMethod =
NullabilityUtil.getClosestOverriddenMethod(methodSymbol, state.getTypes());
NullabilityUtil.getClosestOverriddenMethod(
methodAnalysisContext.getMethodSymbol(), methodAnalysisContext.getState().getTypes());
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similarly getState() is used multiple times

if (closestOverriddenMethod == null) {
return;
}
Expand All @@ -90,8 +89,13 @@ && validateAnnotationSyntax(
} else {
fieldNames = Collections.emptySet();
}
validateOverridingRules(fieldNames, analysis, state, tree, closestOverriddenMethod);
super.onMatchMethod(analysis, tree, state, methodSymbol);
validateOverridingRules(
fieldNames,
methodAnalysisContext.getAnalysis(),
methodAnalysisContext.getState(),
tree,
closestOverriddenMethod);
super.onMatchMethod(tree, methodAnalysisContext);
}

/**
Expand All @@ -117,9 +121,10 @@ protected abstract void validateOverridingRules(
* Validates that a method implementation matches the semantics of the annotation.
*
* @return Returns true, if the annotation conforms to the semantic rules.
* @param methodAnalysisContext The MethodAnalysisContext object
*/
protected abstract boolean validateAnnotationSemantics(
NullAway analysis, VisitorState state, MethodTree tree, Symbol.MethodSymbol methodSymbol);
MethodTree tree, MethodAnalysisContext methodAnalysisContext);

/**
* Validates whether the parameter inside annotation conforms to the syntax rules. Parameters must
Expand All @@ -137,29 +142,29 @@ protected abstract boolean validateAnnotationSemantics(
* <p>
*
* @return Returns true, if the annotation conforms to the syntax rules.
* @param methodAnalysisContext The MethodAnalysisContext object
*/
protected boolean validateAnnotationSyntax(
Set<String> content,
NullAway analysis,
MethodTree tree,
VisitorState state,
Symbol.MethodSymbol methodSymbol) {
Set<String> content, MethodTree tree, MethodAnalysisContext methodAnalysisContext) {
String message;
if (content.isEmpty()) {
// we should not allow useless annotations.
message =
"empty @"
+ annotName
+ " is the default precondition for every method, please remove it.";
state.reportMatch(
analysis
.getErrorBuilder()
.createErrorDescription(
new ErrorMessage(ErrorMessage.MessageTypes.ANNOTATION_VALUE_INVALID, message),
tree,
analysis.buildDescription(tree),
state,
null));
methodAnalysisContext
.getState()
.reportMatch(
methodAnalysisContext
.getAnalysis()
.getErrorBuilder()
.createErrorDescription(
new ErrorMessage(ErrorMessage.MessageTypes.ANNOTATION_VALUE_INVALID, message),
tree,
methodAnalysisContext.getAnalysis().buildDescription(tree),
methodAnalysisContext.getState(),
null));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getState() and getAnalysis() used multiple times here

return false;
} else {
for (String fieldName : content) {
Expand All @@ -172,22 +177,26 @@ protected boolean validateAnnotationSyntax(
+ fieldName
+ " is not supported";

state.reportMatch(
analysis
.getErrorBuilder()
.createErrorDescription(
new ErrorMessage(
ErrorMessage.MessageTypes.ANNOTATION_VALUE_INVALID, message),
tree,
analysis.buildDescription(tree),
state,
null));
methodAnalysisContext
.getState()
.reportMatch(
methodAnalysisContext
.getAnalysis()
.getErrorBuilder()
.createErrorDescription(
new ErrorMessage(
ErrorMessage.MessageTypes.ANNOTATION_VALUE_INVALID, message),
tree,
methodAnalysisContext.getAnalysis().buildDescription(tree),
methodAnalysisContext.getState(),
null));
return false;
} else {
fieldName = fieldName.substring(fieldName.lastIndexOf(".") + 1);
}
}
Symbol.ClassSymbol classSymbol = castToNonNull(ASTHelpers.enclosingClass(methodSymbol));
Symbol.ClassSymbol classSymbol =
castToNonNull(ASTHelpers.enclosingClass(methodAnalysisContext.getMethodSymbol()));
VariableElement field = getInstanceFieldOfClass(classSymbol, fieldName);
if (field == null) {
message =
Expand All @@ -197,15 +206,19 @@ protected boolean validateAnnotationSyntax(
+ fieldName
+ " in class "
+ classSymbol.getSimpleName();
state.reportMatch(
analysis
.getErrorBuilder()
.createErrorDescription(
new ErrorMessage(ErrorMessage.MessageTypes.ANNOTATION_VALUE_INVALID, message),
tree,
analysis.buildDescription(tree),
state,
null));
methodAnalysisContext
.getState()
.reportMatch(
methodAnalysisContext
.getAnalysis()
.getErrorBuilder()
.createErrorDescription(
new ErrorMessage(
ErrorMessage.MessageTypes.ANNOTATION_VALUE_INVALID, message),
tree,
methodAnalysisContext.getAnalysis().buildDescription(tree),
methodAnalysisContext.getState(),
null));
return false;
}
}
Expand Down
24 changes: 6 additions & 18 deletions nullaway/src/main/java/com/uber/nullaway/handlers/BaseNoOpHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,35 +73,25 @@ public void onMatchTopLevelClass(
}

@Override
public void onMatchMethod(
NullAway analysis, MethodTree tree, VisitorState state, Symbol.MethodSymbol methodSymbol) {
public void onMatchMethod(MethodTree tree, MethodAnalysisContext methodAnalysisContext) {
// NoOp
}

@Override
public void onMatchMethodInvocation(
NullAway analysis,
MethodInvocationTree tree,
VisitorState state,
Symbol.MethodSymbol methodSymbol) {
MethodInvocationTree tree, MethodAnalysisContext methodAnalysisContext) {
// NoOp
}

@Override
public void onMatchLambdaExpression(
NullAway analysis,
LambdaExpressionTree tree,
VisitorState state,
Symbol.MethodSymbol methodSymbol) {
LambdaExpressionTree tree, MethodAnalysisContext methodAnalysisContext) {
// NoOp
}

@Override
public void onMatchMethodReference(
NullAway analysis,
MemberReferenceTree tree,
VisitorState state,
Symbol.MethodSymbol methodSymbol) {
MemberReferenceTree tree, MethodAnalysisContext methodAnalysisContext) {
// NoOp
}

Expand Down Expand Up @@ -238,11 +228,9 @@ public MethodInvocationNode onCFGBuildPhase1AfterVisitMethodInvocation(
@Override
@Nullable
public Integer castToNonNullArgumentPositionsForMethod(
NullAway analysis,
VisitorState state,
Symbol.MethodSymbol methodSymbol,
List<? extends ExpressionTree> actualParams,
@Nullable Integer previousArgumentPosition) {
@Nullable Integer previousArgumentPosition,
MethodAnalysisContext methodAnalysisContext) {
// NoOp
return previousArgumentPosition;
}
Expand Down
34 changes: 11 additions & 23 deletions nullaway/src/main/java/com/uber/nullaway/handlers/CompositeHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,43 +80,33 @@ public void onMatchTopLevelClass(
}

@Override
public void onMatchMethod(
NullAway analysis, MethodTree tree, VisitorState state, Symbol.MethodSymbol methodSymbol) {
public void onMatchMethod(MethodTree tree, MethodAnalysisContext methodAnalysisContext) {
for (Handler h : handlers) {
h.onMatchMethod(analysis, tree, state, methodSymbol);
h.onMatchMethod(tree, methodAnalysisContext);
}
}

@Override
public void onMatchLambdaExpression(
NullAway analysis,
LambdaExpressionTree tree,
VisitorState state,
Symbol.MethodSymbol methodSymbol) {
LambdaExpressionTree tree, MethodAnalysisContext methodAnalysisContext) {
for (Handler h : handlers) {
h.onMatchLambdaExpression(analysis, tree, state, methodSymbol);
h.onMatchLambdaExpression(tree, methodAnalysisContext);
}
}

@Override
public void onMatchMethodReference(
NullAway analysis,
MemberReferenceTree tree,
VisitorState state,
Symbol.MethodSymbol methodSymbol) {
MemberReferenceTree tree, MethodAnalysisContext methodAnalysisContext) {
for (Handler h : handlers) {
h.onMatchMethodReference(analysis, tree, state, methodSymbol);
h.onMatchMethodReference(tree, methodAnalysisContext);
}
}

@Override
public void onMatchMethodInvocation(
NullAway analysis,
MethodInvocationTree tree,
VisitorState state,
Symbol.MethodSymbol methodSymbol) {
MethodInvocationTree tree, MethodAnalysisContext methodAnalysisContext) {
for (Handler h : handlers) {
h.onMatchMethodInvocation(analysis, tree, state, methodSymbol);
h.onMatchMethodInvocation(tree, methodAnalysisContext);
}
}

Expand Down Expand Up @@ -310,15 +300,13 @@ public MethodInvocationNode onCFGBuildPhase1AfterVisitMethodInvocation(
@Override
@Nullable
public Integer castToNonNullArgumentPositionsForMethod(
NullAway analysis,
VisitorState state,
Symbol.MethodSymbol methodSymbol,
List<? extends ExpressionTree> actualParams,
@Nullable Integer previousArgumentPosition) {
@Nullable Integer previousArgumentPosition,
MethodAnalysisContext methodAnalysisContext) {
for (Handler h : handlers) {
previousArgumentPosition =
h.castToNonNullArgumentPositionsForMethod(
analysis, state, methodSymbol, actualParams, previousArgumentPosition);
actualParams, previousArgumentPosition, methodAnalysisContext);
}
return previousArgumentPosition;
}
Expand Down
Loading