SONARPY-2242 TypeInferenceV2 should resolve stubs with variable descriptors pointing to submodules #2094
Conversation
ghislainpiot
force-pushed
the
SONARPY-2242
branch
2 times, most recently
from
7a4d942
to
a3760b4
Compare
| @@ -253,4 +253,14 @@ class lib: ... | |||
| // SONARPY-2176 lib should be resolved as the renamed class "A" here | |||
| assertThat(aType.name()).isEqualTo("A"); | |||
| } | |||
|
|
|||
| @Test | |||
| void resolveCustomStub() { | |||
There was a problem hiding this comment.
I think it would make sense to reflect that it's about importedModule and the conflict between module members and submodules. The fact it happens in a custom stub is not really what's relevant here.
I'd suggest naming to resolveCustomStubWithImportedSubmodule or something similar to express that.
There was a problem hiding this comment.
(Note: I took the liberty to rename the ticket/PR title consistently with this, however feel free to further update it)
| if (from.isImportedModule()) { | ||
| var fqn = from.fullyQualifiedName(); | ||
| if (fqn != null) { | ||
| return ctx.lazyTypesContext().getOrCreateLazyType(fqn); |
There was a problem hiding this comment.
Genuine question: what did you decide to do about our discussion for LazyModuleType? Handle it in another ticket? Scrap it?
Not challenging to choice to keep things simple here, though. I think it makes sense (even though it's a shame we lose some information in the process).
There was a problem hiding this comment.
I tried the implementation with LazyModuleType, and it opened up some problematic discussions.
One example is if there is a LazyType and a ModuleLazyType with the same imported path, what happens ?
It also didn't help in the code, and the change was bigger for no real benefit (I didn't do profiling)
| @@ -67,10 +67,11 @@ public PythonType getType(List<String> typeFqnParts) { | |||
| if (parent instanceof ModuleType moduleType) { | |||
| TypeWrapper typeWrapper = moduleType.members().get(part); | |||
| if (typeWrapper instanceof LazyTypeWrapper lazyTypeWrapper && !lazyTypeWrapper.isResolved()) { | |||
| if (i == typeFqnParts.size() - 1) { | |||
| if (i == typeFqnParts.size() - 1 && !(lazyTypeWrapper.hasImportPath(String.join(".", typeFqnParts)))) { | |||
There was a problem hiding this comment.
Acknowledging the confusion when looked at this together, I'm thinking maybe we should extract this to another method, or at least add a tiny bit more info on the purpose of this in a comment (maybe updating the one on line 71).
guillaume-dequenne-sonarsource
changed the title
| @@ -29,6 +29,12 @@ | |||
| public class VariableDescriptorToPythonTypeConverter implements DescriptorToPythonTypeConverter { | |||
|
|
|||
| public PythonType convert(ConversionContext ctx, VariableDescriptor from) { | |||
| if (from.isImportedModule()) { | |||
| var fqn = from.fullyQualifiedName(); | |||
| if (fqn != null) { | |||
There was a problem hiding this comment.
I can do a mock to cover it, but I am not sure of the value.
| @@ -67,8 +67,8 @@ public PythonType getType(List<String> typeFqnParts) { | |||
| if (parent instanceof ModuleType moduleType) { | |||
| TypeWrapper typeWrapper = moduleType.members().get(part); | |||
| if (typeWrapper instanceof LazyTypeWrapper lazyTypeWrapper && !lazyTypeWrapper.isResolved()) { | |||
| if (i == typeFqnParts.size() - 1 && !(lazyTypeWrapper.hasImportPath(String.join(".", typeFqnParts)))) { | |||
| // this is the name we are looking for, resolve it | |||
| if (i == typeFqnParts.size() - 1 && !fqnSameAsImportedPath(typeFqnParts, lazyTypeWrapper)) { | |||
There was a problem hiding this comment.
I must admit, this is not really what I had in mind, fqnSameAsImportedPath and lazyTypeWrapper.hasImportPath(String.join(".", typeFqnParts)) read very similar and don't provide the extra clarity I expected.
What I had in mind was extracting the full condition (both parts of the &&) and expanding a bit more on the cases that can lead us to this condition. Basically explaining that we try to resolve a lazy type only if it's pointing us to a different module, and if it's going to point us on the same module again, we instead try to resolve a submodule of the same name instead of the member.
(Sorry, I realize my suggestion is a bit unclear, and this is probably why we need one in the first place)
ghislainpiot
force-pushed
the
SONARPY-2242
branch
from
6b04ff3
to
c5abe65
Compare
|
SONARPY-2242