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

Stop writing solutions that are equivalent to default annotations #339

Merged
merged 23 commits into from
Feb 10, 2022

Conversation

zcai1
Copy link

@zcai1 zcai1 commented Jun 28, 2021

Problem Description: Currently, we write all solutions back to insertable slots, even if some of them are equivalent to the default assumptions for those slots. This behavior has caused a lot of noises in the generaterd files; for example, in the following piece of code:

@NonNull
Map<@Nullable String, @Nullable String> m = new @NonNull HashMap<>();

@NonNull's are superfluous information.

This PR introduces a new field AnnotationMirror defaultAnnotation to SourceVariableSlot. The field contains the default annotation that's associated with SourceVariableSlot.location from the real type factory. Before writing solutions to Jaif, InferenceMain will check if a solution is equivalent to the default annotation. If the answer is yes, the solution won't be written back into the code.

This PR also adds a new option --insertDefaultAnnotations so we can still insert all solutions for debugging.

@zcai1 zcai1 requested a review from wmdietl June 28, 2021 03:07
if (TreeUtils.isTypeTree(node)) {
annotatedType = realTypeFactory.getAnnotatedTypeFromTypeTree(node);

if (annotatedType instanceof AnnotatedTypeMirror.AnnotatedTypeVariable) {
Copy link
Author

Choose a reason for hiding this comment

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

add a comment for explanation

} else {
annotatedType = realTypeFactory.getAnnotatedType(node);
}
Set<AnnotationMirror> annotations = annotatedType.getAnnotations();
Copy link
Author

Choose a reason for hiding this comment

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

use annotatedType.getAnnotationInHierarchy(top); instead

@zcai1
Copy link
Author

zcai1 commented Jun 28, 2021

find how we filter out the annotations for local variables

@zcai1
Copy link
Author

zcai1 commented Jul 7, 2021

check whether NninfChecker check for class bounds

type, fullyQualifiedName, location);
}

return realTypeFactory.getAnnotatedType(typeElement);
Copy link
Author

Choose a reason for hiding this comment

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

find out the correct method for fetching default annotation

Copy link
Author

Choose a reason for hiding this comment

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

try to see if getTypeDeclarationBounds is the method we want

if (TreeUtils.isTypeTree(node)) {
annotatedType = realTypeFactory.getAnnotatedTypeFromTypeTree(node);

if (annotatedType.isDeclaration() && annotatedType instanceof AnnotatedTypeMirror.AnnotatedTypeVariable) {
Copy link
Author

Choose a reason for hiding this comment

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

try to write some test cases for this

@zcai1
Copy link
Author

zcai1 commented Nov 3, 2021

handle "extends A & B" in a separate PR

Copy link
Member

@wmdietl wmdietl left a comment

Choose a reason for hiding this comment

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

Thanks! Another round of comments from a look through the code.

src/checkers/inference/DefaultSlotManager.java Outdated Show resolved Hide resolved
src/checkers/inference/DefaultSlotManager.java Outdated Show resolved Hide resolved
src/checkers/inference/DefaultSlotManager.java Outdated Show resolved Hide resolved
@@ -122,6 +122,9 @@
@Option("For inference, add debug on the port indicated")
public static String debug;

@Option("If ture, don't insert solutions that are equivalent to the default ones back to the code.")
Copy link
Member

Choose a reason for hiding this comment

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

"Debugging" doesn't sound like the best option group for this.

Copy link
Author

Choose a reason for hiding this comment

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

Moved to "Annotation File Utilities options" because I think it's more related to annotation insertion.

src/checkers/inference/InferenceOptions.java Outdated Show resolved Hide resolved
src/checkers/inference/InferenceOptions.java Outdated Show resolved Hide resolved
src/checkers/inference/InferenceLauncher.java Outdated Show resolved Hide resolved
src/checkers/inference/DefaultSlotManager.java Outdated Show resolved Hide resolved
@zcai1 zcai1 assigned wmdietl and unassigned zcai1 Jan 31, 2022
Copy link
Member

@wmdietl wmdietl left a comment

Choose a reason for hiding this comment

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

This is great! I just have a few small questions/comments left.

src/checkers/inference/DefaultSlotManager.java Outdated Show resolved Hide resolved
addToSlots(sourceVarSlot);
locationCache.put(location, sourceVarSlot.getId());
}
return sourceVarSlot;
}

/**
* Find the default annotation for this location by checking the real type factory.
* @param location Location to create a new {@link SourceVariableSlot}.
Copy link
Member

Choose a reason for hiding this comment

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

Nit: I think the @param and @return text fragments should not end with a .. Can you find and suggest a good javadoc style guide?

Copy link
Author

@zcai1 zcai1 Feb 9, 2022

Choose a reason for hiding this comment

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

Parameter names are lowercase by convention. The data type starts with a lowercase letter to indicate an object rather than a class. The description begins with a lowercase letter if it is a phrase (contains no verb), or an uppercase letter if it is a sentence. End the phrase with a period only if another phrase or sentence follows it.

For return:

Use the same capitalization and punctuation as you used in @ param.

from https://www.oracle.com/technical-resources/articles/java/javadoc-tool.html

src/checkers/inference/DefaultSlotManager.java Outdated Show resolved Hide resolved
src/checkers/inference/DefaultSlotManager.java Outdated Show resolved Hide resolved
src/checkers/inference/InferenceMain.java Show resolved Hide resolved
src/checkers/inference/InferenceMain.java Show resolved Hide resolved
src/checkers/inference/InferenceMain.java Outdated Show resolved Hide resolved
slotManager = new DefaultSlotManager(inferenceChecker.getProcessingEnvironment(),
realTypeFactory.getSupportedTypeQualifiers(), true );
Set<? extends AnnotationMirror> tops = realTypeFactory.getQualifierHierarchy().getTopAnnotations();
if (tops.size() != 1) {
Copy link
Member

Choose a reason for hiding this comment

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

In PICO I think we have two tops, one for immutability and one for initialization. Only one hierarchy, immutability, is inferred, the other one is not. Haifeng will run in problems with this, but I'm fine with solving it then.

Copy link
Author

Choose a reason for hiding this comment

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

Ok, we should discuss more about this in his PR.

src/checkers/inference/model/SourceVariableSlot.java Outdated Show resolved Hide resolved
@wmdietl wmdietl assigned zcai1 and unassigned wmdietl Feb 9, 2022
@wmdietl wmdietl merged commit a3d7683 into opprop:master Feb 10, 2022
@wmdietl wmdietl linked an issue Feb 10, 2022 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

When creating a slot, store the default qualifier for that location.
3 participants