refactored StringConcatFinder pattern #560
Conversation
| elif node.operandr.line: | ||
| lines.add(node.operandr.line) | ||
|
|
||
| elif hasattr(node.operandr, 'value') and isinstance(node.operandr.value, str) and \ |
There was a problem hiding this comment.
It sounds like we check only if one argument of BINARY_OPERATION is string, while comment in class says we are looking for both arguments to be string. If that correct, we have to check both operands to be string.
There was a problem hiding this comment.
we search for any string concatenation. but there can be different concatenation:
"String" + "4"
int a = "b";
a+ "b";
b + "a";
"; " + message.toString()
Integer d = 1 + b;
System.out.println(a + b);
etc.
look at the tests, there are examples there.
There was a problem hiding this comment.
So it just look for using concatenation with any string literal?
If so, we better create a single function to check for a string literal and use it to check left and right side of operators.
There was a problem hiding this comment.
@Vitaly-Protasov please move logic of checking operand to separate function and call it in operandr and operandl here.
| elif node.operandr.line: | ||
| lines.add(node.operandr.line) | ||
|
|
||
| elif hasattr(node.operandr, 'value') and isinstance(node.operandr.value, str) and \ |
There was a problem hiding this comment.
we search for any string concatenation. but there can be different concatenation:
"String" + "4"
int a = "b";
a+ "b";
b + "a";
"; " + message.toString()
Integer d = 1 + b;
System.out.println(a + b);
etc.
look at the tests, there are examples there.
| @@ -33,18 +33,20 @@ def value(self, filename: str) -> List[int]: | |||
| ast = AST.build_from_javalang(build_ast(filename)) | |||
| for node in ast.get_proxy_nodes(ASTNodeType.BINARY_OPERATION): | |||
| if node.operator == '+': | |||
| # by this check we want to filter nodes and consider only | |||
| # [Literal, MemberReference, MethodInvocation, This] | |||
There was a problem hiding this comment.
How did you get this list?
When I check node types for value field, I get following:
- ASSERT_STATEMENT
- ASSIGNMENT
- ELEMENT_VALUE_PAIR
- LITERAL
- TRY_RESOURCE
By the way, can you explain, how other than LITERAL nodes meet this pattern?
| if node.line: | ||
| lines.add(node.line) | ||
| elif node.line: |
There was a problem hiding this comment.
Please pay attention to the code you write.
There was a problem hiding this comment.
We expect .line to return always int. So simply add it to the set.
| if node.line: | ||
| lines.add(node.line) | ||
| elif node.line: | ||
| lines.add(node.line) |
| if node.line: | ||
| lines.add(node.line) | ||
| elif node.line: |
There was a problem hiding this comment.
We expect .line to return always int. So simply add it to the set.
| elif node.operandr.line: | ||
| lines.add(node.operandr.line) | ||
|
|
||
| elif hasattr(node.operandr, 'value') and isinstance(node.operandr.value, str) and \ |
There was a problem hiding this comment.
@Vitaly-Protasov please move logic of checking operand to separate function and call it in operandr and operandl here.
|
@rultor merge |
@acheshkov OK, I'll try to merge now. You can check the progress of the merge here |
@acheshkov Done! FYI, the full log is here (took me 18min) |
Solved in case of #528