diff --git a/python/src/com/jetbrains/python/inspections/PyPropertyDefinitionInspection.java b/python/src/com/jetbrains/python/inspections/PyPropertyDefinitionInspection.java index 807666d9284a..e4df4761bc32 100644 --- a/python/src/com/jetbrains/python/inspections/PyPropertyDefinitionInspection.java +++ b/python/src/com/jetbrains/python/inspections/PyPropertyDefinitionInspection.java @@ -1,5 +1,5 @@ /* - * Copyright 2000-2014 JetBrains s.r.o. + * Copyright 2000-2016 JetBrains s.r.o. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -16,20 +16,22 @@ package com.jetbrains.python.inspections; import com.google.common.collect.ImmutableList; +import com.intellij.codeInsight.controlflow.ControlFlowUtil; import com.intellij.codeInspection.LocalInspectionToolSession; import com.intellij.codeInspection.ProblemHighlightType; import com.intellij.codeInspection.ProblemsHolder; import com.intellij.lang.ASTNode; import com.intellij.openapi.util.Comparing; +import com.intellij.openapi.util.Ref; import com.intellij.psi.PsiElement; import com.intellij.psi.PsiElementVisitor; import com.intellij.psi.PsiFile; import com.intellij.psi.PsiPolyVariantReference; -import com.intellij.psi.util.PsiElementFilter; import com.intellij.psi.util.PsiTreeUtil; import com.intellij.psi.util.QualifiedName; import com.jetbrains.python.PyBundle; import com.jetbrains.python.PyNames; +import com.jetbrains.python.codeInsight.controlflow.ControlFlowCache; import com.jetbrains.python.inspections.quickfix.PyUpdatePropertySignatureQuickFix; import com.jetbrains.python.inspections.quickfix.RenameParameterQuickFix; import com.jetbrains.python.psi.*; @@ -45,6 +47,7 @@ import org.jetbrains.annotations.Nullable; import java.util.ArrayList; import java.util.List; import java.util.Map; +import java.util.function.Predicate; /** * Checks that arguments to property() and @property and friends are ok. @@ -295,38 +298,53 @@ public class PyPropertyDefinitionInspection extends PyInspection { } } - private void checkReturnValueAllowed(PyCallable callable, PsiElement beingChecked, boolean allowed, String message) { - // TODO: use a real flow analysis to check all exit points - boolean hasReturns; + private void checkReturnValueAllowed(@NotNull PyCallable callable, + @NotNull PsiElement beingChecked, + boolean allowed, + @NotNull String message) { if (callable instanceof PyFunction) { - final PsiElement[] returnStatements = PsiTreeUtil.collectElements(callable, new PsiElementFilter() { - @Override - public boolean isAccepted(PsiElement element) { - return (element instanceof PyReturnStatement && ((PyReturnStatement)element).getExpression() != null) || - (element instanceof PyYieldExpression); - } - }); - hasReturns = returnStatements.length > 0; + final PyFunction function = (PyFunction)callable; + + if (PyUtil.isDecoratedAsAbstract(function)) { + return; + } + + if (allowed && !someFlowHasExitPoint(function, Visitor::isAllowedExitPoint) || + !allowed && someFlowHasExitPoint(function, Visitor::isDisallowedExitPoint)) { + registerProblem(beingChecked, message); + } } else { final PyType type = myTypeEvalContext.getReturnType(callable); - hasReturns = !(type instanceof PyNoneType); - } - if (allowed ^ hasReturns) { - if (allowed && callable instanceof PyFunction) { - if (PyUtil.isDecoratedAsAbstract(((PyFunction)callable))) { - return; - } - // one last chance: maybe there's no return but a 'raise' statement, see PY-4043, PY-5048 - PyStatementList statementList = ((PyFunction)callable).getStatementList(); - for (PyStatement stmt : statementList.getStatements()) { - if (stmt instanceof PyRaiseStatement) { - return; - } - } + final boolean hasReturns = !(type instanceof PyNoneType); + + if (allowed ^ hasReturns) { + registerProblem(beingChecked, message); } - registerProblem(beingChecked, message); } } + + private static boolean someFlowHasExitPoint(@NotNull PyFunction function, @NotNull Predicate exitPointPredicate) { + final Ref result = new Ref<>(false); + + ControlFlowUtil.process(ControlFlowCache.getControlFlow(function).getInstructions(), + 0, + instruction -> { + result.set(exitPointPredicate.test(instruction.getElement())); + return !result.get(); + } + ); + + return result.get(); + } + + private static boolean isAllowedExitPoint(@Nullable PsiElement element) { + return element instanceof PyRaiseStatement || element instanceof PyReturnStatement || element instanceof PyYieldExpression; + } + + private static boolean isDisallowedExitPoint(@Nullable PsiElement element) { + return element instanceof PyReturnStatement && ((PyReturnStatement)element).getExpression() != null || + element instanceof PyYieldExpression; + } } } diff --git a/python/testData/inspections/PyPropertyDefinitionInspection26/expected.xml b/python/testData/inspections/PyPropertyDefinitionInspection26/expected.xml index a7ef8033cef1..5fdadd56a3db 100644 --- a/python/testData/inspections/PyPropertyDefinitionInspection26/expected.xml +++ b/python/testData/inspections/PyPropertyDefinitionInspection26/expected.xml @@ -25,5 +25,37 @@ 38 Deleter should not return a value + + prop_test.py + 222 + light_idea_test_case + + Property definitions + Setter should not return a value + + + prop_test.py + 237 + light_idea_test_case + + Property definitions + Setter should not return a value + + + prop_test.py + 268 + light_idea_test_case + + Property definitions + Setter should not return a value + + + prop_test.py + 284 + light_idea_test_case + + Property definitions + Setter should not return a value + diff --git a/python/testData/inspections/PyPropertyDefinitionInspection26/src/prop_test.py b/python/testData/inspections/PyPropertyDefinitionInspection26/src/prop_test.py index f7b7d2065e45..40a6edc922a7 100644 --- a/python/testData/inspections/PyPropertyDefinitionInspection26/src/prop_test.py +++ b/python/testData/inspections/PyPropertyDefinitionInspection26/src/prop_test.py @@ -68,3 +68,233 @@ class A(object): @abstractproperty def abstract_property(self): pass + + +# PY-19701 +class Test(object): + def __init__(self): + self._myprop = None + + def get_myprop(self): + return self._myprop + + def set_myprop(self, val): + def inner_func(n): + return n + self._myprop = inner_func(val) + + myprop = property(get_myprop, set_myprop) # pass + + +# all flows have exit point +class Test(object): + def __init__(self): + self._myprop = None + + def get_myprop(self): + if a > b: + return self._myprop + elif a < b: + raise self._myprop + else: + yield self._myprop + + myprop = property(get_myprop) # pass + + +# some flows have not exit point +class Test(object): + def __init__(self): + self._myprop = None + + def get_myprop(self): + if a > b: + return self._myprop + elif a < b: + raise self._myprop + + myprop = property(get_myprop) # pass + + +# some flows have not exit point +class Test(object): + def __init__(self): + self._myprop = None + + def get_myprop(self): + if a > b: + return self._myprop + + myprop = property(get_myprop) # pass + + +# non-empty for +class Test(object): + def __init__(self): + self._myprop = None + + def get_myprop(self): + for i in range(5): + yield i + + myprop = property(get_myprop) # pass + + +# empty for +class Test(object): + def __init__(self): + self._myprop = None + + def get_myprop(self): + for i in []: + yield i + + myprop = property(get_myprop) # shouldn't pass with better analysis, pass at the moment + + +# non-empty while +class Test(object): + def __init__(self): + self._myprop = None + + def get_myprop(self): + i = 0 + while i < 5: + yield i + i += 1 + + myprop = property(get_myprop) # pass + + +# empty while +class Test(object): + def __init__(self): + self._myprop = None + + def get_myprop(self): + while False: + yield i + + myprop = property(get_myprop) # shouldn't pass with better analysis, pass at the moment + + +# non-empty while with two conditions +class Test(object): + def __init__(self): + self._myprop = None + + def get_myprop(self): + i = 0 + j = 0 + while i < 5 and j == 0: + yield i + i += 1 + + myprop = property(get_myprop) # pass + + +# empty while with two conditions +class Test(object): + def __init__(self): + self._myprop = None + + def get_myprop(self): + i = 0 + j = 0 + while i > 5 and j == 0: + yield i + + myprop = property(get_myprop) # shouldn't pass with better analysis, pass at the moment + + +# setter has exit point +class Test(object): + def __init__(self): + self._myprop = None + + def get_myprop(self): + return self._myprop + + def set_myprop(self, val): + self._myprop = val + return 10 + + myprop = property(get_myprop, set_myprop) # shouldn't pass + + +# setter has exit point +class Test(object): + def __init__(self): + self._myprop = None + + def get_myprop(self): + return self._myprop + + def set_myprop(self, val): + self._myprop = val + yield 10 + + myprop = property(get_myprop, set_myprop) # shouldn't pass + + +# setter has raise statement +class Test(object): + def __init__(self): + self._myprop = None + + def get_myprop(self): + return self._myprop + + def set_myprop(self, val): + self._myprop = val + raise NotImplementedError() + + myprop = property(get_myprop, set_myprop) # pass + + +# setter has exit point in some flow +class Test(object): + def __init__(self): + self._myprop = None + + def get_myprop(self): + return self._myprop + + def set_myprop(self, val): + self._myprop = val + if a > b: + return 10 + + myprop = property(get_myprop, set_myprop) # shouldn't pass + + +# setter has exit point in some flow +class Test(object): + def __init__(self): + self._myprop = None + + def get_myprop(self): + return self._myprop + + def set_myprop(self, val): + self._myprop = val + if a > b: + yield 10 + + myprop = property(get_myprop, set_myprop) # shouldn't pass + + +# setter has raise statement in some flow +class Test(object): + def __init__(self): + self._myprop = None + + def get_myprop(self): + return self._myprop + + def set_myprop(self, val): + self._myprop = val + if a > b: + raise NotImplementedError() + + myprop = property(get_myprop, set_myprop) # pass