SONARPY-2313 Fix FN on S6543 when a generic class is defined in another file #2162
Conversation
guillaume-dequenne-sonarsource
force-pushed
the
SONARPY-2313
branch
15 times, most recently
from
bdaac22 to
b49d901
Compare
guillaume-dequenne-sonarsource
changed the title
guillaume-dequenne-sonarsource
requested a review
from thomas-serre-sonarsource
| @@ -1,6 +1,6 @@ | |||
| from typing import Any | |||
|
|
|||
| class ParentClass(object): | |||
| class ParentClass[T](object): | |||
There was a problem hiding this comment.
Note: the issues were initially not raised due to ParentClass not being a proper generic class.
| class MyImportedChild(SomeGeneric[T]): ... | ||
| class MyImportedChild2(SomeGeneric): ... | ||
| class MyImportedChild3(SomeGeneric[str]): ... |
There was a problem hiding this comment.
nit I think it is usually clearer to give more explicit names to show the intent of the class you create even in tests.
| class MyImportedChild(SomeGeneric[T]): ... | |
| class MyImportedChild2(SomeGeneric): ... | |
| class MyImportedChild3(SomeGeneric[str]): ... | |
| class MyImportedGenericTypeVarChild(SomeGeneric[T]): ... | |
| class MyImportedGenericChild(SomeGeneric): ... | |
| class MyImportedConcreteChild(SomeGeneric[str]): ... |
There was a problem hiding this comment.
I renamed MyImportedChild2 to MyImportedNonGenericChild (negated) to be more explicit about the fact it's no longer a generic class.
| String annotatedType = from.annotatedType(); | ||
| if (annotatedType != null) { | ||
| if ("typing._SpecialForm".equals(annotatedType) && fullyQualifiedName != null) { | ||
| // Defensive null check on fullyQualifiedName: it should never be null for SpecialForm |
| if (pythonType instanceof ClassType classType && classType.isGeneric()) { | ||
| return TriBool.TRUE; | ||
| } | ||
| if (pythonType instanceof SpecialFormType specialFormType) { | ||
| return "typing.Generic".equals(specialFormType.fullyQualifiedName()) ? TriBool.TRUE : TriBool.UNKNOWN; | ||
| } | ||
| if (pythonType instanceof UnknownType.UnresolvedImportType unresolvedImportType && "typing.Generic".equals(unresolvedImportType.importPath())) { | ||
| return TriBool.TRUE; | ||
| } | ||
| return TriBool.UNKNOWN; |
There was a problem hiding this comment.
this function never returns false. Is it a conservative choice? For example, It seems paradoxical for ClassType to have isGeneric false and to return UNKNOWN here.
There was a problem hiding this comment.
Yes, it's conservative. I think that isGeneric in ClassType should probably be a TriBool in fact, as what I want to express is that isGeneric=true means that it's for sure a generic class, and false just means there's no sign that it is.
I don't think it should have an impact until we actually want to raise issues on explicitly non generic classes, where this heuristic will have to be thought about more deeply.
guillaume-dequenne-sonarsource
force-pushed
the
SONARPY-2313
branch
2 times, most recently
from
b23cce7 to
dd77158
Compare
guillaume-dequenne-sonarsource
requested a review
from thomas-serre-sonarsource
guillaume-dequenne-sonarsource
force-pushed
the
SONARPY-2313
branch
from
fc68d75 to
b5f24b4
Compare
| from genericTypeWithoutArgumentImported import ( | ||
| SomeGeneric, | ||
| SomeGenericWithTypeParam, | ||
| MyImportedGenericTypeVarChild, | ||
| MyImportedNonGenericChild, | ||
| MyImportedConcreteChild, | ||
| SomeGenericIncorrectlyDefined | ||
| ) |
There was a problem hiding this comment.
may be?
| from genericTypeWithoutArgumentImported import ( | |
| SomeGeneric, | |
| SomeGenericWithTypeParam, | |
| MyImportedGenericTypeVarChild, | |
| MyImportedNonGenericChild, | |
| MyImportedConcreteChild, | |
| SomeGenericIncorrectlyDefined | |
| ) | |
| from genericTypeWithoutArgumentImported import * |
There was a problem hiding this comment.
Since support of wildcard import is brittle at the moment, I don't want to add confusion to this test: I prefer to keep the explicit import.
guillaume-dequenne-sonarsource
force-pushed
the
SONARPY-2313
branch
3 times, most recently
from
98694cc to
bf2b80c
Compare
…s hierarchy with generic classes
guillaume-dequenne-sonarsource
force-pushed
the
SONARPY-2313
branch
from
bf2b80c to
f627c32
Compare
|
This PR introduces the notion of a
SpecialFormType.This kind of type is a special typing internal type used to mark typing constructs (such as
typing.Union,typing.Optional, etc...).In
typing.pyi, these constructs are defined as:Representing these constructs as instances of the
ClassTypeSpecialFormis problematic for two reasons:ObjectTypeinstances ofSpecialForm. Disambiguating them would be difficult (relying onattributes, which is initially not suited to store simple names).Therefore, I believe introducing this special kind of type is simpler to circumvent these problems.