SONARPY-2310: Partly support wildcard import #2167
Conversation
joke1196
force-pushed
the
SONARPY-2310
branch
from
6ae107f to
017a351
Compare
| @@ -61,10 +62,15 @@ public TypeInferenceV2(TypeTable projectLevelTypeTable, PythonFile pythonFile, S | |||
| } | |||
|
|
|||
| public Map<SymbolV2, Set<PythonType>> inferTypes(FileInput fileInput) { | |||
| TrivialTypeInferenceVisitor trivialTypeInferenceVisitor = new TrivialTypeInferenceVisitor(projectLevelTypeTable, pythonFile, fullyQualifiedModuleName); | |||
| WildcardImportVisitor wildcardImportVisitor = new WildcardImportVisitor(projectLevelTypeTable); | |||
There was a problem hiding this comment.
Why would this need to be its dedicated visitor?
I know we're going for a naive implementation, but I think it's still sensible to assume that we don't want the following resolution:
foo()
from lib import * # contains foo
I'm under the impression that relying on the main visit of TrivialTypeInferenceVisitor should be enough here, no?
There was a problem hiding this comment.
The TrivialTypeInferenceVisistor is enough (initially that's what we did). Our thought was that if it is moved to its own visitor, it is easier to rip it out when we eventually will do that. But we didn't think of the example you provided
Seppli11
force-pushed
the
SONARPY-2310
branch
5 times, most recently
from
ecf4705 to
82abc6d
Compare
Seppli11
force-pushed
the
SONARPY-2310
branch
from
82abc6d to
beaa712
Compare
Seppli11
changed the title
|
| assertThat(fooExpr.typeV2()).isInstanceOf(FunctionType.class); | ||
| assertThat(importedFooExpr.typeV2()).isInstanceOf(FunctionType.class); |
There was a problem hiding this comment.
Can you add an assertion checking whether these are the same objects?
| import mod2 | ||
| (foo, mod1.foo, mod2.foo) | ||
| """), | ||
| // foo should be mod2.foo, not mod1.foo. Since wildcard imports are only assigned to names without a symbol, foo will be equal to mod1.foo. See SONARPY-2357 |
There was a problem hiding this comment.
Good! I think this is a reasonable behavior for now.
As a nitpick, I'd rename the Jira ticket to something a bit more explicit/neutral (Something like: Support edge cases of wildcard imports resolution?)
| @@ -3124,4 +3262,25 @@ private static Statement lastStatement(StatementList statementList) { | |||
| List<Statement> statements = statementList.statements(); | |||
| return statements.get(statements.size() - 1); | |||
| } | |||
|
|
|||
| private static class TestProject { | |||
There was a problem hiding this comment.
That's a pretty neat construct!
I'm wondering if it wouldn't make more sense for this to be in a different test category though (either ProjectLevelSymbolTableTest or ProjectLevelTypeTableTest, or even a new CrossFileTypeInferenceTest dedicated to tests focused on import resolution.
We probably can keep this here for now though.
SONARPY-2310