SONARPY-957 Protobuf Typeshed should serialize information about variables#1041
Conversation
andrea-guarino-sonarsource
changed the title
andrea-guarino-sonarsource
force-pushed
the
MMF-2578
branch
from
a20de92 to
9bf3e19
Compare
andrea-guarino-sonarsource
force-pushed
the
SONARPY-957
branch
from
193725e to
4f069f4
Compare
guillaume-dequenne
left a comment
guillaume-dequenne
left a comment
There was a problem hiding this comment.
Looks good, but I think this highlights an incomplete handling for type classes from MyPy (which is my bad really...) and we should probably do something about it to avoid risking some unwanted behavior for those classes.
| def __init__(self, _type: mpt.Type): | ||
| self.args = [] | ||
| self.fully_qualified_name = None | ||
| self.kind = None |
There was a problem hiding this comment.
If you had to add this, I assume that means that the final else on line 132 can actually be triggered. As this shows, it wasn't a robust piece of code, but back when I wrote this I actually assumed this condition could not be triggered because all "common" type classes encountered in TypeShed were covered (and I just wanted to avoid a failure if it actually wasn't the case due to some unsupported edge case).
Do you know on which case it happens? Maybe the comment on line 133 can be updated to mention when it is triggered, or maybe there are other type classes we can actually handle here.
EDIT: At least one case where it seems to happen is when setting an alias for an overloaded symbol. For example in gettext.pyi I see multiple overloaded definitions for the function translation, then a variable definition:
Catalog = translation
This ultimately means we serialize variable tranlsation to have a type annotation such as:
type_annotation {
pretty_printed_name: "#Unknown"
}
Maybe we shouldn't do that and just not serialize any type annotation? Or maybe from the actual type class we see from mypy, we can infer this is an overloaded function and actually serialize that (I see the type class Overload actually exist in mypy).
There was a problem hiding this comment.
Good point! I added self.kind = None because indeed I had trouble with variables pointing to aliases of overloaded symbols and I then forgot to handle the "#Unknown" types.
I think we can just avoid to serialize any type annotation in that case, and maybe improve the logic and try to get the actual type class in another ticket / PR.
There was a problem hiding this comment.
Could you create a ticket for that? I don't think it major but I guess it would provide a small precision boost in some cases.
| this.name = varSymbol.getName(); | ||
| this.fullyQualifiedName = TypeShed.normalizedFqn(varSymbol.getFullyQualifiedName()); | ||
| String fqn = varSymbol.getTypeAnnotation().getFullyQualifiedName(); | ||
| if (!fqn.isEmpty()) { |
There was a problem hiding this comment.
Related to the previous comment, what happens here on a serialized type name that is #Unknown?
There was a problem hiding this comment.
I think we can prevent this to happen in the serializer. We could also add here a check that would throw an exception if type name is #Unknown
There was a problem hiding this comment.
Thinking again about this: #Unknown is the pretty_printed_name which we currently don't really use in the deserializer. The fqn instead should never contain #Unknown.
andrea-guarino-sonarsource
force-pushed
the
SONARPY-957
branch
from
d012c65 to
409a97c
Compare
|
SonarQube Quality Gate
|
GitOrigin-RevId: 2e7e792b13b4a267ce09121e77919afb10120b94
3 participants
No description provided.