SONARPY-2288 Fix FP on S5795 when checking if type is unsuitable #2116
Conversation
There was a problem hiding this comment.
The change makes sense, and the underlying problem highlights still some weaknesses in our APIs.
I think it would be nice to move the custom logic to the type checker so that it can be later rethought in context, without needing to consider custom logic implemented in specific rules.
| @@ -147,7 +148,7 @@ private boolean isUnsuitableOperand(Expression expr) { | |||
| private boolean isUnsuitableType(PythonType type) { | |||
| for (String builtinName : NAMES_OF_TYPES_UNSUITABLE_FOR_COMPARISON) { | |||
| TypeCheckBuilder builtinWithNameChecker = typeChecker.typeCheckBuilder().isBuiltinWithName(builtinName); | |||
| if (builtinWithNameChecker.check(type) == TriBool.TRUE) { | |||
| if (type instanceof ObjectType && builtinWithNameChecker.check(type) == TriBool.TRUE) { | |||
There was a problem hiding this comment.
I feel that we shouldn't mix checks done by the type checker and by the rule itself.
In this case, you want to ensure the object is actually an object of a specific type.
The proper fix (that you can consider in the type checking API sprint), is in my opinion the possibility to check something like isObjectOfOf(PythonType expected) that checks that we have an object of a certain type, rather than the type itself. I feel it's something that will come up enough that it will deserve its own method to create the relevant predicates.
As for the immediate fix, I think you could introduce a isObject method in the type checker that perform the same check. Then, thanks to the natural ANDing of predicates, you'll be able to write:
TypeCheckBuilder isObjectOfBuiltinCheck = typeChecker.typeCheckBuilder().isObject().isBuiltinWithName(builtinName);
And simply check that for TRUE.
| @@ -156,3 +156,7 @@ def comparison_to_none(): | |||
| def potential_null_symbols(): | |||
| type(arr) is tuple | |||
| some_thing is other_thing | |||
|
|
|||
| def unknown_is_not_type(arg): | |||
There was a problem hiding this comment.
Does that method name make sense?
I guess the underlying problem is to not confuse a type object with an object of a given type?
There was a problem hiding this comment.
The name does not make sense. You're absolutely right with the reason why the FP is happening.
| record IsInstancePredicate() implements TypePredicate { | ||
|
|
||
| @Override | ||
| public TriBool test(PythonType pythonType) { |
There was a problem hiding this comment.
Because this is now in the frontend (and not checks), we need a dedicated unit test for this method.
|
SONARPY-2288
Part of