Skip to content

SONARPY-775 Fix FN on S5797: flow sensitive inference not enabled for nested try/except #827

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
guillaume-dequenne merged 2 commits into from
Aug 10, 2020
Merged

SONARPY-775 Fix FN on S5797: flow sensitive inference not enabled for nested try/except #827

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
090d9d3
Fix FN on S5797: flow sensitive inference not enabled for nested try/…
guillaume-dequenne Aug 10, 2020
77f9750
Fix after review
guillaume-dequenne Aug 10, 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
2 changes: 1 addition & 1 deletion python-checks/src/test/resources/checks/functionReturnType.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,7 @@ def my_list_union_nok(cond) -> List[str]:
value = 42
else:
value = "hello"
return value # FN (SONARPY-775)
return value # Noncompliant {{Return a value of type "list[str]" or update function "my_list_union_nok" type hint.}}

def custom_classes():
class A: ...
Expand Down
12 changes: 12 additions & 0 deletions python-checks/src/test/resources/checks/nonCallableCalled.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,18 @@ def flow_sensitivity():
my_other_var = 42
my_other_var() # Noncompliant

def flow_sensitivity_nested_try_except():
def func_with_try_except():
try:
...
except:
...

def other_func():
my_var = "hello"
my_var = 42
my_var() # Noncompliant

def member_access():
my_callable = MyCallable()
my_callable.non_callable = 42
Expand Down
38 changes: 35 additions & 3 deletions python-frontend/src/main/java/org/sonar/python/types/TypeInference.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,16 +36,17 @@
import org.sonar.plugins.python.api.symbols.Usage;
import org.sonar.plugins.python.api.tree.AssignmentStatement;
import org.sonar.plugins.python.api.tree.BaseTreeVisitor;
import org.sonar.plugins.python.api.tree.ClassDef;
import org.sonar.plugins.python.api.tree.Expression;
import org.sonar.plugins.python.api.tree.FileInput;
import org.sonar.plugins.python.api.tree.FunctionDef;
import org.sonar.plugins.python.api.tree.Name;
import org.sonar.plugins.python.api.tree.QualifiedExpression;
import org.sonar.plugins.python.api.tree.Tree;
import org.sonar.plugins.python.api.tree.TryStatement;
import org.sonar.plugins.python.api.types.InferredType;
import org.sonar.python.semantic.SymbolImpl;
import org.sonar.python.tree.NameImpl;
import org.sonar.python.tree.TreeUtils;

public class TypeInference extends BaseTreeVisitor {

Expand Down Expand Up @@ -98,10 +99,17 @@ private static void inferTypesAndMemberAccessSymbols(FunctionDef functionDef, Py
}
}

if (TreeUtils.hasDescendant(functionDef, tree -> tree.is(Tree.Kind.TRY_STMT))) {
TryStatementVisitor tryStatementVisitor = new TryStatementVisitor(functionDef);
functionDef.body().accept(tryStatementVisitor);
if (tryStatementVisitor.hasTryStatement) {
// CFG doesn't model precisely try-except statements. Hence we fallback to AST based type inference
visitor.processPropagations(trackedVars);
functionDef.accept(new BaseTreeVisitor() {
functionDef.body().accept(new BaseTreeVisitor() {
@Override
public void visitFunctionDef(FunctionDef visited) {
// Don't visit nested functions
}

@Override
public void visitName(Name name) {
Optional.ofNullable(name.symbol()).ifPresent(symbol ->
Expand All @@ -118,6 +126,30 @@ public void visitName(Name name) {
}
}

private static class TryStatementVisitor extends BaseTreeVisitor {
FunctionDef functionDef;
boolean hasTryStatement = false;

TryStatementVisitor(FunctionDef functionDef) {
this.functionDef = functionDef;
}

@Override
public void visitClassDef(ClassDef classDef) {
// Don't visit nested classes
}

@Override
public void visitFunctionDef(FunctionDef visited) {
// Don't visit nested functions
}

@Override
public void visitTryStatement(TryStatement tryStatement) {
hasTryStatement = true;
}
}

@Override
public void visitAssignmentStatement(AssignmentStatement assignmentStatement) {
super.visitAssignmentStatement(assignmentStatement);
Expand Down
105 changes: 105 additions & 0 deletions python-frontend/src/test/java/org/sonar/python/types/TypeInferenceTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -380,6 +380,111 @@ public void flow_insensitive_when_try_except() {
assertThat(thirdX.expression().type()).isEqualTo(or(INT, STR));
}

@Test
public void nested_try_except() {
FileInput fileInput = parse(
"def func(cond):",
" def f(p):",
" try:",
" if p:",
" x = 42",
" type(x)",
" else:",
" x = 'foo'",
" type(x)",
" except:",
" type(x)",
" def g(p):",
" if p:",
" y = 42",
" type(y)",
" else:",
" y = \"hello\"",
" type(y)",
" type(y)",
" if cond:",
" z = 42",
" type(z)",
" else:",
" z = \"hello\"",
" type(z)",
" type(z)"
);
List<CallExpression> calls = PythonTestUtils.getAllDescendant(fileInput, tree -> tree.is(Tree.Kind.CALL_EXPR));
RegularArgument firstX = (RegularArgument) calls.get(0).arguments().get(0);
RegularArgument secondX = (RegularArgument) calls.get(1).arguments().get(0);
RegularArgument thirdX = (RegularArgument) calls.get(2).arguments().get(0);
assertThat(firstX.expression().type()).isEqualTo(or(INT, STR));
assertThat(secondX.expression().type()).isEqualTo(or(INT, STR));
assertThat(thirdX.expression().type()).isEqualTo(or(INT, STR));

RegularArgument firstY = (RegularArgument) calls.get(3).arguments().get(0);
RegularArgument secondY = (RegularArgument) calls.get(4).arguments().get(0);
RegularArgument thirdY = (RegularArgument) calls.get(5).arguments().get(0);
assertThat(firstY.expression().type()).isEqualTo(INT);
assertThat(secondY.expression().type()).isEqualTo(STR);
assertThat(thirdY.expression().type()).isEqualTo(or(INT, STR));

RegularArgument firstZ = (RegularArgument) calls.get(6).arguments().get(0);
RegularArgument secondZ = (RegularArgument) calls.get(7).arguments().get(0);
RegularArgument thirdZ = (RegularArgument) calls.get(8).arguments().get(0);
assertThat(firstZ.expression().type()).isEqualTo(INT);
assertThat(secondZ.expression().type()).isEqualTo(STR);
assertThat(thirdZ.expression().type()).isEqualTo(or(INT, STR));
}

@Test
public void nested_try_except_2() {
FileInput fileInput = parse(
"def func(cond):",
" try:",
" if p:",
" x = 42",
" type(x)",
" else:",
" x = 'foo'",
" type(x)",
" except:",
" type(x)",
" def g(p):",
" if p:",
" y = 42",
" type(y)",
" else:",
" y = \"hello\"",
" type(y)",
" type(y)",
" if cond:",
" z = 42",
" type(z)",
" else:",
" z = \"hello\"",
" type(z)",
" type(z)"
);
List<CallExpression> calls = PythonTestUtils.getAllDescendant(fileInput, tree -> tree.is(Tree.Kind.CALL_EXPR));
RegularArgument firstX = (RegularArgument) calls.get(0).arguments().get(0);
RegularArgument secondX = (RegularArgument) calls.get(1).arguments().get(0);
RegularArgument thirdX = (RegularArgument) calls.get(2).arguments().get(0);
assertThat(firstX.expression().type()).isEqualTo(or(INT, STR));
assertThat(secondX.expression().type()).isEqualTo(or(INT, STR));
assertThat(thirdX.expression().type()).isEqualTo(or(INT, STR));

RegularArgument firstY = (RegularArgument) calls.get(3).arguments().get(0);
RegularArgument secondY = (RegularArgument) calls.get(4).arguments().get(0);
RegularArgument thirdY = (RegularArgument) calls.get(5).arguments().get(0);
assertThat(firstY.expression().type()).isEqualTo(INT);
assertThat(secondY.expression().type()).isEqualTo(STR);
assertThat(thirdY.expression().type()).isEqualTo(or(INT, STR));

RegularArgument firstZ = (RegularArgument) calls.get(6).arguments().get(0);
RegularArgument secondZ = (RegularArgument) calls.get(7).arguments().get(0);
RegularArgument thirdZ = (RegularArgument) calls.get(8).arguments().get(0);
assertThat(firstZ.expression().type()).isEqualTo(or(INT, STR));
assertThat(secondZ.expression().type()).isEqualTo(or(INT, STR));
assertThat(thirdZ.expression().type()).isEqualTo(or(INT, STR));
}

@Test
public void execution_order_assignment_statement() {
FileInput fileInput = parse(
Expand Down