Deprecate StringConcatenateAssignmentNode #183

al3xliu · 2022-02-27T05:28:47Z

No description provided.

framework/tests/reflection/MethodTest.java

dataflow/src/main/java/org/checkerframework/dataflow/cfg/builder/CFGTranslationPhaseOne.java

zcai1 · 2022-02-28T20:40:01Z

dataflow/src/main/java/org/checkerframework/dataflow/cfg/builder/CFGTranslationPhaseOne.java

+                                        tree.getExpression());
+                        Node operNode = new StringConcatenateNode(operTree, targetRHS, value);
+                        extendWithNode(operNode);
+                        AssignmentNode r = new AssignmentNode(tree, targetLHS, operNode);


Let's rename it to assignNode for consistency

Thanks, done.

zcai1 · 2022-02-28T21:55:47Z

Please update the constructor and docs in AssignmentNode, thanks!

wmdietl

Please also deprecate the now unused class (and uses) and update the manual.

dataflow/src/main/java/org/checkerframework/dataflow/cfg/builder/CFGTranslationPhaseOne.java

framework/tests/reflection/MethodTest.java

docs

wmdietl

Thanks!

dataflow/manual/content.tex

dataflow/src/main/java/org/checkerframework/dataflow/cfg/builder/CFGTranslationPhaseOne.java

wmdietl · 2022-03-09T04:39:24Z

dataflow/src/main/java/org/checkerframework/dataflow/cfg/node/AbstractNodeVisitor.java

@@ -159,6 +159,7 @@ public R visitBitwiseXor(BitwiseXorNode n, P p) {
    }

    // Compound assignments
+    @Deprecated()


Just use @Deprecated, without the empty parenthesis.

wmdietl · 2022-03-09T04:40:43Z

dataflow/src/main/java/org/checkerframework/dataflow/cfg/node/AssignmentNode.java

@@ -18,6 +18,7 @@
 *
 * <pre>
 *   <em>variable</em> = <em>expression</em>
+ *   <em>String variable+</em> = <em>expression</em>


Suggested change

* String variable+ = expression

* variable += expression

wmdietl · 2022-03-09T04:42:20Z

dataflow/src/main/java/org/checkerframework/dataflow/cfg/node/AssignmentNode.java

@@ -26,6 +27,8 @@
 *
 * <p>Some desugarings create additional assignments to synthetic local variables. Such assignment
 * nodes are marked as synthetic to allow special handling in transfer functions.
+ *
+ * <p>StringConcatenateAssignmentNode is parsed as an Assignment node now.


Suggested change

* StringConcatenateAssignmentNode is parsed as an Assignment node now.

* String concatenation compound assignments are desugared to an assignment and a concatenation.

Don't focus on the change, document what is. Don't use a class that doesn't exist anymore in the documentation.
What is the proper term for it?

The examples and this text don't mention that other compound-assignments are also desugared. Might be better to also fix that problem now.

wmdietl · 2022-03-09T04:44:31Z

...ow/src/main/java/org/checkerframework/dataflow/cfg/node/StringConcatenateAssignmentNode.java

- * A node for the string concatenation compound assignment:
- *
- * <pre>
+ * @deprecated Use {@code #AssignmentNode()} instead. A node for the string concatenation compound


The @deprecated part is the last thing in the javadoc block, separate from the rest.

wmdietl · 2022-03-09T04:45:16Z

framework/src/main/java/org/checkerframework/common/value/ValueTransfer.java

@@ -13,6 +13,8 @@
 import org.checkerframework.dataflow.analysis.RegularTransferResult;
 import org.checkerframework.dataflow.analysis.TransferInput;
 import org.checkerframework.dataflow.analysis.TransferResult;
+// TODO: remove this line after merging StringConcatenateAssignmentNode completely.
+import org.checkerframework.dataflow.cfg.node.*;


Unfortunately, a format check fails because of this.
Is there no cleaner way to suppress the deprecation warning?

wmdietl · 2022-03-09T04:45:53Z

framework/tests/h1h2checker/CompoundStringAssignment.java

@@ -9,7 +9,6 @@ void test1() {
        String local = null;
        // There was a bug in data flow where
        // the type of local was @H1Bot @H2Bot after this
-        // StringConcatenateAssignmentNode,


The sentence doesn't make sense if you remove the class name. What is this saying?

wmdietl

Please add a notice in docs/CHANGELOG.md under Implementation details.

Adding SKIP-REQUIRE-JAVADOC for this PR is fine, we don't want to document NodeVisitor here.

wmdietl · 2022-03-12T22:39:19Z

dataflow/manual/content.tex

@@ -1344,7 +1338,8 @@ \subsubsection{Desugaring}

    \item Once we decided to make conversion nodes explicit it made
      sense to desugar compound assignments.  A compound assignment
-      like \begin{verbatim}Integer i; i += 3;\end{verbatim} performs
+      like \begin{verbatim}Integer i; i += 3;\end{verbatim} or
+      \begin{verbatim}String s; s += "a";\end{verbatim} performs


This sentence talks about boxing of i, which doesn't make sense in the string example.
Maybe string compound concatenation should be a separate item in this list?

Yeah, will start a new item here, I think it is a suitable place.

wmdietl · 2022-03-12T22:40:33Z

dataflow/src/main/java/org/checkerframework/dataflow/cfg/node/AssignmentNode.java

+ * <p>Number concatenation compound assignments are desugared to an assignment and a number
+ * concatenation.


Suggested change

* Number concatenation compound assignments are desugared to an assignment and a number

* concatenation.

* Numeric concatenation compound assignments are desugared to an assignment and a numeric

* concatenation.

wmdietl · 2022-03-12T22:42:11Z

...ow/src/main/java/org/checkerframework/dataflow/cfg/node/StringConcatenateAssignmentNode.java

@@ -15,7 +15,10 @@
 * <pre>
 *   <em>variable</em> += <em>expression</em>
 * </pre>
+ *
+ * @deprecated Use {@code #AssignmentNode()} instead.


Nobody has to use AssignmentNode instead and this seems like a reference to a constructor, which doesn't exist in this class. Say that StringConcatenateAssignmentNode isn't generated any more in CFGs and that instead an assignment and concatenation is generated.

Okay, thanks!

wmdietl

Thanks! A few small things left.

wmdietl · 2022-03-22T20:12:25Z

dataflow/manual/content.tex

      all compound assignments greatly reduced the total number of
      node classes.

+    \item Also, we desugared string concatenation assignments. A string
+      concatenation assignment like \begin{verbatim}String s; s += "str";
+      \end{verbatim} is represented as \begin{verbatim}String s; s = s + "str";
+      \end{verbatim} in CFGs.
+


Suggested change

all compound assignments greatly reduced the total number of

node classes.

\item Also, we desugared string concatenation assignments. A string

concatenation assignment like \begin{verbatim}String s; s += "str";

\end{verbatim} is represented as \begin{verbatim}String s; s = s + "str";

\end{verbatim} in CFGs.

all compound assignments greatly reduced the total number of

node classes.

\item String concatenation assignments are also desugared. A string

concatenation assignment like

\begin{verbatim}String s; s += "str";\end{verbatim} is represented as

\begin{verbatim}String s; s = s + "str";\end{verbatim} in CFGs.

This avoids duplicating the same logic that is necessary in assignment

and concatenation nodes, which was error prone.

Thanks, done.

wmdietl · 2022-03-22T20:17:07Z

dataflow/src/main/java/org/checkerframework/dataflow/cfg/node/AssignmentNode.java

+ * <p>Numeric concatenation compound assignments are desugared to an assignment and a numeric
+ * concatenation.


Suggested change

* Numeric concatenation compound assignments are desugared to an assignment and a numeric

* concatenation.

* Numeric compound assignments are desugared to an assignment and a numeric operation.

Thanks, done.

wmdietl · 2022-03-22T20:19:52Z

...ow/src/main/java/org/checkerframework/dataflow/cfg/node/StringConcatenateAssignmentNode.java

+ * @deprecated StringConcatenateAssignmentNode is not generated anymore in CFG nodes, and that
+ *     instead an assignment and concatenation is generated.


Suggested change

* @deprecated StringConcatenateAssignmentNode is not generated anymore in CFG nodes, and that

* instead an assignment and concatenation is generated.

* @deprecated StringConcatenateAssignmentNode is no longer used in CFGs.

* Instead, an assignment and a concatenation node are generated.

Thanks, done.

...ow/src/main/java/org/checkerframework/dataflow/cfg/node/StringConcatenateAssignmentNode.java

wmdietl · 2022-03-22T20:25:42Z

docs/CHANGELOG.md

+`CFGTranslationPhaseOne` now generates an assignment and concatenation instead.
+


Suggested change

`CFGTranslationPhaseOne` now generates an assignment and concatenation instead.

`CFGTranslationPhaseOne` now generates an assignment and a concatenation node instead.

This avoids error prone duplication of logic.

Thanks, done.

wmdietl · 2022-03-22T20:25:58Z

docs/CHANGELOG.md

@@ -12,6 +12,9 @@ Changed `CFAbstractTransfer.insertIntoStores` from public to protected
 visibility. It is only meant as a utility method for use within a
 transfer function.

+Deprecated class `StringConcatenateAssignmentNode` and its related usage.


Suggested change

Deprecated class `StringConcatenateAssignmentNode` and its related usage.

Deprecated class `StringConcatenateAssignmentNode` and its usages.

Thanks, done.

docs/CHANGELOG.md

dataflow/src/main/java/org/checkerframework/dataflow/cfg/node/NodeVisitor.java

framework/src/main/java/org/checkerframework/common/value/ValueTransfer.java

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

dataflow/src/main/java/org/checkerframework/dataflow/cfg/node/AbstractNodeVisitor.java

dataflow/src/main/java/org/checkerframework/dataflow/cfg/node/NodeVisitor.java

wmdietl

Thanks!

remove StringConcatenateAssignmentNode

c883b50

zcai1 reviewed Feb 28, 2022

View reviewed changes

framework/tests/reflection/MethodTest.java Show resolved Hide resolved

zcai1 reviewed Feb 28, 2022

View reviewed changes

dataflow/src/main/java/org/checkerframework/dataflow/cfg/builder/CFGTranslationPhaseOne.java Show resolved Hide resolved

zcai1 reviewed Feb 28, 2022

View reviewed changes

wmdietl requested changes Mar 2, 2022

View reviewed changes

dataflow/src/main/java/org/checkerframework/dataflow/cfg/builder/CFGTranslationPhaseOne.java Show resolved Hide resolved

framework/tests/reflection/MethodTest.java Show resolved Hide resolved

wmdietl assigned al3xliu Mar 2, 2022

al3xliu added 2 commits March 6, 2022 18:17

deprecated stringconcatenateassignmentnode and relating usage;refine

4a3cf8d

docs

fix import warning

0b6a54b

wmdietl requested changes Mar 9, 2022

View reviewed changes

wmdietl assigned al3xliu and unassigned al3xliu Mar 9, 2022

al3xliu added 9 commits March 11, 2022 22:43

fix comments

acc2f15

fix missing doc

389c6bf

try to fix doc again

899f638

fix doc

7b4f95e

fix doc

960dc55

fix doc

31f1301

fix doc

968612a

fix doc

4e15e51

add SKIP-REQUIRE-JAVADOC

8d78aa8

wmdietl requested changes Mar 12, 2022

View reviewed changes

wmdietl and others added 2 commits March 12, 2022 17:45

Merge branch 'master' into rm_str_concatenate

9b0b9da

fix comments

07ba7c5

wmdietl requested changes Mar 22, 2022

View reviewed changes

al3xliu and others added 2 commits March 22, 2022 17:35

fix comments from Werner

9734ba5

Merge branch 'master' into rm_str_concatenate

13e8942

wmdietl reviewed Mar 22, 2022

View reviewed changes

docs/CHANGELOG.md Outdated Show resolved Hide resolved

Update docs/CHANGELOG.md

5424a6d

wmdietl changed the title ~~Remove StringConcatenateAssignmentNode~~ Deprecate StringConcatenateAssignmentNode Mar 23, 2022

al3xliu commented Mar 23, 2022

View reviewed changes

dataflow/src/main/java/org/checkerframework/dataflow/cfg/node/NodeVisitor.java Outdated Show resolved Hide resolved

al3xliu commented Mar 23, 2022

View reviewed changes

framework/src/main/java/org/checkerframework/common/value/ValueTransfer.java Outdated Show resolved Hide resolved

al3xliu commented Mar 23, 2022

View reviewed changes

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

al3xliu commented Mar 23, 2022

View reviewed changes

dataflow/src/main/java/org/checkerframework/dataflow/cfg/node/AbstractNodeVisitor.java Outdated Show resolved Hide resolved

al3xliu and others added 2 commits March 23, 2022 13:50

Add date comment to @deprecated

cad04b5

Merge branch 'master' into rm_str_concatenate

7c5be34

wmdietl reviewed Mar 23, 2022

View reviewed changes

dataflow/src/main/java/org/checkerframework/dataflow/cfg/node/NodeVisitor.java Outdated Show resolved Hide resolved

Fix spacing

5715c66

wmdietl approved these changes Mar 23, 2022

View reviewed changes

wmdietl enabled auto-merge (squash) March 23, 2022 18:16

wmdietl merged commit 605eb41 into eisop:master Mar 23, 2022

al3xliu added a commit to al3xliu/checker-framework that referenced this pull request Mar 27, 2022

Deprecate StringConcatenateAssignmentNode (eisop#183)

65140c7

al3xliu added a commit to al3xliu/checker-framework that referenced this pull request Mar 28, 2022

Deprecate StringConcatenateAssignmentNode (eisop#183)

4af93e7

wmdietl mentioned this pull request Mar 29, 2022

Add reaching definitions analysis #181

Merged

	* <em>String variable+</em> = <em>expression</em>
	* <em>variable</em> += <em>expression</em>

	* <p>StringConcatenateAssignmentNode is parsed as an Assignment node now.
	* <p>String concatenation compound assignments are desugared to an assignment and a concatenation.

		* <p>Number concatenation compound assignments are desugared to an assignment and a number
		* concatenation.

		* <p>Numeric concatenation compound assignments are desugared to an assignment and a numeric
		* concatenation.

	* <p>Numeric concatenation compound assignments are desugared to an assignment and a numeric
	* concatenation.
	* <p>Numeric compound assignments are desugared to an assignment and a numeric operation.

		* @deprecated StringConcatenateAssignmentNode is not generated anymore in CFG nodes, and that
		* instead an assignment and concatenation is generated.

		`CFGTranslationPhaseOne` now generates an assignment and concatenation instead.

	`CFGTranslationPhaseOne` now generates an assignment and concatenation instead.
	`CFGTranslationPhaseOne` now generates an assignment and a concatenation node instead.
	This avoids error prone duplication of logic.

	Deprecated class `StringConcatenateAssignmentNode` and its related usage.
	Deprecated class `StringConcatenateAssignmentNode` and its usages.

Deprecate StringConcatenateAssignmentNode #183

Deprecate StringConcatenateAssignmentNode #183

Conversation

al3xliu commented Feb 27, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zcai1 commented Feb 28, 2022

wmdietl left a comment

Choose a reason for hiding this comment

wmdietl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wmdietl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wmdietl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wmdietl left a comment

Choose a reason for hiding this comment