SONARPY-2245 Fix resolving type for a name if its type is UnresolvedImportType #2099
Conversation
maksim-grebeniuk-sonarsource
requested review from
guillaume-dequenne-sonarsource and
ghislainpiot
There was a problem hiding this comment.
Left some comments.
I would expect the // FAILS comment to be removed, and am tempted to say we should extract the custom logic in a dedicated method. I leave @ghislainpiot with the decision to approve as-is or not, though.
| var arg = ((RegularArgument) ((CallExpression) funcCall).arguments().get(0)); | ||
| var argType = arg.expression().typeV2(); | ||
|
|
||
| assertThat(argType).isInstanceOfSatisfying(UnresolvedImportType.class, a -> assertThat(a.importPath()).isEqualTo("a.b")); // FAILS |
There was a problem hiding this comment.
You copy pasted the test case in the ticket, but forgot to remove this FAILS comment.
| @@ -471,7 +471,7 @@ public void visitName(Name name) { | |||
| .map(Expression.class::cast) | |||
| .map(Expression::typeV2) | |||
| // TODO: classes (SONARPY-1829) and functions should be propagated like other types | |||
| .filter(t -> (t instanceof ClassType) || (t instanceof FunctionType) || (t instanceof ModuleType)) | |||
| .filter(t -> (t instanceof ClassType) || (t instanceof FunctionType) || (t instanceof ModuleType) || (t instanceof UnknownType.UnresolvedImportType)) | |||
There was a problem hiding this comment.
I'm wondering if we should extract this to a dedicated method that could be named something like shouldTypeBeEagerlyPropagated, to make this heuristic more intentional.
I have the feeling that we'll never fully remove this heuristic (even as we fix SONARPY-1829), because of the reasons mentioned in SONARPY-2245 (basically, we sometimes want to circumvent the actual Python propagations to ensure classes, functions and imports referenced in inner scopes are correctly resolved, even if they could technically be shadowed)
maksim-grebeniuk-sonarsource
force-pushed
the
SONARPY-2245
branch
from
7bba13e
to
bd28bef
Compare
|
SONARPY-2245