Skip to content

SONARPY-394 Migrate FieldNameCheck to strongly typed AST #360

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
merged 2 commits into from
Sep 25, 2019
Merged

SONARPY-394 Migrate FieldNameCheck to strongly typed AST #360

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
80bcbd3
Migrate FieldNameCheck to strongly typed AST
pynicolas Sep 24, 2019
ad18120
Refactor PythonTestUtils
benzonico Sep 25, 2019
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
4 changes: 0 additions & 4 deletions its/ruling/src/test/resources/expected/python-S116.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -526,7 +526,6 @@
'project:twisted-12.1.0/twisted/internet/task.py':[
63,
64,
112,
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand why this issue is missing and not relevant anymore : this is an instance field and should be detected as such.

Copy link
Contributor

Choose a reason for hiding this comment

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

And I figured it out : this self variable is instantiated via a class method. This is way out of scope of this sprint, so fine to accept this "loss" for now.

124,
374,
375,
Expand Down Expand Up @@ -573,7 +572,6 @@
212,
212,
224,
763,
],
'project:twisted-12.1.0/twisted/internet/test/test_tls.py':[
42,
Expand Down Expand Up @@ -1230,8 +1228,6 @@
],
'project:twisted-12.1.0/twisted/trial/test/test_tests.py':[
536,
537,
537,
538,
],
'project:twisted-12.1.0/twisted/trial/test/test_util.py':[
Expand Down
72 changes: 32 additions & 40 deletions python-checks/src/main/java/org/sonar/python/checks/FieldNameCheck.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,22 +19,21 @@
*/
package org.sonar.python.checks;

import com.sonar.sslr.api.AstNode;
import com.sonar.sslr.api.AstNodeType;
import com.sonar.sslr.api.Token;
import java.util.Collections;
import java.util.ArrayList;
import java.util.List;
import java.util.Set;
import java.util.regex.Pattern;
import java.util.stream.Collectors;
import org.sonar.check.Rule;
import org.sonar.check.RuleProperty;
import org.sonar.python.PythonCheckAstNode;
import org.sonar.python.api.PythonGrammar;
import org.sonar.python.PythonSubscriptionCheck;
import org.sonar.python.api.tree.PyClassDefTree;
import org.sonar.python.api.tree.Tree;
import org.sonar.python.semantic.TreeSymbol;
import org.sonar.python.semantic.Usage;

@Rule(key = FieldNameCheck.CHECK_KEY)
public class FieldNameCheck extends PythonCheckAstNode {

public static final String CHECK_KEY = "S116";
@Rule(key = "S116")
public class FieldNameCheck extends PythonSubscriptionCheck {

private static final String MESSAGE = "Rename this field \"%s\" to match the regular expression %s.";

Expand All @@ -44,41 +43,34 @@ public class FieldNameCheck extends PythonCheckAstNode {
@RuleProperty(key = "format", defaultValue = DEFAULT)
public String format = DEFAULT;

private Pattern pattern = null;
private Pattern constantPattern = null;

@Override
public Set<AstNodeType> subscribedKinds() {
return Collections.singleton(PythonGrammar.CLASSDEF);
}

@Override
public void visitNode(AstNode astNode) {
if (!CheckUtils.classHasInheritance(astNode)) {
List<Token> allFields = new NewSymbolsAnalyzer().getClassFields(astNode);
checkNames(allFields);
}
}

private void checkNames(List<Token> varNames) {
if (constantPattern == null) {
constantPattern = Pattern.compile(CONSTANT_PATTERN);
}
for (Token name : varNames) {
if (!constantPattern.matcher(name.getValue()).matches()) {
checkName(name);
public void initialize(Context context) {
Pattern pattern = Pattern.compile(format);
Pattern constantPattern = Pattern.compile(CONSTANT_PATTERN);
context.registerSyntaxNodeConsumer(Tree.Kind.CLASSDEF, ctx -> {
PyClassDefTree classDef = (PyClassDefTree) ctx.syntaxNode();
if (CheckUtils.classHasInheritance(classDef)) {
return;
}
for (TreeSymbol field : fieldsToCheck(classDef)) {
String name = field.name();
if (!pattern.matcher(name).matches() && !constantPattern.matcher(name).matches()) {
String message = String.format(MESSAGE, name, this.format);
field.usages().stream()
.filter(usage -> usage.kind() == Usage.Kind.ASSIGNMENT_LHS)
.limit(1)
.forEach(usage -> ctx.addIssue(usage.tree(), message));
}
}
}
});
}

private void checkName(Token token) {
String name = token.getValue();
if (pattern == null) {
pattern = Pattern.compile(format);
}
if (!pattern.matcher(name).matches()) {
addIssue(token, String.format(MESSAGE, name, format));
}
private static List<TreeSymbol> fieldsToCheck(PyClassDefTree classDef) {
Set<String> classFieldNames = classDef.classFields().stream().map(TreeSymbol::name).collect(Collectors.toSet());
List<TreeSymbol> result = new ArrayList<>(classDef.classFields());
classDef.instanceFields().stream().filter(f -> !classFieldNames.contains(f.name())).forEach(result::add);
return result;
}

}
5 changes: 5 additions & 0 deletions python-checks/src/test/resources/checks/fieldName.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,17 +2,22 @@
class MyClass:
myField = 4 # Noncompliant {{Rename this field "myField" to match the regular expression ^[_a-z][a-z0-9_]+$.}}
# ^^^^^^^
myField = 5
myField2: int = 4 # Noncompliant {{Rename this field "myField2" to match the regular expression ^[_a-z][a-z0-9_]+$.}}
# ^^^^^^^^
my_field = 4

def __init__(self):
localVar = 0
self.myField = 0
self.myField = 0
self.my_field1 = 0
self.myField1 = 0 # Noncompliant
# ^^^^^^^^

def instance_field_usage(self):
print(self.newField)

def fun(self):
self.myField.field = 1
self.newField = 0 # Noncompliant
Expand Down
6 changes: 6 additions & 0 deletions python-squid/src/main/java/org/sonar/python/api/tree/PyClassDefTree.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,9 @@
package org.sonar.python.api.tree;

import java.util.List;
import java.util.Set;
import javax.annotation.CheckForNull;
import org.sonar.python.semantic.TreeSymbol;

public interface PyClassDefTree extends PyStatementTree {

Expand Down Expand Up @@ -48,4 +50,8 @@ public interface PyClassDefTree extends PyStatementTree {

@CheckForNull
PyToken docstring();

Set<TreeSymbol> classFields();

Set<TreeSymbol> instanceFields();
}
2 changes: 0 additions & 2 deletions python-squid/src/main/java/org/sonar/python/api/tree/PyFunctionDefTree.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,8 +47,6 @@ public interface PyFunctionDefTree extends PyStatementTree, PyFunctionLikeTree {

PyStatementListTree body();

boolean isMethodDefinition();

@CheckForNull
PyToken docstring();

Expand Down
2 changes: 2 additions & 0 deletions python-squid/src/main/java/org/sonar/python/api/tree/PyFunctionLikeTree.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,4 +28,6 @@ public interface PyFunctionLikeTree extends Tree {
PyParameterListTree parameters();

Set<TreeSymbol> localVariables();

boolean isMethodDefinition();
}
87 changes: 77 additions & 10 deletions python-squid/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 @@ -35,12 +35,14 @@
import org.sonar.python.api.tree.HasSymbol;
import org.sonar.python.api.tree.PyAliasedNameTree;
import org.sonar.python.api.tree.PyAnnotatedAssignmentTree;
import org.sonar.python.api.tree.PyAnyParameterTree;
import org.sonar.python.api.tree.PyAssignmentStatementTree;
import org.sonar.python.api.tree.PyClassDefTree;
import org.sonar.python.api.tree.PyCompoundAssignmentStatementTree;
import org.sonar.python.api.tree.PyComprehensionForTree;
import org.sonar.python.api.tree.PyDecoratorTree;
import org.sonar.python.api.tree.PyDottedNameTree;
import org.sonar.python.api.tree.PyExpressionTree;
import org.sonar.python.api.tree.PyFileInputTree;
import org.sonar.python.api.tree.PyForStatementTree;
import org.sonar.python.api.tree.PyFunctionDefTree;
Expand All @@ -52,11 +54,13 @@
import org.sonar.python.api.tree.PyNameTree;
import org.sonar.python.api.tree.PyNonlocalStatementTree;
import org.sonar.python.api.tree.PyParameterListTree;
import org.sonar.python.api.tree.PyParameterTree;
import org.sonar.python.api.tree.PyQualifiedExpressionTree;
import org.sonar.python.api.tree.PyTupleTree;
import org.sonar.python.api.tree.Tree;
import org.sonar.python.api.tree.Tree.Kind;
import org.sonar.python.tree.BaseTreeVisitor;
import org.sonar.python.tree.PyClassDefTreeImpl;
import org.sonar.python.tree.PyFunctionDefTreeImpl;
import org.sonar.python.tree.PyLambdaExpressionTreeImpl;
import org.sonar.python.tree.PyNameTreeImpl;
Expand All @@ -65,6 +69,7 @@
public class SymbolTableBuilder extends BaseTreeVisitor {

private Map<Tree, Scope> scopesByRootTree;
private Set<Tree> assignmentLeftHandSides = new HashSet<>();

@Override
public void visitFileInput(PyFileInputTree pyFileInputTree) {
Expand All @@ -83,6 +88,13 @@ public void visitFileInput(PyFileInputTree pyFileInputTree) {
}
}
});
scopesByRootTree.values().stream()
.filter(scope -> scope.rootTree.is(Kind.CLASSDEF))
.forEach(scope -> {
PyClassDefTreeImpl classDef = (PyClassDefTreeImpl) scope.rootTree;
scope.symbols.forEach(classDef::addClassField);
scope.instanceAttributesByName.values().forEach(classDef::addInstanceField);
});
}

private static class ScopeVisitor extends BaseTreeVisitor {
Expand Down Expand Up @@ -115,7 +127,7 @@ public void visitFileInput(PyFileInputTree tree) {
public void visitLambda(PyLambdaExpressionTree pyLambdaExpressionTree) {
createScope(pyLambdaExpressionTree, currentScope());
enterScope(pyLambdaExpressionTree);
createParameters(pyLambdaExpressionTree.parameters());
createParameters(pyLambdaExpressionTree);
super.visitLambda(pyLambdaExpressionTree);
leaveScope();
}
Expand All @@ -124,7 +136,7 @@ public void visitLambda(PyLambdaExpressionTree pyLambdaExpressionTree) {
public void visitFunctionDef(PyFunctionDefTree pyFunctionDefTree) {
createScope(pyFunctionDefTree, currentScope());
enterScope(pyFunctionDefTree);
createParameters(pyFunctionDefTree.parameters());
createParameters(pyFunctionDefTree);
super.visitFunctionDef(pyFunctionDefTree);
leaveScope();
}
Expand Down Expand Up @@ -192,11 +204,25 @@ private void createLoopVariables(PyForStatementTree loopTree) {
});
}

private void createParameters(@Nullable PyParameterListTree parameterList) {
if (parameterList == null) {
private void createParameters(PyFunctionLikeTree function) {
PyParameterListTree parameterList = function.parameters();
if (parameterList == null || parameterList.all().isEmpty()) {
return;
}
parameterList.nonTuple().forEach(param -> addBindingUsage(param.name(), Usage.Kind.PARAMETER));

boolean hasSelf = false;
if (function.isMethodDefinition()) {
PyAnyParameterTree first = parameterList.all().get(0);
if (first.is(Kind.PARAMETER)) {
currentScope().createSelfParameter((PyParameterTree) first);
hasSelf = true;
}
}

parameterList.nonTuple().stream()
.skip(hasSelf ? 1 : 0)
.forEach(param -> addBindingUsage(param.name(), Usage.Kind.PARAMETER));

parameterList.all().stream()
.filter(param -> param.is(Kind.TUPLE))
.flatMap(param -> ((PyTupleTree) param).elements().stream())
Expand All @@ -207,15 +233,30 @@ private void createParameters(@Nullable PyParameterListTree parameterList) {

@Override
public void visitAssignmentStatement(PyAssignmentStatementTree pyAssignmentStatementTree) {
pyAssignmentStatementTree.lhsExpressions().stream()
List<PyExpressionTree> lhs = pyAssignmentStatementTree.lhsExpressions().stream()
.flatMap(exprList -> exprList.expressions().stream())
.flatMap(expr -> expr.is(Kind.TUPLE) ? ((PyTupleTree) expr).elements().stream() : Stream.of(expr))
.flatMap(this::flattenTuples)
.collect(Collectors.toList());

assignmentLeftHandSides.addAll(lhs);

lhs.stream()
.filter(expr -> expr.is(Kind.NAME))
.map(PyNameTree.class::cast)
.forEach(name -> addBindingUsage(name, Usage.Kind.ASSIGNMENT_LHS));

super.visitAssignmentStatement(pyAssignmentStatementTree);
}

private Stream<PyExpressionTree> flattenTuples(PyExpressionTree expression) {
if (expression.is(Kind.TUPLE)) {
PyTupleTree tuple = (PyTupleTree) expression;
return tuple.elements().stream().flatMap(this::flattenTuples);
} else {
return Stream.of(expression);
}
}

@Override
public void visitAnnotatedAssignment(PyAnnotatedAssignmentTree pyAnnotatedAssignmentTree) {
if (pyAnnotatedAssignmentTree.variable().is(Kind.NAME)) {
Expand Down Expand Up @@ -275,6 +316,7 @@ private static class Scope {
private final Set<TreeSymbol> symbols = new HashSet<>();
private final Set<String> globalNames = new HashSet<>();
private final Set<String> nonlocalNames = new HashSet<>();
private final Map<String, SymbolImpl> instanceAttributesByName = new HashMap<>();

private Scope(@Nullable Scope parent, Tree rootTree) {
this.parent = parent;
Expand All @@ -285,6 +327,14 @@ private Set<TreeSymbol> symbols() {
return Collections.unmodifiableSet(symbols);
}

private void createSelfParameter(PyParameterTree parameter) {
PyNameTree nameTree = parameter.name();
String symbolName = nameTree.name();
SymbolImpl symbol = new SelfSymbolImpl(symbolName, parent);
symbols.add(symbol);
symbolsByName.put(symbolName, symbol);
symbol.addUsage(nameTree, Usage.Kind.PARAMETER);
}

void addBindingUsage(PyNameTree nameTree, Usage.Kind kind, @Nullable String fullyQualifiedName) {
String symbolName = nameTree.name();
Expand Down Expand Up @@ -357,7 +407,7 @@ void addUsage(Tree tree, Usage.Kind kind) {
}
}

void addOrCreateChildUsage(PyNameTree name) {
void addOrCreateChildUsage(PyNameTree name, Usage.Kind kind) {
String childSymbolName = name.name();
if (!childrenSymbolByName.containsKey(childSymbolName)) {
String childFullyQualifiedName = fullyQualifiedName != null
Expand All @@ -367,7 +417,23 @@ void addOrCreateChildUsage(PyNameTree name) {
childrenSymbolByName.put(childSymbolName, symbol);
}
TreeSymbol symbol = childrenSymbolByName.get(childSymbolName);
((SymbolImpl) symbol).addUsage(name, Usage.Kind.OTHER);
((SymbolImpl) symbol).addUsage(name, kind);
}
}

private static class SelfSymbolImpl extends SymbolImpl {

private final Scope classScope;

SelfSymbolImpl(String name, Scope classScope) {
super(name, null);
this.classScope = classScope;
}

@Override
void addOrCreateChildUsage(PyNameTree nameTree, Usage.Kind kind) {
SymbolImpl symbol = classScope.instanceAttributesByName.computeIfAbsent(nameTree.name(), name -> new SymbolImpl(name, null));
symbol.addUsage(nameTree, kind);
}
}

Expand Down Expand Up @@ -412,7 +478,8 @@ public void visitQualifiedExpression(PyQualifiedExpressionTree qualifiedExpressi
if (qualifiedExpression.qualifier() instanceof HasSymbol) {
TreeSymbol qualifierSymbol = ((HasSymbol) qualifiedExpression.qualifier()).symbol();
if (qualifierSymbol != null) {
((SymbolImpl) qualifierSymbol).addOrCreateChildUsage(qualifiedExpression.name());
Usage.Kind usageKind = assignmentLeftHandSides.contains(qualifiedExpression) ? Usage.Kind.ASSIGNMENT_LHS : Usage.Kind.OTHER;
((SymbolImpl) qualifierSymbol).addOrCreateChildUsage(qualifiedExpression.name(), usageKind);
}
}
}
Expand Down
Loading