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

Jump to bottom

Propagate length information from argument to return value of Arrays.copyOf() #3524

Merged
merged 14 commits into from
Aug 3, 2020
Merged

Propagate length information from argument to return value of Arrays.copyOf() #3524

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
5624d27
Merge pull request #1 from typetools/master
PRITI1999 Jul 10, 2020
3723b51
Merge pull request #4 from typetools/master
PRITI1999 Jul 21, 2020
32a728f
Temporary fix for #3224
PRITI1999 Jul 25, 2020
2fea713
Add ArrayLen annotations from @IntVal
PRITI1999 Jul 26, 2020
888a7a4
Remove unnecessary () and add documentation to test case
PRITI1999 Jul 28, 2020
48ad98a
Merge pull request #8 from typetools/master
PRITI1999 Jul 29, 2020
688ca4f
Remove unnecessary ()
PRITI1999 Jul 29, 2020
3966be6
Improve fix and augment a func to the test case
PRITI1999 Jul 30, 2020
35a6430
Merge branch 'fix-issue-3224' of https://github.com/priti1999/checker…
PRITI1999 Jul 30, 2020
ecb48f5
Improvements
PRITI1999 Jul 31, 2020
e309153
Correct variable
PRITI1999 Jul 31, 2020
221d3c0
Build fix and minor improvement
PRITI1999 Aug 1, 2020
5a02c56
Add doc
PRITI1999 Aug 1, 2020
881bbe9
build fix
PRITI1999 Aug 1, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
20 changes: 20 additions & 0 deletions checker/tests/index/Issue3224.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
// Test case for https://tinyurl.com/cfissue/3224

import java.util.Arrays;
import org.checkerframework.common.value.qual.MinLen;

public class Issue3224 {

public static void m1(String @MinLen(1) [] args) {
int i = 4;
String @MinLen(1) [] args2 = Arrays.copyOf(args, i);
}

public static void m2(String @MinLen(1) [] args) {
String @MinLen(1) [] args2 = Arrays.copyOf(args, args.length);
}

public static void m3(String @MinLen(1) ... args) {
String @MinLen(1) [] args2 = Arrays.copyOf(args, args.length);
}
}
23 changes: 23 additions & 0 deletions framework/src/main/java/org/checkerframework/common/value/ValueVisitor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,14 @@

import com.sun.source.tree.AnnotationTree;
import com.sun.source.tree.ExpressionTree;
import com.sun.source.tree.MethodInvocationTree;
import com.sun.source.tree.MethodTree;
import com.sun.source.tree.Tree;
import com.sun.source.tree.Tree.Kind;
import com.sun.source.tree.TypeCastTree;
import java.util.Collections;
import java.util.List;
import java.util.stream.Collectors;
import javax.lang.model.element.AnnotationMirror;
import javax.lang.model.type.TypeKind;
import javax.lang.model.type.TypeMirror;
Expand All @@ -17,6 +19,7 @@
import org.checkerframework.common.value.qual.IntRangeFromGTENegativeOne;
import org.checkerframework.common.value.qual.IntRangeFromNonNegative;
import org.checkerframework.common.value.qual.IntRangeFromPositive;
import org.checkerframework.common.value.qual.IntVal;
import org.checkerframework.common.value.util.NumberUtils;
import org.checkerframework.common.value.util.Range;
import org.checkerframework.framework.type.AnnotatedTypeMirror;
Expand Down Expand Up @@ -76,6 +79,26 @@ protected void commonAssignmentCheck(
getTypeFactory().createIntRangeAnnotation(Range.CHAR_EVERYTHING));
}

if (valueTree.getKind() == Kind.METHOD_INVOCATION
&& TreeUtils.isArrayscopyOfMethodInvocation((MethodInvocationTree) valueTree)
&& valueType.getKind() == TypeKind.ARRAY) {
List<? extends ExpressionTree> args = ((MethodInvocationTree) valueTree).getArguments();
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add a check here that the length of args is what you expect (i.e. 2) and throw a BugInCF otherwise

AnnotatedTypeMirror arrType = atypeFactory.getAnnotatedType(args.get(0));
if (TreeUtils.isArrayLengthAccess(args.get(1))) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This only tests that the second argument is any array length access expression. The correct test needs to check that it is an array length access for the array that is the first argument. You can add a test case for this:

    public static void m4(String @MinLen(1) [] args, String[] otherArray) {
        // :: error: assignment.type.incompatible
        String @MinLen(1) [] args2 = Arrays.copyOf(args, otherArray.length);
    }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do we want to check this now? As suggested above I have added to valuetype any @IntVal or @IntRange annotation on the second argument. Suppose the user intends to have args2 of the same length as otherArray, then this check might lead to a false positive.

Copy link
Contributor

Choose a reason for hiding this comment

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

In the test case I suggested, that is a true positive: otherArray might not be @MinLen(1), even if the user hopes that it is - its length could be zero. I think you can add that test method verbatim into the existing test (except for changing the name).

valueType = arrType;
} else if (atypeFactory.getAnnotatedType(args.get(1)).getAnnotation(IntVal.class)
!= null) {
kelloggm marked this conversation as resolved.
Show resolved Hide resolved
AnnotationMirror argType =
atypeFactory.getAnnotatedType(args.get(1)).getAnnotation(IntVal.class);
valueType.addAnnotation(
getTypeFactory()
.createArrayLenAnnotation(
ValueAnnotatedTypeFactory.getIntValues(argType).stream()
.map(Long::intValue)
.collect(Collectors.toList())));
}
}

super.commonAssignmentCheck(varType, valueType, valueTree, errorKey, extraArgs);
}

Expand Down
21 changes: 21 additions & 0 deletions javacutil/src/main/java/org/checkerframework/javacutil/TreeUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1273,6 +1273,27 @@ public static boolean isAnonymousConstructor(final MethodTree method) {
return false;
}

/**
* Determines if the tree is a call to Arrays.copyOf()
*
* @param tree tree to check
* @return true if the given tree is a call to Arrays.copyOf() method
*/
public static boolean isArrayscopyOfMethodInvocation(MethodInvocationTree tree) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: in camel-case, this should be isArraysCopyOfMethodInvocation (that is, capitalize the "c" in "copy")

if (tree.getMethodSelect().getKind() != Kind.MEMBER_SELECT) {
return false;
}

MemberSelectTree memberSelectTree = (MemberSelectTree) tree.getMethodSelect();

if (memberSelectTree.getExpression().toString().equals("Arrays")
&& getMethodName(memberSelectTree).equals("copyOf")) {
return true;
Copy link
Contributor

Choose a reason for hiding this comment

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

This test isn't strong enough - it will return true on any method named copyOf in a class named Arrays, even if it is not the one from the JDK.

You should look at how IndexMethodIdentifier in the Index Checker identifies specific methods; you should be able to use the same kind of code here to identify Arrays.copyOf with confidence.

(It would probably also be worthwhile to add an inner static class named "Arrays" with a static "copyOf" method to the tests and then ensure that your code is not invoked when that method is called.)

}

return false;
}

/**
* Converts the given AnnotationTrees to AnnotationMirrors.
*
Expand Down