SONARPY-1801 infer type of subscription expression when it is a class with generic type #1776
Conversation
maksim-grebeniuk-sonarsource
changed the base branch from
master
to
rnd/type-inference-engine-specification
maksim-grebeniuk-sonarsource
force-pushed
the
mg/SONARPY-1801
branch
2 times, most recently
from
0ad5542
to
b230d05
Compare
maksim-grebeniuk-sonarsource
requested a review
from guillaume-dequenne-sonarsource
maksim-grebeniuk-sonarsource
force-pushed
the
mg/SONARPY-1801
branch
from
b230d05
to
6bc838b
Compare
… with generic type
maksim-grebeniuk-sonarsource
force-pushed
the
mg/SONARPY-1801
branch
from
6bc838b
to
bd2b352
Compare
|
There was a problem hiding this comment.
I'm not sure about keeping this logic in the Tree itself despite the fact we did that in the previous implementation. I think it would make sense to do it during the visit of TypeInferenceV2 to centralize the places where types are set (I realize it's actually quite difficult to know where a type is set in the current implementation, as it can happen in many places...).
I'm also unsure about the edge cases we could face with this. Ideally it could be nice to have an extra test for a possible edge case that is actually not generic (if it exists0>
| @@ -77,4 +79,13 @@ public List<Tree> computeChildren() { | |||
| public Kind getKind() { | |||
| return Kind.SUBSCRIPTION; | |||
| } | |||
|
|
|||
| @Override | |||
| public PythonType typeV2() { | |||
There was a problem hiding this comment.
Instinctively, I believe typeV2() should ideally only be a getter for types.
I believe the logic of this method would make more sense in TypeInferenceV2 itself.
I see in the previous implementation this was done in CallExpressionImpl directly, using a similar logic, though. It did have some checks to ensure we were indeed dealing with a generic type, I think it would make sense to have it as well to avoid edge cases (I guess you could create a class where the class object itself has a __getitem__ method...
No description provided.