Skip to content

SONARPY-618 Introduce Ambiguous Symbol #663

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
andrea-guarino-sonarsource merged 2 commits into from
Apr 2, 2020
Merged

SONARPY-618 Introduce Ambiguous Symbol #663

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c07d290
Add Symbol#is() API
andreaguarino Mar 30, 2020
abb416f
SONARPY-618 Introduce Ambiguous Symbol
andreaguarino Mar 30, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions its/ruling/src/test/resources/expected/python-S2638.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,9 @@
'project:django-cms-3.7.1/cms/tests/test_permmod.py':[
665,
],
'project:numpy-1.16.4/numpy/distutils/fcompiler/pg.py':[
131,
],
'project:numpy-1.16.4/numpy/ma/core.py':[
6170,
],
Expand Down Expand Up @@ -161,6 +164,10 @@
'project:twisted-12.1.0/twisted/web/_newclient.py':[
515,
],
'project:twisted-12.1.0/twisted/web/client.py':[
669,
669,
],
'project:twisted-12.1.0/twisted/web/sux.py':[
193,
],
Expand Down
10 changes: 9 additions & 1 deletion python-checks/src/main/java/org/sonar/python/checks/AbstractUnreadPrivateMembersCheck.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
import java.util.Optional;
import org.sonar.plugins.python.api.PythonSubscriptionCheck;
import org.sonar.plugins.python.api.SubscriptionContext;
import org.sonar.plugins.python.api.symbols.AmbiguousSymbol;
import org.sonar.plugins.python.api.symbols.Symbol;
import org.sonar.plugins.python.api.symbols.Usage;
import org.sonar.plugins.python.api.tree.ClassDef;
Expand All @@ -37,11 +38,18 @@ public void initialize(Context context) {
context.registerSyntaxNodeConsumer(CLASSDEF, ctx -> {
ClassDef classDef = (ClassDef) ctx.syntaxNode();
Optional.ofNullable(getClassSymbolFromDef(classDef)).ifPresent(classSymbol -> classSymbol.declaredMembers().stream()
.filter(s -> s.name().startsWith(memberPrefix) && !s.name().endsWith("__") && s.kind() == kind() && isNeverRead(s))
.filter(s -> s.name().startsWith(memberPrefix) && !s.name().endsWith("__") && equalsToKind(s) && isNeverRead(s))
.forEach(symbol -> reportIssue(ctx, symbol)));
});
}

private boolean equalsToKind(Symbol symbol) {
if (symbol.kind().equals(Symbol.Kind.AMBIGUOUS)) {
return ((AmbiguousSymbol) symbol).alternatives().stream().allMatch(s -> s.kind() == kind());
}
return symbol.kind() == kind();
}

private void reportIssue(SubscriptionContext ctx, Symbol symbol) {
PreciseIssue preciseIssue = null;
for (int i = 0; i < symbol.usages().size(); i++) {
Expand Down
26 changes: 26 additions & 0 deletions python-frontend/src/main/java/org/sonar/plugins/python/api/symbols/AmbiguousSymbol.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
/*
* SonarQube Python Plugin
* Copyright (C) 2011-2020 SonarSource SA
* mailto:info AT sonarsource DOT com
*
* This program is free software; you can redistribute it and/or
* modify it under the terms of the GNU Lesser General Public
* License as published by the Free Software Foundation; either
* version 3 of the License, or (at your option) any later version.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
* Lesser General Public License for more details.
*
* You should have received a copy of the GNU Lesser General Public License
* along with this program; if not, write to the Free Software Foundation,
* Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
*/
package org.sonar.plugins.python.api.symbols;

import java.util.Set;

public interface AmbiguousSymbol extends Symbol {
Set<Symbol> alternatives();
}
3 changes: 3 additions & 0 deletions python-frontend/src/main/java/org/sonar/plugins/python/api/symbols/Symbol.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,11 +31,14 @@ public interface Symbol {
@CheckForNull
String fullyQualifiedName();

boolean is(Kind... kinds);

Kind kind();

enum Kind {
FUNCTION,
CLASS,
AMBIGUOUS,
OTHER
}
}
67 changes: 67 additions & 0 deletions python-frontend/src/main/java/org/sonar/python/semantic/AmbiguousSymbolImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
/*
* SonarQube Python Plugin
* Copyright (C) 2011-2020 SonarSource SA
* mailto:info AT sonarsource DOT com
*
* This program is free software; you can redistribute it and/or
* modify it under the terms of the GNU Lesser General Public
* License as published by the Free Software Foundation; either
* version 3 of the License, or (at your option) any later version.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
* Lesser General Public License for more details.
*
* You should have received a copy of the GNU Lesser General Public License
* along with this program; if not, write to the Free Software Foundation,
* Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
*/
package org.sonar.python.semantic;

import java.util.Collections;
import java.util.Objects;
import java.util.Set;
import java.util.stream.Collectors;
import javax.annotation.Nullable;
import org.sonar.plugins.python.api.symbols.AmbiguousSymbol;
import org.sonar.plugins.python.api.symbols.Symbol;

public class AmbiguousSymbolImpl extends SymbolImpl implements AmbiguousSymbol {

private final Set<Symbol> symbols;

private AmbiguousSymbolImpl(String name, @Nullable String fullyQualifiedName, Set<Symbol> symbols) {
super(name, fullyQualifiedName);
setKind(Kind.AMBIGUOUS);
this.symbols = symbols;
}

public static AmbiguousSymbol create(Set<Symbol> symbols) {
if (symbols.size() < 2) {
throw new IllegalArgumentException("Ambiguous symbol should contain at least two symbols");
}
Symbol firstSymbol = symbols.iterator().next();
if (!symbols.stream().map(Symbol::name).allMatch(symbolName -> symbolName.equals(firstSymbol.name()))) {
throw new IllegalArgumentException("Ambiguous symbol should contain symbols with the same name");
}
if (!symbols.stream().map(Symbol::fullyQualifiedName).allMatch(fqn -> Objects.equals(firstSymbol.fullyQualifiedName(), fqn))) {
return new AmbiguousSymbolImpl(firstSymbol.name(), null, symbols);
}
return new AmbiguousSymbolImpl(firstSymbol.name(), firstSymbol.fullyQualifiedName(), symbols);
}

@Override
public Set<Symbol> alternatives() {
return symbols;
}

@Override
AmbiguousSymbolImpl copyWithoutUsages() {
Set<SymbolImpl> copiedAlternativeSymbols = symbols.stream()
.map(SymbolImpl.class::cast)
.map(SymbolImpl::copyWithoutUsages)
.collect(Collectors.toSet());
return ((AmbiguousSymbolImpl) create(Collections.unmodifiableSet(copiedAlternativeSymbols)));

@pynicolas pynicolas Mar 31, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we call setSymbolsByDeclarationTree?

}
}
25 changes: 16 additions & 9 deletions python-frontend/src/main/java/org/sonar/python/semantic/Scope.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
import javax.annotation.CheckForNull;
import javax.annotation.Nullable;
import org.sonar.plugins.python.api.PythonFile;
import org.sonar.plugins.python.api.symbols.AmbiguousSymbol;
import org.sonar.plugins.python.api.symbols.ClassSymbol;
import org.sonar.plugins.python.api.symbols.FunctionSymbol;
import org.sonar.plugins.python.api.symbols.Symbol;
Expand Down Expand Up @@ -65,13 +66,7 @@ void createBuiltinSymbol(String name, Map<String, Symbol> typeShedSymbols) {
SymbolImpl symbol;
Symbol typeShedSymbol = typeShedSymbols.get(name);
if (typeShedSymbol != null) {
if (typeShedSymbol.kind() == Symbol.Kind.CLASS) {
symbol = ((ClassSymbolImpl) typeShedSymbol).copyWithoutUsages();
} else if (typeShedSymbol.kind() == Symbol.Kind.FUNCTION) {
symbol = ((FunctionSymbolImpl) typeShedSymbol).copyWithoutUsages();
} else {
symbol = new SymbolImpl(typeShedSymbol.name(), typeShedSymbol.fullyQualifiedName());
}
symbol = ((SymbolImpl) typeShedSymbol).copyWithoutUsages();
} else {
symbol = new SymbolImpl(name, name);
}
Expand Down Expand Up @@ -116,9 +111,9 @@ void addFunctionSymbol(FunctionDef functionDef, @Nullable String fullyQualifiedN
}

private static Symbol copySymbol(String symbolName, Symbol symbol, Map<String, Symbol> globalSymbolsByFQN) {
if (symbol.kind() == Symbol.Kind.FUNCTION) {
if (symbol.is(Symbol.Kind.FUNCTION)) {
return new FunctionSymbolImpl(symbolName, (FunctionSymbol) symbol);
} else if (symbol.kind() == Symbol.Kind.CLASS) {
} else if (symbol.is(Symbol.Kind.CLASS)) {
ClassSymbolImpl classSymbol = new ClassSymbolImpl(symbolName, symbol.fullyQualifiedName());
for (Symbol originalSymbol : ((ClassSymbol) symbol).superClasses()) {
Symbol globalSymbol = globalSymbolsByFQN.get(originalSymbol.fullyQualifiedName());
Expand All @@ -133,6 +128,11 @@ private static Symbol copySymbol(String symbolName, Symbol symbol, Map<String, S
.map(m -> ((SymbolImpl) m).copyWithoutUsages())
.collect(Collectors.toList()));
return classSymbol;
} else if (symbol.is(Symbol.Kind.AMBIGUOUS)) {
Set<Symbol> alternativeSymbols = ((AmbiguousSymbol) symbol).alternatives().stream()
.map(s -> copySymbol(s.name(), s, globalSymbolsByFQN))
.collect(Collectors.toSet());
return AmbiguousSymbolImpl.create(alternativeSymbols);
}
return new SymbolImpl(symbolName, symbol.fullyQualifiedName());
}
Expand Down Expand Up @@ -219,4 +219,11 @@ void addClassSymbol(ClassDef classDef, @Nullable String fullyQualifiedName) {
classSymbol.addUsage(classDef.name(), Usage.Kind.CLASS_DECLARATION);
}
}

void replaceSymbolWithAmbiguousSymbol(Symbol symbol, AmbiguousSymbol ambiguousSymbol) {
symbols.remove(symbol);
symbols.add(ambiguousSymbol);
symbolsByName.remove(symbol.name());
symbolsByName.put(symbol.name(), ambiguousSymbol);
}
}
11 changes: 11 additions & 0 deletions python-frontend/src/main/java/org/sonar/python/semantic/SymbolImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,17 @@ public String fullyQualifiedName() {
return fullyQualifiedName;
}

@Override
public boolean is(Kind... kinds) {
Kind symbolKind = kind();
for (Kind kindIter : kinds) {
if (symbolKind == kindIter) {
return true;
}
}
return false;
}

@Override
public Kind kind() {
return this.kind;
Expand Down
59 changes: 59 additions & 0 deletions python-frontend/src/main/java/org/sonar/python/semantic/SymbolTableBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@
import javax.annotation.CheckForNull;
import javax.annotation.Nullable;
import org.sonar.plugins.python.api.PythonFile;
import org.sonar.plugins.python.api.symbols.AmbiguousSymbol;
import org.sonar.plugins.python.api.symbols.ClassSymbol;
import org.sonar.plugins.python.api.symbols.Symbol;
import org.sonar.plugins.python.api.symbols.Usage;
Expand Down Expand Up @@ -132,13 +133,71 @@ public void visitFileInput(FileInput fileInput) {
scopesByRootTree = new HashMap<>();
fileInput.accept(new FirstPhaseVisitor());
fileInput.accept(new SecondPhaseVisitor());
createAmbiguousSymbols();

@pynicolas pynicolas Mar 31, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like changing symbols at this point. It seems very late. What are the risks that we end up with a graph of symbols which doesn't make sense? Aren't we changing symbols which are referenced by other symbols?

addSymbolsToTree((FileInputImpl) fileInput);
fileInput.accept(new ThirdPhaseVisitor());
if (!SymbolUtils.isTypeShedFile(pythonFile)) {
TypeInference.inferTypes(fileInput);
}
}

private static class SymbolToUpdate {
final Symbol symbol;
final AmbiguousSymbol ambiguousSymbol;

SymbolToUpdate(Symbol symbol, AmbiguousSymbol ambiguousSymbol) {
this.symbol = symbol;
this.ambiguousSymbol = ambiguousSymbol;
}
}

private void createAmbiguousSymbols() {
for (Scope scope : scopesByRootTree.values()) {
Set<SymbolToUpdate> symbolsToUpdate = new HashSet<>();
for (Symbol symbol : scope.symbols()) {
if (symbol.kind() == Symbol.Kind.OTHER) {
List<Usage> bindingUsages = symbol.usages().stream().filter(Usage::isBindingUsage).collect(Collectors.toList());
if (bindingUsages.size() > 1 &&
bindingUsages.stream().anyMatch(usage -> usage.kind() == Usage.Kind.FUNC_DECLARATION || usage.kind() == Usage.Kind.CLASS_DECLARATION)) {

Set<Symbol> alternativeDefinitions = getAlternativeDefinitions(symbol, bindingUsages);
AmbiguousSymbol ambiguousSymbol = AmbiguousSymbolImpl.create(alternativeDefinitions);
// update symbol and usage to newly created ambiguous symbol
symbol.usages().forEach(usage -> ((SymbolImpl) ambiguousSymbol).addUsage(usage.tree(), usage.kind()));
symbolsToUpdate.add(new SymbolToUpdate(symbol, ambiguousSymbol));
}
}
}
symbolsToUpdate.forEach(symbolToUpdate -> scope.replaceSymbolWithAmbiguousSymbol(symbolToUpdate.symbol, symbolToUpdate.ambiguousSymbol));
}
}

private Set<Symbol> getAlternativeDefinitions(Symbol symbol, List<Usage> bindingUsages) {
Set<Symbol> alternativeDefinitions = new HashSet<>();
for (Usage bindingUsage : bindingUsages) {
switch (bindingUsage.kind()) {
case FUNC_DECLARATION:
FunctionDef functionDef = (FunctionDef) bindingUsage.tree().parent();
FunctionSymbolImpl functionSymbol = new FunctionSymbolImpl(functionDef, symbol.fullyQualifiedName(), pythonFile);
((FunctionDefImpl) functionDef).setFunctionSymbol(functionSymbol);
alternativeDefinitions.add(functionSymbol);
break;
case CLASS_DECLARATION:
ClassSymbolImpl classSymbol = new ClassSymbolImpl(symbol.name(), symbol.fullyQualifiedName());
ClassDef classDef = (ClassDef) bindingUsage.tree().parent();
resolveTypeHierarchy(classDef, classSymbol);
Scope classScope = scopesByRootTree.get(classDef);
classSymbol.addMembers(getClassMembers(classScope.symbolsByName, classScope.instanceAttributesByName));
alternativeDefinitions.add(classSymbol);
break;
default:
SymbolImpl alternativeSymbol = new SymbolImpl(symbol.name(), symbol.fullyQualifiedName());
alternativeDefinitions.add(alternativeSymbol);
}
}
return alternativeDefinitions;
}

private void addSymbolsToTree(FileInputImpl fileInput) {
for (Scope scope : scopesByRootTree.values()) {
if (scope.rootTree instanceof FunctionLike) {
Expand Down
11 changes: 11 additions & 0 deletions python-frontend/src/main/java/org/sonar/python/tree/FunctionDefImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
import java.util.stream.Stream;
import javax.annotation.CheckForNull;
import javax.annotation.Nullable;
import org.sonar.plugins.python.api.symbols.FunctionSymbol;
import org.sonar.plugins.python.api.tree.Decorator;
import org.sonar.plugins.python.api.tree.FunctionDef;
import org.sonar.plugins.python.api.tree.Name;
Expand Down Expand Up @@ -59,6 +60,7 @@ public class FunctionDefImpl extends PyTree implements FunctionDef {
private final boolean isMethodDefinition;
private final StringLiteral docstring;
private Set<Symbol> symbols = new HashSet<>();
private FunctionSymbol functionSymbol;

public FunctionDefImpl(List<Decorator> decorators, @Nullable Token asyncKeyword, Token defKeyword, Name name,
Token leftPar, @Nullable ParameterList parameters, Token rightPar, @Nullable TypeAnnotation returnType,
Expand Down Expand Up @@ -169,4 +171,13 @@ public List<Tree> computeChildren() {
return Stream.of(decorators, Arrays.asList(asyncKeyword, defKeyword, name, leftPar, parameters, rightPar, returnType, colon, newLine, indent, body, dedent))
.flatMap(List::stream).filter(Objects::nonNull).collect(Collectors.toList());
}

public void setFunctionSymbol(FunctionSymbol functionSymbol) {
this.functionSymbol = functionSymbol;
}

@CheckForNull
public FunctionSymbol functionSymbol() {
return functionSymbol;
}
}
24 changes: 16 additions & 8 deletions python-frontend/src/main/java/org/sonar/python/types/TypeShed.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@
import org.sonar.python.semantic.FunctionSymbolImpl;
import org.sonar.python.semantic.SymbolImpl;
import org.sonar.python.semantic.SymbolTableBuilder;
import org.sonar.python.tree.FunctionDefImpl;
import org.sonar.python.tree.PythonTreeMaker;

import static org.sonar.plugins.python.api.types.BuiltinTypes.NONE_TYPE;
Expand All @@ -66,25 +67,32 @@ public static Map<String, Symbol> builtinSymbols() {
for (Symbol globalVariable : fileInput.globalVariables()) {
builtins.put(globalVariable.fullyQualifiedName(), globalVariable);
}
TypeShed.builtins = Collections.unmodifiableMap(builtins);
BaseTreeVisitor visitor = new BaseTreeVisitor() {
@Override
public void visitFunctionDef(FunctionDef functionDef) {
TypeAnnotation returnTypeAnnotation = functionDef.returnTypeAnnotation();
Optional.ofNullable(functionDef.name().symbol()).ifPresent(symbol -> {
if (symbol.kind() == Symbol.Kind.FUNCTION && returnTypeAnnotation != null) {
FunctionSymbolImpl functionSymbol = (FunctionSymbolImpl) symbol;
functionSymbol.setDeclaredReturnType(InferredTypes.declaredType(returnTypeAnnotation, builtins));
}
});
Optional.ofNullable(functionDef.name().symbol()).ifPresent(symbol -> setDeclaredReturnType(symbol, functionDef));
super.visitFunctionDef(functionDef);
}
};
fileInput.accept(visitor);
TypeShed.builtins = Collections.unmodifiableMap(builtins);
}
return builtins;
}

private static void setDeclaredReturnType(Symbol symbol, FunctionDef functionDef) {
TypeAnnotation returnTypeAnnotation = functionDef.returnTypeAnnotation();
if (returnTypeAnnotation == null) {
return;
}
if (symbol.is(Symbol.Kind.FUNCTION)) {
FunctionSymbolImpl functionSymbol = (FunctionSymbolImpl) symbol;
functionSymbol.setDeclaredReturnType(InferredTypes.declaredType(returnTypeAnnotation, builtins));
} else if (symbol.is(Symbol.Kind.AMBIGUOUS)) {
Optional.ofNullable(((FunctionDefImpl) functionDef).functionSymbol()).ifPresent(functionSymbol -> setDeclaredReturnType(functionSymbol, functionDef));
}
}

// visible for testing
static Set<Symbol> typingModuleSymbols() {
Map<String, Symbol> typingPython3 = getModuleSymbols("3/typing.pyi", TYPING);
Expand Down
Loading