SONARPY-870 Use serialized version of TypeShed core modules (builtins and its dependencies) by andrea-guarino-sonarsource · Pull Request #940 · SonarSource/sonar-python

andrea-guarino-sonarsource · 2021-07-13T11:55:52Z

No description provided.

… and its dependencies)

guillaume-dequenne

LGTM overall.
Left a few questions/suggestions here and there.

guillaume-dequenne · 2021-07-20T08:21:20Z

    return null;
  }
+
+  public Set<String> validFor() {


I've been using this name in the context of serialization of merged symbols but I realize it might not be explicit once the context is missing.
Do you think we should try to come up with a better name? (validForPythonVersions or something like that...)

guillaume-dequenne · 2021-07-20T08:41:10Z

+340,
+341,
+342,
+343,
+344,


I think these issues are FPs. The LHS operator is indeed is supposed to be bytes and thus unsuitable for identity comparisons, but I think the rule should make exceptions for is not None cases.

I think it's not that urgent to fix because this is a problem only when there is a TP reported by S5727, but it's a bit noisy. (and if we ever have FPs on S5727, it would be extra noisy). Could you create a ticket for that? (and if we have time during the sprint I don't think it would take too much time to address).

Ticket created

guillaume-dequenne · 2021-07-20T09:19:46Z

+   * This method sort ambiguous symbol by python version and returns the one which is valid for
+   * the most recent Python version.
+   */
+  private static Symbol disambiguateWithLatestPythonSymbol(Set<Symbol> alternatives) {


nitpick: I think this could be done more simply in a single for loop where you only keep the symbol corresponding to the max validFor value encountered.
It's not like it will have any real performance impact so feel free to keep things this way if you think it's more readable.

guillaume-dequenne · 2021-07-20T09:27:32Z

+    Arrays.asList(INT, FLOAT, COMPLEX, STR, BuiltinTypes.SET, DICT, LIST, TUPLE, NONE_TYPE, BOOL, "type", "super", "frozenset", "memoryview"));
+
+  static {
+    BUILTINS_TO_DISAMBIGUATE.addAll(BuiltinSymbols.EXCEPTIONS);


I'm surprised to see we still rely on this BuiltinSymbols class for quite some things.

It's out of the scope of this PR but do you think we should consider getting rid of it and possibly generate it from TypeShed (through separate serialization of a list of all encountered builtin names for example) ?

We're not really maintaining it so it might be out of sync with latest Python versions (not sure many builtins were added, if any, though - but still).

If you agree, maybe we can create some ticket in our backlog to address this at some point?

Yes I think it makes sense to have a ticket to remove this class

Ticket created.

guillaume-dequenne · 2021-07-20T09:53:02Z

+    Map<String, Symbol> builtinSymbols = builtinSymbols();
+    Symbol builtinSymbol = builtinSymbols.get(fullyQualifiedName);
+    if (builtinSymbol != null) {
+      return builtinSymbol;
+    }
    String[] fqnSplittedByDot = fullyQualifiedName.split("\\.");
    String localName = fqnSplittedByDot[fqnSplittedByDot.length - 1];
-    if (fqnSplittedByDot.length == 2 && fqnSplittedByDot[0].equals("builtins") && builtins.containsKey(localName)) {
-      return builtins.get(localName);
+    if (fqnSplittedByDot.length == 2 && fqnSplittedByDot[0].equals(BUILTINS_FQN) && builtinSymbols.containsKey(localName)) {
+      return builtinSymbols.get(localName);
    }


If I understand correctly, those two if are meant to try and retrieve the builtin symbol matching the given FQN.
Perhaps the two conditions can be simplified into one by calling normalizedFQN before. Wdyt?

guillaume-dequenne · 2021-07-20T10:04:56Z

+
+  @Override
+  public Set<String> validFor() {
+    return alternatives().stream().flatMap(symbol -> ((SymbolImpl) symbol).validFor().stream()).collect(Collectors.toSet());


With the current implementation, I think it's hard to understand whether an ambiguous symbol is ambiguous due to having different definitions depending on the Python version or if it's an overloaded function/method.

In the latter case, I think it would make sense to actually store validFor as a field for the whole ambiguous symbol. Maybe we could store an isOverloaded field as well instead. I don't think it's really needed, though.

As you said, I don't think we really need to know whether an ambiguous symbol is due to multiple Python version or not. Do you have some usecase in mind?

guillaume-dequenne · 2021-07-20T11:11:55Z

+    Symbol memoryview = TypeShed.builtinSymbols().get("memoryview");
+    InferredType memoryView = InferredTypes.runtimeType(memoryview);


I don't understand this change. Above, we keep using runtimeBuiltinType for unicode and it seems to be doing the same thing. I don't have an opinion on which approach to keep but why have two?

Indeed, I forgot to remove this change!

guillaume-dequenne · 2021-07-20T11:40:32Z

    this.supportsGenerics = supportsGenerics;
  }
+
+  public boolean hasResolvedSuperClasses() {


Intuitively from its name, this method would return !hasUnresolvedTypeHierarchy in my mind.

I'm not sure I understand it very well because to me it might still return true if there are no super classes at all or if they are unresolved. Am I missing something?

This method is used to know whether super classes have been already computed of if the symbol is still "lazy".
superClassesFqns.isEmpty() condition is used to know wheter the symbol is from typeshed or not. (i.e. if superClassesFqns is empty, we don't care too much if the symbol is lazy or not)
Maybe hasComputedSuperClasses would be clearer? or do you have better suggestions?

guillaume-dequenne · 2021-07-20T11:52:03Z

+        } else if (superClass.kind() == Kind.CLASS) {
+          copiedClassSymbol.superClasses.add(((ClassSymbolImpl) superClass).copyWithoutUsages());
+        } else if (superClass.is(Kind.AMBIGUOUS)) {
+          copiedClassSymbol.superClasses.add(((AmbiguousSymbolImpl) superClass).copyWithoutUsages());


Actually why are we keeping two branches here? I know this was done this way before but we could merge them and cast to SymbolImpl instead I think?

guillaume-dequenne · 2021-07-20T12:46:52Z

+    this(functionSymbolProto, insideClass, functionSymbolProto.getValidForList());
+  }
+
+  public FunctionSymbolImpl(SymbolsProtos.FunctionSymbol functionSymbolProto, boolean insideClass, List<String> validFor) {


It's a bit weird that we need this validFor parameter, to me the SymbolsProtos.FunctionSymbol should probably contain it, even in case of overloaded method/function.
It might make the protobuf serialization a bit heavier for no gain though, so I'm not entirely sure it's worth doing. Wdyt?

I would prefer it to be done in the serialization too.
I think for the time being we can keep it in the deserialization, and maybe change this in another PR

sonarsource-next · 2021-07-21T08:47:17Z

SonarQube Quality Gate

0 Bugs
0 Vulnerabilities
0 Security Hotspots
2 Code Smells

98.9% Coverage
0.0% Duplication

guillaume-dequenne

LGTM

… and its dependencies) (#940)

andrea-guarino-sonarsource force-pushed the SONARPY-870 branch 3 times, most recently from f7df02a to 3eb4633 Compare July 15, 2021 22:29

SONARPY-870 Add builtins protobuf and its dependencies as resources

e1d7598

andrea-guarino-sonarsource force-pushed the SONARPY-870 branch 2 times, most recently from 1cd43be to 9738c85 Compare July 19, 2021 10:30

andrea-guarino-sonarsource marked this pull request as ready for review July 19, 2021 12:41

SONARPY-870 Use serialized version of TypeShed core modules (builtins…

a21dfde

… and its dependencies)

andrea-guarino-sonarsource force-pushed the SONARPY-870 branch from 9738c85 to a21dfde Compare July 19, 2021 15:18

andrea-guarino-sonarsource requested a review from guillaume-dequenne July 19, 2021 15:46

andrea-guarino-sonarsource changed the title ~~Use serialized version of TypeShed core modules (builtins and its dependencies)~~ SONARPY-870 Use serialized version of TypeShed core modules (builtins and its dependencies) Jul 19, 2021

guillaume-dequenne requested changes Jul 20, 2021

View reviewed changes

andreaguarino added 2 commits July 20, 2021 17:10

fixes from review

47c0dda

Rename 'hasResolvedSuperClasses' and add documentation

d91584f

andrea-guarino-sonarsource requested a review from guillaume-dequenne July 21, 2021 09:06

guillaume-dequenne approved these changes Jul 21, 2021

View reviewed changes

guillaume-dequenne merged commit a3003fb into mmf-2435 Jul 21, 2021

guillaume-dequenne deleted the SONARPY-870 branch July 21, 2021 12:29

andrea-guarino-sonarsource added a commit that referenced this pull request Jul 21, 2021

SONARPY-870 Use serialized version of TypeShed core modules (builtins…

5b471db

… and its dependencies) (#940)

+,
+,
+,
+,
+,

		Symbol memoryview = TypeShed.builtinSymbols().get("memoryview");
		InferredType memoryView = InferredTypes.runtimeType(memoryview);

Conversation

andrea-guarino-sonarsource commented Jul 13, 2021

Uh oh!

guillaume-dequenne left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andrea-guarino-sonarsource Jul 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sonarsource-next Bot commented Jul 21, 2021

Uh oh!

guillaume-dequenne left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

andrea-guarino-sonarsource Jul 20, 2021 •

edited

Loading