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

Deprecate StringConcatenateAssignmentNode #183

Merged
merged 20 commits into from
Mar 23, 2022
Merged

Deprecate StringConcatenateAssignmentNode #183

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
c883b50
remove StringConcatenateAssignmentNode
al3xliu Feb 25, 2022
4a3cf8d
deprecated stringconcatenateassignmentnode and relating usage;refine
al3xliu Mar 6, 2022
0b6a54b
fix import warning
al3xliu Mar 7, 2022
acc2f15
fix comments
al3xliu Mar 12, 2022
389c6bf
fix missing doc
al3xliu Mar 12, 2022
899f638
try to fix doc again
al3xliu Mar 12, 2022
7b4f95e
fix doc
al3xliu Mar 12, 2022
960dc55
fix doc
al3xliu Mar 12, 2022
31f1301
fix doc
al3xliu Mar 12, 2022
968612a
fix doc
al3xliu Mar 12, 2022
4e15e51
fix doc
al3xliu Mar 12, 2022
8d78aa8
add SKIP-REQUIRE-JAVADOC
al3xliu Mar 12, 2022
9b0b9da
Merge branch 'master' into rm_str_concatenate
wmdietl Mar 12, 2022
07ba7c5
fix comments
al3xliu Mar 13, 2022
9734ba5
fix comments from Werner
al3xliu Mar 22, 2022
13e8942
Merge branch 'master' into rm_str_concatenate
wmdietl Mar 22, 2022
5424a6d
Update docs/CHANGELOG.md
wmdietl Mar 22, 2022
cad04b5
Add date comment to @Deprecated
al3xliu Mar 23, 2022
7c5be34
Merge branch 'master' into rm_str_concatenate
wmdietl Mar 23, 2022
5715c66
Fix spacing
wmdietl Mar 23, 2022
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
10 changes: 2 additions & 8 deletions dataflow/manual/content.tex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -813,7 +813,6 @@ \subsection{Formal Definition of the Control-Flow Graph}
\midrule

\code{Assignment} & & \code{x = 1} \\
\code{StringConcatenateAssignment} & A compound assignment. & \code{s += ".txt"} \\
\midrule

\code{ArrayCreation} & & \code{new double[]} \\
Expand Down Expand Up @@ -913,19 +912,14 @@ \subsection{Formal Definition of the Control-Flow Graph}
\end{workinprogress}

\begin{workinprogress}
``Could be desugared'' on ``StringConcatenateAssignment'' and ``Conversion
``Could be desugared'' on ``Conversion
nodes'' was confusing. What is the design rationale for
desugaring in the Dataflow Framework? Discuss that. Here, at least
forward-reference to \autoref{sec:desugaring}, if that's relevant. \\
More generally, for any cases that will be discussed in the text, add a
forward reference to the section with the discussion.
\end{workinprogress}

\begin{workinprogress}
There is a \code{StringConcatenateAssignmentNode}.
What about other compound assignments? Are they desugared?
\end{workinprogress}

\begin{workinprogress}
When we added Java~8 support, did we add additional nodes that are not
listed here? Cross-check with implementation.
Expand Down Expand Up @@ -2018,7 +2012,7 @@ \subsubsubsection{Alias Information}
% LocalWords: NumericalSubtraction SignedRightShift StringConcatenate
% LocalWords: TernaryExpression UnsignedRightShift NotEqual GreaterThan
% LocalWords: GreaterThanOrEqual LessThan LessThanOrEqual ArrayCreation
% LocalWords: StringConcatenateAssignment ObjectCreation TypeCast cond
% LocalWords: ObjectCreation TypeCast cond
% LocalWords: InstanceOf instanceof NarrowingConversion StringConversion
% LocalWords: WideningConversion ArrayType ParameterizedType ClassName
% LocalWords: PrimitiveType PackageName AssertionError NullChk accessor
Expand Down
16 changes: 12 additions & 4 deletions dataflow/src/main/java/org/checkerframework/dataflow/cfg/builder/CFGTranslationPhaseOne.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,6 @@
import org.checkerframework.dataflow.cfg.node.PrimitiveTypeNode;
import org.checkerframework.dataflow.cfg.node.ReturnNode;
import org.checkerframework.dataflow.cfg.node.SignedRightShiftNode;
import org.checkerframework.dataflow.cfg.node.StringConcatenateAssignmentNode;
import org.checkerframework.dataflow.cfg.node.StringConcatenateNode;
import org.checkerframework.dataflow.cfg.node.StringConversionNode;
import org.checkerframework.dataflow.cfg.node.StringLiteralNode;
Expand Down Expand Up @@ -1805,9 +1804,18 @@ public Node visitCompoundAssignment(CompoundAssignmentTree tree, Void p) {
assert (kind == Tree.Kind.PLUS_ASSIGNMENT);
Node targetRHS = stringConversion(targetLHS);
value = stringConversion(value);
Node r = new StringConcatenateAssignmentNode(tree, targetRHS, value);
extendWithNode(r);
return r;
BinaryTree operTree =
treeBuilder.buildBinary(
leftType,
withoutAssignment(kind),
tree.getVariable(),
tree.getExpression());
handleArtificialTree(operTree);
Node operNode = new StringConcatenateNode(operTree, targetRHS, value);
extendWithNode(operNode);
AssignmentNode assignNode = new AssignmentNode(tree, targetLHS, operNode);
extendWithNode(assignNode);
return assignNode;
} else {
TypeMirror promotedType = binaryPromotedType(leftType, rightType);
Node targetRHS = binaryNumericPromotion(targetLHS, promotedType);
Expand Down
1 change: 1 addition & 0 deletions dataflow/src/main/java/org/checkerframework/dataflow/cfg/node/AbstractNodeVisitor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,6 +159,7 @@ public R visitBitwiseXor(BitwiseXorNode n, P p) {
}

// Compound assignments
@Deprecated()
Copy link
Member

Choose a reason for hiding this comment

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

Just use @Deprecated, without the empty parenthesis.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed.

@Override
public R visitStringConcatenateAssignment(StringConcatenateAssignmentNode n, P p) {
return visitNode(n, p);
Expand Down
3 changes: 3 additions & 0 deletions dataflow/src/main/java/org/checkerframework/dataflow/cfg/node/AssignmentNode.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
*
* <pre>
* <em>variable</em> = <em>expression</em>
* <em>String variable+</em> = <em>expression</em>
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed.

* <em>expression</em> . <em>field</em> = <em>expression</em>
* <em>expression</em> [ <em>index</em> ] = <em>expression</em>
* </pre>
Expand All @@ -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.
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed.

*/
public class AssignmentNode extends Node {

Expand Down
1 change: 1 addition & 0 deletions dataflow/src/main/java/org/checkerframework/dataflow/cfg/node/NodeVisitor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@ public interface NodeVisitor<R, P> {
R visitBitwiseXor(BitwiseXorNode n, P p);

// Compound assignments
@Deprecated()
R visitStringConcatenateAssignment(StringConcatenateAssignmentNode n, P p);

// Comparison operations
Expand Down
7 changes: 4 additions & 3 deletions ...src/main/java/org/checkerframework/dataflow/cfg/node/StringConcatenateAssignmentNode.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,12 +10,13 @@
import java.util.Objects;

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

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed.

* assignment:
* <pre>
* <em>variable</em> += <em>expression</em>
* </pre>
*/
@Deprecated()
public class StringConcatenateAssignmentNode extends Node {
/** The entire tree of the assignment */
protected final Tree tree;
Expand Down
12 changes: 0 additions & 12 deletions dataflow/src/main/java/org/checkerframework/dataflow/livevariable/LiveVarTransfer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@
import org.checkerframework.dataflow.cfg.node.Node;
import org.checkerframework.dataflow.cfg.node.ObjectCreationNode;
import org.checkerframework.dataflow.cfg.node.ReturnNode;
import org.checkerframework.dataflow.cfg.node.StringConcatenateAssignmentNode;

import java.util.List;

Expand Down Expand Up @@ -49,17 +48,6 @@ public RegularTransferResult<LiveVarValue, LiveVarStore> visitAssignment(
return transferResult;
}

@Override
public RegularTransferResult<LiveVarValue, LiveVarStore> visitStringConcatenateAssignment(
StringConcatenateAssignmentNode n, TransferInput<LiveVarValue, LiveVarStore> p) {
RegularTransferResult<LiveVarValue, LiveVarStore> transferResult =
(RegularTransferResult<LiveVarValue, LiveVarStore>)
super.visitStringConcatenateAssignment(n, p);
processLiveVarInAssignment(
n.getLeftOperand(), n.getRightOperand(), transferResult.getRegularStore());
return transferResult;
}

@Override
public RegularTransferResult<LiveVarValue, LiveVarStore> visitMethodInvocation(
MethodInvocationNode n, TransferInput<LiveVarValue, LiveVarStore> p) {
Expand Down
1 change: 1 addition & 0 deletions docs/manual/contributors.tex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
Adian Qian,
Aditya Singh,
Akash Srivastava,
Alex Liu,
Alvin Abdagic,
Anant Jain,
Anatoly Kupriyanov,
Expand Down
4 changes: 3 additions & 1 deletion framework/src/main/java/org/checkerframework/common/value/ValueTransfer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.*;
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed.

import org.checkerframework.dataflow.cfg.node.BitwiseAndNode;
import org.checkerframework.dataflow.cfg.node.BitwiseComplementNode;
import org.checkerframework.dataflow.cfg.node.BitwiseOrNode;
Expand Down Expand Up @@ -41,7 +43,6 @@
import org.checkerframework.dataflow.cfg.node.NumericalPlusNode;
import org.checkerframework.dataflow.cfg.node.NumericalSubtractionNode;
import org.checkerframework.dataflow.cfg.node.SignedRightShiftNode;
import org.checkerframework.dataflow.cfg.node.StringConcatenateAssignmentNode;
import org.checkerframework.dataflow.cfg.node.StringConcatenateNode;
import org.checkerframework.dataflow.cfg.node.StringConversionNode;
import org.checkerframework.dataflow.cfg.node.StringLiteralNode;
Expand Down Expand Up @@ -601,6 +602,7 @@ private void refineAtLengthAccess(Node lengthNode, Node receiverNode, CFStore st
}

@Override
@Deprecated()
public TransferResult<CFValue, CFStore> visitStringConcatenateAssignment(
StringConcatenateAssignmentNode n, TransferInput<CFValue, CFStore> p) {
TransferResult<CFValue, CFStore> result = super.visitStringConcatenateAssignment(n, p);
Expand Down
4 changes: 3 additions & 1 deletion framework/src/main/java/org/checkerframework/framework/flow/CFAbstractTransfer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@
import org.checkerframework.dataflow.cfg.UnderlyingAST;
import org.checkerframework.dataflow.cfg.UnderlyingAST.CFGLambda;
import org.checkerframework.dataflow.cfg.UnderlyingAST.CFGMethod;
// TODO: remove this line after merging StringConcatenateAssignmentNode completely.
import org.checkerframework.dataflow.cfg.node.*;
import org.checkerframework.dataflow.cfg.node.AbstractNodeVisitor;
import org.checkerframework.dataflow.cfg.node.ArrayAccessNode;
import org.checkerframework.dataflow.cfg.node.AssignmentNode;
Expand All @@ -34,7 +36,6 @@
import org.checkerframework.dataflow.cfg.node.NotEqualNode;
import org.checkerframework.dataflow.cfg.node.ObjectCreationNode;
import org.checkerframework.dataflow.cfg.node.ReturnNode;
import org.checkerframework.dataflow.cfg.node.StringConcatenateAssignmentNode;
import org.checkerframework.dataflow.cfg.node.StringConversionNode;
import org.checkerframework.dataflow.cfg.node.SwitchExpressionNode;
import org.checkerframework.dataflow.cfg.node.TernaryExpressionNode;
Expand Down Expand Up @@ -912,6 +913,7 @@ public TransferResult<V, S> visitLambdaResultExpression(
}

@Override
@Deprecated()
public TransferResult<V, S> visitStringConcatenateAssignment(
StringConcatenateAssignmentNode n, TransferInput<V, S> in) {
// This gets the type of LHS + RHS
Expand Down
1 change: 0 additions & 1 deletion framework/tests/h1h2checker/CompoundStringAssignment.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed.

// but only if the RHS was a method call.
local += getSib1();

Expand Down
4 changes: 0 additions & 4 deletions framework/tests/reflection/MethodTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,10 +57,6 @@ public void pass3() {
try {
Class<?> c = Class.forName("MethodTest$SuperClass");
Method m = c.getMethod(str, new Class[] {});
// TODO: Should not fail -> enhance Value checker
// and remove the expected error

// :: error: (assignment.type.incompatible)
@Sibling1 Object a = m.invoke(superClass, (@ReflectBottom Object[]) null);
} catch (Exception ignore) {
}
Expand Down