Apply declaration bounds to string conversions and binary comparisons #213

zcai1 · 2022-04-06T15:04:00Z

This PR is based on #212

Previously, string conversion node gets the same value as its operand has. This behavior is error prone because the annotations of its operand could be the supertypes of String's declaration bounds. This PR fixes such issue using the following rules:

If the operand is a type variable and has no annotations, we apply its effective annotations instead.
Otherwise, the operand should always have some annotations. For each annotation, if its a subtype of the string declaration bound in the same hierarchy, we add it to the result value; otherwise, we add the bound to the result value.

One new issue is also introduced by this PR: opprop#208

wmdietl

Thanks! I'll look at this in detail once #212 is merged.

framework/src/main/java/org/checkerframework/framework/flow/CFAbstractTransfer.java

framework/src/main/java/org/checkerframework/framework/type/AnnotatedTypeFactory.java

This reverts commit ec3c6e1.

…ng-conversion-node

…ypesApplier

…Annotations

…ng-conversion-node

wmdietl

Thanks for all the work on this, especially the extensive tests! I have a bunch of comments we can discuss.

javacutil/src/main/java/org/checkerframework/javacutil/TreeUtils.java

checker/src/main/java/org/checkerframework/checker/lock/LockTreeAnnotator.java

framework/src/main/java/org/checkerframework/framework/flow/CFAbstractTransfer.java

framework/src/main/java/org/checkerframework/framework/type/AnnotatedTypeFactory.java

...rc/main/java/org/checkerframework/framework/type/treeannotator/PropagationTreeAnnotator.java

…binaryTreeArgTypes

…ng-conversion-node

wmdietl

Thanks for all these fixes! I only have small comments.
Can you expand the changelog to mention these changes?
Also, should the PR title be a bit more general now?

checker/src/main/java/org/checkerframework/checker/signedness/SignednessVisitor.java

framework/src/main/java/org/checkerframework/framework/type/AnnotatedTypeFactory.java

javacutil/src/main/java/org/checkerframework/javacutil/TreeUtils.java

wmdietl · 2022-06-02T22:58:41Z

javacutil/src/main/java/org/checkerframework/javacutil/TypesUtils.java

@@ -336,6 +336,17 @@ public static boolean isBooleanType(TypeMirror type) {
        return isDeclaredOfName(type, "java.lang.Boolean") || type.getKind() == TypeKind.BOOLEAN;
    }

+    /**
+     * Checks if the type represents a character type, that is either char (primitive type) or


I see that this is similar to the comment on the above method, but shouldn't it be that is, is either...? At the moment, doesn't this mean that there are other possible options for "character type", but we return true only for these two?

I thought this means we are checking if the given TypeMirror is a boxed or primitive character type. Could you please elaborate on "other possible options"?

…cai1/checker-framework into apply-bounds-to-string-conversion-node

wmdietl

Thanks!

docs/CHANGELOG.md

framework/src/main/java/org/checkerframework/framework/type/AnnotatedTypeFactory.java

…tedTypeFactory` can customize its behavior

wmdietl reviewed Apr 6, 2022

View reviewed changes

framework/src/main/java/org/checkerframework/framework/flow/CFAbstractTransfer.java Outdated Show resolved Hide resolved

framework/src/main/java/org/checkerframework/framework/type/AnnotatedTypeFactory.java Outdated Show resolved Hide resolved

wmdietl assigned zcai1 Apr 6, 2022

zcai1 force-pushed the apply-bounds-to-string-conversion-node branch from eead7d5 to 429a905 Compare April 13, 2022 08:20

zcai1 added 8 commits April 13, 2022 04:23

apply type bound to string conversion

90f942c

add @return javadoc

08f0307

use effective annotation from operand

0af1cea

use effective annotation for type var only

28337c2

add simple type system for testing

ae1f8d0

address comments

ec71d56

use erased type in addAnnoOrBound

486fa0e

Revert "use erased type in addAnnoOrBound"

1deedb7

This reverts commit ec3c6e1.

zcai1 force-pushed the apply-bounds-to-string-conversion-node branch from 429a905 to 1deedb7 Compare April 13, 2022 08:23

zcai1 added 12 commits April 13, 2022 04:23

Merge remote-tracking branch 'eisop/master' into apply-bounds-to-stri…

423118c

…ng-conversion-node

Merge remote-tracking branch 'eisop/master' into apply-bounds-to-stri…

4fa24ad

…ng-conversion-node

apply bounds for binary

9f0f6af

Merge remote-tracking branch 'eisop/master' into apply-bounds-to-stri…

27aed40

…ng-conversion-node

rename "implicit conversion test" to "type declaration bounds test"

5ccbcef

fix primary not found for type var and NPE issues in DefaultInferredT…

35a5477

…ypesApplier

add more tests

7030d48

simplify TreeUtils

ce5304d

remove redundant logic from LockTreeAnnotator

298c25f

add TODO to visitInstanceOf in CFAbstractTransfer

7e4fc45

revert LockTreeAnnotator and add detailed comments to addComputedType…

961a9b9

…Annotations

Merge remote-tracking branch 'eisop/master' into apply-bounds-to-stri…

a4cbdf1

…ng-conversion-node

wmdietl reviewed May 18, 2022

View reviewed changes

zcai1 and others added 4 commits May 20, 2022 03:25

address feedback

4196cd8

rollback changes to dataflow analysis and apply string conversion at …

765ce9a

…binaryTreeArgTypes

Merge remote-tracking branch 'eisop/master' into apply-bounds-to-stri…

10b4afc

…ng-conversion-node

Merge branch 'master' into apply-bounds-to-string-conversion-node

1e2d94d

wmdietl reviewed Jun 2, 2022

View reviewed changes

wmdietl and others added 3 commits June 2, 2022 19:00

Merge branch 'master' into apply-bounds-to-string-conversion-node

be5aabd

address feedback

8436201

Merge branch 'apply-bounds-to-string-conversion-node' of github.com:z…

75c7a79

…cai1/checker-framework into apply-bounds-to-string-conversion-node

wmdietl approved these changes Jun 3, 2022

View reviewed changes

wmdietl and others added 5 commits June 3, 2022 09:09

Restructure changelog

112d466

Reword comment

597bde5

Merge branch 'master' into apply-bounds-to-string-conversion-node

a76ebd8

Reformat comment

cd9811b

make binaryTreeArgTypes a protected method so subclasses of `Annota…

1fb4c6b

…tedTypeFactory` can customize its behavior

wmdietl changed the title ~~Apply declaration bounds to string conversion node~~ Apply declaration bounds to string conversions and binary comparisons Jun 3, 2022

wmdietl merged commit 7f9e27b into eisop:master Jun 3, 2022

zcai1 mentioned this pull request Jul 7, 2022

Apply String declaration bounds to String conversion nodes opprop/checker-framework#207

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Apply declaration bounds to string conversions and binary comparisons #213

Apply declaration bounds to string conversions and binary comparisons #213

zcai1 commented Apr 6, 2022 •

edited

Loading

wmdietl left a comment

wmdietl left a comment

wmdietl left a comment

wmdietl Jun 2, 2022

zcai1 Jun 3, 2022

wmdietl left a comment

Apply declaration bounds to string conversions and binary comparisons #213

Apply declaration bounds to string conversions and binary comparisons #213

Conversation

zcai1 commented Apr 6, 2022 • edited Loading

wmdietl left a comment

Choose a reason for hiding this comment

wmdietl left a comment

Choose a reason for hiding this comment

wmdietl left a comment

Choose a reason for hiding this comment

wmdietl Jun 2, 2022

Choose a reason for hiding this comment

zcai1 Jun 3, 2022

Choose a reason for hiding this comment

wmdietl left a comment

Choose a reason for hiding this comment

zcai1 commented Apr 6, 2022 •

edited

Loading