SONARPY-2366 Propagate binary expressions of the same type #2216
Conversation
maksim-grebeniuk-sonarsource
force-pushed
the
SONARPY-2366
branch
6 times, most recently
from
175bd8b to
e6767e0
Compare
maksim-grebeniuk-sonarsource
force-pushed
the
SONARPY-2366
branch
from
e6767e0 to
cafb782
Compare
| && rightOperand.typeV2() instanceof ObjectType rightObjectType | ||
| && rightObjectType.unwrappedType() instanceof ClassType rightClassType | ||
| && leftClassType == rightClassType) { | ||
| return new ObjectType(leftClassType, TypeSource.min(leftObjectType.typeSource(), rightObjectType.typeSource())); |
There was a problem hiding this comment.
Couldn't the ObjectType have attributes or members which would get lost here?
There was a problem hiding this comment.
as discussed - there is no generic way of transfer the rest of type attributes for the binary expression result calculation
| import org.sonar.plugins.python.api.tree.Tree; | ||
| import org.sonar.python.types.HasTypeDependencies; | ||
|
|
||
| public class TypeDependenciesCalculator { |
There was a problem hiding this comment.
I like the idea of a TypeDependenciesCalculator as it helps splitting type inference and the types from the AST and reduces the complexity of inheritance. I wonder though, if it makes sense to keep the HasTypeDependencies interface around. It leaves AST nodes which have their logic implemented in this class in a weird place.
Case in point, BinaryExpressionImpl implements HasTypeDependencies and has an implementation of HasTypeDependencies#typeDependencies(), but that method will never be called.
Alternatively, the BinaryExpressionImpl#typeDependencies() could throw an exception, but that goes against all OOP wisdom. Finally, the BinaryExpressionImpl could simply not implement HasTypeDependencies, but conceptially, that would be strange. As a reader, I would expect a binary expression to have type dependencies.
To summaries, I feel that, either, we should get rid of HasTypeDependencies entirely and move everything here, or move the implementation of getTypeDependencies() for BinaryExpression to BinaryExpressionImpl (along with the SAME_TYPE_PRODUCING_BINARY_EXPRESSION_KINDS)
There was a problem hiding this comment.
Actually, having thought about this a bit more, maybe TypeDependenciesCalculator could be a TreeVisistor, which calculates the dependencies of a given Tree
There was a problem hiding this comment.
Tree visitor is not needed here (discussed)
| } else if (e instanceof HasTypeDependencies hasTypeDependencies) { | ||
| workList.addAll(hasTypeDependencies.typeDependencies()); | ||
| } else if (typeDependenciesCalculator.hasTypeDependencies(e)) { | ||
| workList.addAll(typeDependenciesCalculator.getTypeDependencies(e)); |
There was a problem hiding this comment.
TypeInference.Propagation#computeDependencies(...) also calls HasTypeDependencies#typeDependencies(). It probably should also use the TypeDependenciesCalculator
There was a problem hiding this comment.
We don't want to affect the v1 type inference
| @ParameterizedTest | ||
| @MethodSource("binaryExpressionsSource") | ||
| void binaryExpressionTest(String code, PythonType expectedType) { | ||
| assertThat(lastExpression(code).typeV2()).isInstanceOf(ObjectType.class).extracting(PythonType::unwrappedType).isEqualTo(expectedType); |
There was a problem hiding this comment.
It would be nice to have negative tests. For example to check if 1 + 2.0 is UNKNOWN.
|
SONARPY-2366