SONARPY-2209 Avoid duplicating re-exported stub symbols when deserializing stubs #2078
Conversation
guillaume-dequenne-sonarsource
force-pushed
the
SONARPY-2209
branch
2 times, most recently
from
7933a5b to
6a01a09
Compare
guillaume-dequenne-sonarsource
changed the title
guillaume-dequenne-sonarsource
force-pushed
the
SONARPY-2209
branch
from
6a01a09 to
0db1fed
Compare
guillaume-dequenne-sonarsource
force-pushed
the
SONARPY-2203
branch
from
1987926 to
f028b00
Compare
guillaume-dequenne-sonarsource
force-pushed
the
SONARPY-2209
branch
8 times, most recently
from
56f9eab to
56b5ace
Compare
guillaume-dequenne-sonarsource
requested review from
ghislainpiot and
maksim-grebeniuk-sonarsource
| var descriptor = entry.getValue(); | ||
| var name = entry.getKey(); | ||
| PythonType result; | ||
| String fullyQualifiedName = descriptor.fullyQualifiedName(); |
There was a problem hiding this comment.
If I understand correctly, this is the fullname from Mypy
There was a problem hiding this comment.
Yes, which represents the actual fully qualified name of the symbol (where it's defined, rather than where it's re-exposed).
Do you think it could be better named?
There was a problem hiding this comment.
It was just to be sure, I think it is strange that we didn't keep the name of the filed from Mypy
| if (parent instanceof ObjectType) { | ||
| return PythonType.UNKNOWN; | ||
| } | ||
| if (parent instanceof ModuleType moduleType) { | ||
| TypeWrapper typeWrapper = moduleType.members().get(part); | ||
| if (typeWrapper instanceof LazyTypeWrapper lazyTypeWrapper && !lazyTypeWrapper.isResolved()) { |
There was a problem hiding this comment.
Manually checking if a LazyType is not resolved makes it feel like a bandaid to me. I think properly representing submodules would be better, but perhaps outside the scope of the PR.
There was a problem hiding this comment.
That makes sense. I've added 13d2386 which adds a bit more intentionality, although the "LazyType is not resolved" heuristics is still used.
Ultimately, I think SONARPY-2176 should model a bit better the cases where a submodule or a member should be looked at in priority. I do think that the "LazyType is not resolved" is a case where resolving the submodule first is necessary (because the LazyType is apparently pointing to it - not resolving it would lead to a stack overflow), and we only tackle this one due to the crashes this avoids.
| public Optional<PythonType> resolveSubmodule(String submoduleName) { | ||
| return Optional.ofNullable(subModules.get(submoduleName)).map(TypeWrapper::type); |
There was a problem hiding this comment.
Is it expected that subModules is only used in test code? Not counting adding and retrieving from the parents, it never leaves the module types.
There was a problem hiding this comment.
This is a good point. I've added 13d2386 to instead consistently register submodules in the subModule field.
I've updated the strategy of resolveMember to first resolve actual members, then sub modules if no member is present. The behavior is not entirely accurate depending on the context, but I believe it is a decent enough approximation before we tackle SONARPY-2176.
| @@ -39,12 +40,43 @@ void resolveMemberTest() { | |||
| @Test | |||
| void replaceMemberIfUnknown() { | |||
| var a = new ModuleType("a"); | |||
| a.members().put("b", PythonType.UNKNOWN); | |||
| a.members().put("b", TypeWrapper.of(PythonType.UNKNOWN)); | |||
There was a problem hiding this comment.
nitpick: We have TypeWrapper.UNKNOWN_TYPE_WRAPPER
| @@ -30,23 +30,23 @@ | |||
| public final class ModuleType implements PythonType { | |||
| private final String name; | |||
| private final ModuleType parent; | |||
| private final Map<String, PythonType> members; | |||
| private final Map<String, TypeWrapper> members; | |||
| private final Map<String, TypeWrapper> subModules = new HashMap<>(); | |||
There was a problem hiding this comment.
I'd suggest to assign the field value in the same place where other fields are initialized, to have single place to look at
guillaume-dequenne-sonarsource
force-pushed
the
SONARPY-2209
branch
from
13d2386 to
04832ad
Compare
guillaume-dequenne-sonarsource
force-pushed
the
SONARPY-2209
branch
from
04832ad to
a959306
Compare
|
No description provided.