From 9960f718e81e5d66914110176bf621efb0d3bef4 Mon Sep 17 00:00:00 2001 From: Andrey Vlasovskikh Date: Fri, 21 Aug 2015 16:56:51 +0300 Subject: [PATCH] PyArgumentsListInspection now uses new PyCallExpressionHelper.mapArguments() instead of CallArgumentsMapping We simplified two checks: now we don't detect some cases of multiple values that resolve to a single positional parameter (we show a related warning about the wrong order of keyword arguments anyway) and we stopped telling the user that a sequence literal argument doens't match against a tuple paramater in Python 2. We might enhance the new version of arguments-parameters matching to restore this functionality. --- .../inspections/PyArgumentListInspection.java | 131 ++++++++++++------ .../psi/impl/PyCallExpressionHelper.java | 118 +++++++++------- .../PyArgumentListInspection/py1268.py | 13 +- .../PyArgumentListInspection/py3k.py | 5 +- .../tupleVsLiteralList.py | 2 +- 5 files changed, 166 insertions(+), 103 deletions(-) diff --git a/python/src/com/jetbrains/python/inspections/PyArgumentListInspection.java b/python/src/com/jetbrains/python/inspections/PyArgumentListInspection.java index 67c56ff4096e..d729c242366b 100644 --- a/python/src/com/jetbrains/python/inspections/PyArgumentListInspection.java +++ b/python/src/com/jetbrains/python/inspections/PyArgumentListInspection.java @@ -37,10 +37,8 @@ import com.jetbrains.python.psi.types.TypeEvalContext; import org.jetbrains.annotations.Nls; import org.jetbrains.annotations.NotNull; -import java.util.ArrayList; -import java.util.EnumSet; -import java.util.List; -import java.util.Map; +import java.util.*; +import java.util.HashMap; /** * Looks at argument lists. @@ -114,7 +112,6 @@ public class PyArgumentListInspection extends PyInspection { } final PyResolveContext resolveContext = PyResolveContext.noImplicits().withTypeEvalContext(context); final PyCallExpression.PyArgumentsMapping mapping = callExpr.mapArguments(resolveContext, implicitOffset); - CallArgumentsMapping result = node.analyzeCall(resolveContext, implicitOffset); final PyCallExpression.PyMarkedCallee callee = mapping.getMarkedCallee(); if (callee != null) { final PyCallable callable = callee.getCallable(); @@ -123,7 +120,7 @@ public class PyArgumentListInspection extends PyInspection { return; } } - highlightIncorrectArguments(holder, result, context); + highlightIncorrectArguments(callExpr, holder, mapping); highlightMissingArguments(node, holder, mapping); highlightStarArgumentTypeMismatch(node, holder, context); } @@ -132,50 +129,91 @@ public class PyArgumentListInspection extends PyInspection { inspectPyArgumentList(node, holder, context, 0); } - private static void highlightIncorrectArguments(ProblemsHolder holder, CallArgumentsMapping result, @NotNull TypeEvalContext context) { - for (Map.Entry> argEntry : result.getArgumentFlags().entrySet()) { - EnumSet flags = argEntry.getValue(); - if (!flags.isEmpty()) { // something's wrong - PyExpression arg = argEntry.getKey(); - if (flags.contains(CallArgumentsMapping.ArgFlag.IS_DUP)) { - holder.registerProblem(arg, PyBundle.message("INSP.duplicate.argument"), new PyRemoveArgumentQuickFix()); + private enum ArgumentProblem { + OK, + DUPLICATE_KEYWORD_ARGUMENT, + DUPLICATE_KEYWORD_CONTAINER, + DUPLICATE_POSITIONAL_CONTAINER, + CANNOT_APPEAR_AFTER_KEYWORD_OR_CONTAINER, + } + + @NotNull + private static Map analyzeArguments(@NotNull PyCallExpression callExpression) { + final Map results = new HashMap(); + final Set keywordArgumentNames = new HashSet(); + boolean seenKeywordOrContainerArgument = false; + boolean seenKeywordContainer = false; + boolean seenPositionalContainer = false; + for (PyExpression argument : callExpression.getArguments()) { + if (argument instanceof PyKeywordArgument) { + seenKeywordOrContainerArgument = true; + final String keyword = ((PyKeywordArgument)argument).getKeyword(); + final ArgumentProblem problem; + if (keywordArgumentNames.contains(keyword)) { + problem = ArgumentProblem.DUPLICATE_KEYWORD_ARGUMENT; } - if (flags.contains(CallArgumentsMapping.ArgFlag.IS_DUP_KWD)) { - holder.registerProblem(arg, PyBundle.message("INSP.duplicate.doublestar.arg"), new PyRemoveArgumentQuickFix()); + else if (seenKeywordContainer) { + problem = ArgumentProblem.CANNOT_APPEAR_AFTER_KEYWORD_OR_CONTAINER; } - if (flags.contains(CallArgumentsMapping.ArgFlag.IS_DUP_TUPLE)) { - holder.registerProblem(arg, PyBundle.message("INSP.duplicate.star.arg"), new PyRemoveArgumentQuickFix()); + else { + problem = ArgumentProblem.OK; } - if (flags.contains(CallArgumentsMapping.ArgFlag.IS_POS_PAST_KWD)) { - holder.registerProblem(arg, PyBundle.message("INSP.cannot.appear.past.keyword.arg"), ProblemHighlightType.ERROR, new PyRemoveArgumentQuickFix()); + results.put(argument, problem); + keywordArgumentNames.add(keyword); + } + else if (argument instanceof PyStarArgument) { + seenKeywordOrContainerArgument = true; + final PyStarArgument starArgument = (PyStarArgument)argument; + if (starArgument.isKeyword()) { + results.put(argument, seenKeywordContainer ? ArgumentProblem.DUPLICATE_KEYWORD_CONTAINER : ArgumentProblem.OK); + seenKeywordContainer = true; } - if (flags.contains(CallArgumentsMapping.ArgFlag.IS_UNMAPPED)) { - ArrayList quickFixes = Lists.newArrayList(new PyRemoveArgumentQuickFix()); - if (arg instanceof PyKeywordArgument) { - quickFixes.add(new PyRenameArgumentQuickFix()); - } - holder.registerProblem(arg, PyBundle.message("INSP.unexpected.arg"), quickFixes.toArray(new LocalQuickFix[quickFixes.size()-1])); + else { + results.put(argument, seenPositionalContainer ? ArgumentProblem.DUPLICATE_POSITIONAL_CONTAINER : ArgumentProblem.OK); + seenPositionalContainer = true; } - if (flags.contains(CallArgumentsMapping.ArgFlag.IS_TOO_LONG)) { - final PyCallExpression.PyMarkedCallee markedCallee = result.getMarkedCallee(); - String parameterName = null; - if (markedCallee != null) { - final List parameters = PyUtil.getParameters(markedCallee.getCallable(), context); - for (int i = parameters.size() - 1; i >= 0; --i) { - final PyParameter param = parameters.get(i); - if (param instanceof PyNamedParameter) { - final List unmappedParams = result.getUnmappedParams(); - if (!((PyNamedParameter)param).isPositionalContainer() && !((PyNamedParameter)param).isKeywordContainer() && - param.getDefaultValue() == null && !unmappedParams.contains(param)) { - parameterName = param.getName(); - break; - } - } - } - holder.registerProblem(arg, parameterName != null ? PyBundle.message("INSP.multiple.values.resolve.to.positional.$0", parameterName) - : PyBundle.message("INSP.more.args.that.pos.params")); - } + } + else { + results.put(argument, seenKeywordOrContainerArgument ? ArgumentProblem.CANNOT_APPEAR_AFTER_KEYWORD_OR_CONTAINER : ArgumentProblem.OK); + } + } + return results; + } + + private static void highlightIncorrectArguments(@NotNull PyCallExpression callExpr, + @NotNull ProblemsHolder holder, + @NotNull PyCallExpression.PyArgumentsMapping mapping) { + final Set problematicArguments = new HashSet(); + for (Map.Entry entry : analyzeArguments(callExpr).entrySet()) { + final PyExpression argument = entry.getKey(); + final ArgumentProblem problem = entry.getValue(); + switch (problem) { + case OK: + break; + case DUPLICATE_KEYWORD_ARGUMENT: + holder.registerProblem(argument, PyBundle.message("INSP.duplicate.argument"), new PyRemoveArgumentQuickFix()); + break; + case DUPLICATE_KEYWORD_CONTAINER: + holder.registerProblem(argument, PyBundle.message("INSP.duplicate.doublestar.arg"), new PyRemoveArgumentQuickFix()); + break; + case DUPLICATE_POSITIONAL_CONTAINER: + holder.registerProblem(argument, PyBundle.message("INSP.duplicate.star.arg"), new PyRemoveArgumentQuickFix()); + break; + case CANNOT_APPEAR_AFTER_KEYWORD_OR_CONTAINER: + holder.registerProblem(argument, PyBundle.message("INSP.cannot.appear.past.keyword.arg"), ProblemHighlightType.ERROR, new PyRemoveArgumentQuickFix()); + } + if (problem != ArgumentProblem.OK) { + problematicArguments.add(argument); + } + } + + for (PyExpression argument : mapping.getUnmappedArguments()) { + if (!problematicArguments.contains(argument)) { + final List quickFixes = Lists.newArrayList(new PyRemoveArgumentQuickFix()); + if (argument instanceof PyKeywordArgument) { + quickFixes.add(new PyRenameArgumentQuickFix()); } + holder.registerProblem(argument, PyBundle.message("INSP.unexpected.arg"), quickFixes.toArray(new LocalQuickFix[quickFixes.size() - 1])); } } } @@ -210,7 +248,10 @@ public class PyArgumentListInspection extends PyInspection { ASTNode close_paren = our_node.findChildByType(PyTokenTypes.RPAR); if (close_paren != null) { for (PyParameter parameter : mapping.getUnmappedParameters()) { - holder.registerProblem(close_paren.getPsi(), PyBundle.message("INSP.parameter.$0.unfilled", parameter.getName())); + final String name = parameter.getName(); + if (name != null) { + holder.registerProblem(close_paren.getPsi(), PyBundle.message("INSP.parameter.$0.unfilled", name)); + } } } } diff --git a/python/src/com/jetbrains/python/psi/impl/PyCallExpressionHelper.java b/python/src/com/jetbrains/python/psi/impl/PyCallExpressionHelper.java index 9d903d055e0f..fd7b45db558c 100644 --- a/python/src/com/jetbrains/python/psi/impl/PyCallExpressionHelper.java +++ b/python/src/com/jetbrains/python/psi/impl/PyCallExpressionHelper.java @@ -645,6 +645,7 @@ public class PyCallExpressionHelper { } boolean seenSingleStar = false; + boolean mappedVariadicArgumentsToParameters = false; final TypeEvalContext context = resolveContext.getTypeEvalContext(); final Map mappedParameters = new LinkedHashMap(); final List unmappedParameters = new ArrayList(); @@ -653,32 +654,34 @@ public class PyCallExpressionHelper { final List parameters = dropImplicitParameters(allParameters, markedCallee.getImplicitOffset()); final List arguments = new ArrayList(Arrays.asList(argumentList.getArguments())); - final List positionalArguments = removePositionalElements(arguments, resolveContext); + final List positionalArguments = filterPositionalElements(arguments); final List keywordArguments = filterKeywordArguments(arguments); - final List variadicPositionalArguments = filterVariadicPositionalArguments(arguments); + final Pair, List> variadicPositionalArgumentsAndTheirComponents = filterVariadicPositionalArguments(arguments); + final List variadicPositionalArguments = variadicPositionalArgumentsAndTheirComponents.getFirst(); + final List positionalComponentsOfVariadicArguments = variadicPositionalArgumentsAndTheirComponents.getSecond(); final List variadicKeywordArguments = filterVariadicKeywordArguments(arguments); + final List allPositionalArguments = new ArrayList(); + allPositionalArguments.addAll(positionalArguments); + allPositionalArguments.addAll(positionalComponentsOfVariadicArguments); + for (PyParameter parameter : parameters) { if (parameter instanceof PyNamedParameter) { final PyNamedParameter namedParameter = (PyNamedParameter)parameter; final String parameterName = namedParameter.getName(); if (namedParameter.isPositionalContainer()) { if (variadicPositionalArguments.size() == 1) { - mappedParameters.put(variadicPositionalArguments.remove(0), namedParameter); - } - else { - positionalArguments.clear(); - variadicPositionalArguments.clear(); + mappedParameters.put(variadicPositionalArguments.get(0), namedParameter); } + allPositionalArguments.clear(); + variadicPositionalArguments.clear(); } else if (namedParameter.isKeywordContainer()) { if (variadicKeywordArguments.size() == 1) { - mappedParameters.put(variadicKeywordArguments.remove(0), namedParameter); - } - else { - keywordArguments.clear(); - variadicKeywordArguments.clear(); + mappedParameters.put(variadicKeywordArguments.get(0), namedParameter); } + keywordArguments.clear(); + variadicKeywordArguments.clear(); } else if (seenSingleStar) { final PyExpression keywordArgument = removeKeywordArgument(keywordArguments, parameterName); @@ -690,16 +693,7 @@ public class PyCallExpressionHelper { } } else { - if (!positionalArguments.isEmpty()) { - final PyExpression positionalArgument = next(positionalArguments); - if (positionalArgument != null) { - mappedParameters.put(positionalArgument, namedParameter); - } - else if (!namedParameter.hasDefaultValue()) { - unmappedParameters.add(namedParameter); - } - } - else { + if (allPositionalArguments.isEmpty()) { final PyKeywordArgument keywordArgument = removeKeywordArgument(keywordArguments, parameterName); if (keywordArgument != null) { mappedParameters.put(keywordArgument, namedParameter); @@ -707,11 +701,35 @@ public class PyCallExpressionHelper { else if (variadicPositionalArguments.isEmpty() && variadicKeywordArguments.isEmpty() && !namedParameter.hasDefaultValue()) { unmappedParameters.add(namedParameter); } + else { + mappedVariadicArgumentsToParameters = true; + } + } + else { + final PyExpression positionalArgument = next(allPositionalArguments); + if (positionalArgument != null) { + mappedParameters.put(positionalArgument, namedParameter); + } + else if (!namedParameter.hasDefaultValue()) { + unmappedParameters.add(namedParameter); + } } } } else if (parameter instanceof PyTupleParameter) { - // TODO: Handle tuple parameters + final PyExpression positionalArgument = next(allPositionalArguments); + // TODO: Python 2: If we found a positional argument, we should match the components of the argument against the components of the + // tuple parameter + if (positionalArgument == null) { + if (variadicPositionalArguments.isEmpty()) { + if (!parameter.hasDefaultValue()) { + unmappedParameters.add(parameter); + } + } + else { + mappedVariadicArgumentsToParameters = true; + } + } } else if (parameter instanceof PySingleStarParameter) { seenSingleStar = true; @@ -721,8 +739,13 @@ public class PyCallExpressionHelper { } } + if (mappedVariadicArgumentsToParameters) { + variadicPositionalArguments.clear(); + variadicKeywordArguments.clear(); + } + final List unmappedArguments = new ArrayList(); - unmappedArguments.addAll(positionalArguments); + unmappedArguments.addAll(allPositionalArguments); unmappedArguments.addAll(keywordArguments); unmappedArguments.addAll(variadicPositionalArguments); unmappedArguments.addAll(variadicKeywordArguments); @@ -747,28 +770,17 @@ public class PyCallExpressionHelper { } @NotNull - private static List removePositionalElements(@NotNull List arguments, - @NotNull PyResolveContext resolveContext) { + private static List filterPositionalElements(@NotNull List arguments) { final List results = new ArrayList(); - for (PyExpression argument : new ArrayList(arguments)) { + boolean seenKeywordOrContainerArgument = false; + for (PyExpression argument : arguments) { if (isPositionalArgument(argument)) { - results.add(argument); - arguments.remove(argument); + if (!seenKeywordOrContainerArgument) { + results.add(argument); + } } - else if (isVariadicPositionalArgument(argument)) { - final PsiElement expr = PyPsiUtils.flattenParens(PsiTreeUtil.getChildOfType(argument, PyExpression.class)); - final PsiElement element; - if (expr instanceof PyReferenceExpression) { - element = ((PyReferenceExpression)expr).followAssignmentsChain(resolveContext).getElement(); - } - else { - element = expr; - } - if (element instanceof PySequenceExpression) { - final PySequenceExpression sequenceExpr = (PySequenceExpression)element; - results.addAll(Arrays.asList(sequenceExpr.getElements())); - arguments.remove(argument); - } + if (argument instanceof PyStarArgument || argument instanceof PyKeywordArgument) { + seenKeywordOrContainerArgument = true; } } return results; @@ -785,15 +797,27 @@ public class PyCallExpressionHelper { return results; } + /** + * Returns a list of variadic positional arguments and a list of components of variadic positional arguments + * if they are sequence literals. + */ @NotNull - private static List filterVariadicPositionalArguments(@NotNull List arguments) { - final List results = new ArrayList(); + private static Pair, List> filterVariadicPositionalArguments(@NotNull List arguments) { + final List variadicArguments = new ArrayList(); + final List positionalComponentsOfVariadicArguments = new ArrayList(); for (PyExpression argument : arguments) { if (argument != null && isVariadicPositionalArgument(argument)) { - results.add(argument); + final PsiElement expr = PyPsiUtils.flattenParens(PsiTreeUtil.getChildOfType(argument, PyExpression.class)); + if (expr instanceof PySequenceExpression) { + final PySequenceExpression sequenceExpr = (PySequenceExpression)expr; + positionalComponentsOfVariadicArguments.addAll(Arrays.asList(sequenceExpr.getElements())); + } + else { + variadicArguments.add(argument); + } } } - return results; + return Pair.create(variadicArguments, positionalComponentsOfVariadicArguments); } @NotNull diff --git a/python/testData/inspections/PyArgumentListInspection/py1268.py b/python/testData/inspections/PyArgumentListInspection/py1268.py index 4e33321bb520..87fcc16b238a 100644 --- a/python/testData/inspections/PyArgumentListInspection/py1268.py +++ b/python/testData/inspections/PyArgumentListInspection/py1268.py @@ -3,8 +3,8 @@ def f(a, b, c): f(c=1, *(10, 20)) f(*(10, 20), c=1) -f(*(10, 20, 30), c=1) # fail: duplicate c -f(1, *(10, 20, 30)) # fail: tuple too long +f(*(10, 20, 30), c=1) # fail: duplicate c +f(1, *(10, 20, 30)) # fail: tuple too long f(1, *(10)) # fail: wrong type f(1, *(10,)) # fail: tuple too short, c not mapped @@ -24,15 +24,14 @@ f2(*(1,2), a=1, b=2, *(1,2)) # fail: a twice -f3(1, 2, *(3,), c=4) # fail: c twice +f3(c=3, a=1, b=2, *(1,2)) # fail: a and b twice +f3(1, 2, *(3,), c=4) # fail: c twice f3(1,2,3, *(1,2)) f3(c=3, *(1,2)) # -f3(1, c=3, *(1,2)) # fail: c twice -f3(c=3, a=1, b=2, *(1,2)) # fail: a twice, no positinals +f3(1, c=3, *(1,2)) # fail: c twice f3(c=3, a=1, b=2, d=(1,2)) # fail: unexpected d f3(1, c=3, *(10,)) # ZZZ f3(1, *(10,)) f3(1, *(10,), c=20) f3(*(1,2), c=20) -f3(*(1,2), a=20) # fail: a twice +f3(*(1,2), a=20) # fail: a twice diff --git a/python/testData/inspections/PyArgumentListInspection/py3k.py b/python/testData/inspections/PyArgumentListInspection/py3k.py index d60b09203c86..f47c28aa8998 100644 --- a/python/testData/inspections/PyArgumentListInspection/py3k.py +++ b/python/testData/inspections/PyArgumentListInspection/py3k.py @@ -19,9 +19,8 @@ def a23(a, *b, c=1): pass a23(1,2,3, c=10) # pass -a23(1,2,3, c=10, a=1) # fail +a23(1,2,3, c=10, a=1) # fail a23(c=10, a=1) # pass -a23(c=10, 1) # fail -a23(*args, a=1) # fail +a23(c=10, 1) # fail a23(*args, c=1) # pass diff --git a/python/testData/inspections/PyArgumentListInspection/tupleVsLiteralList.py b/python/testData/inspections/PyArgumentListInspection/tupleVsLiteralList.py index 41302aae7fe8..6acbbbc14f33 100644 --- a/python/testData/inspections/PyArgumentListInspection/tupleVsLiteralList.py +++ b/python/testData/inspections/PyArgumentListInspection/tupleVsLiteralList.py @@ -2,4 +2,4 @@ def f20(a, (b, c)): pass f20(1, [2, 3]) # ok -f20(1, (2, 3, 4)) # fail: 4 is unexpected +f20(1, (2, 3, 4)) # ok: ignore problems with arguments of tuple parameters