Repository: groovy Updated Branches: refs/heads/master 3ac1abcd0 -> 3bdea65e1
GROOVY-7632: Groovy named parameters static check (initial cut of support) (closes #822) Project: http://git-wip-us.apache.org/repos/asf/groovy/repo Commit: http://git-wip-us.apache.org/repos/asf/groovy/commit/3bdea65e Tree: http://git-wip-us.apache.org/repos/asf/groovy/tree/3bdea65e Diff: http://git-wip-us.apache.org/repos/asf/groovy/diff/3bdea65e Branch: refs/heads/master Commit: 3bdea65e1b7b2382f0f743670e19dfa5f61d21c6 Parents: 3ac1abc Author: Paul King <pa...@asert.com.au> Authored: Thu Nov 15 00:33:14 2018 +1000 Committer: Paul King <pa...@asert.com.au> Committed: Fri Nov 16 10:27:49 2018 +1000 ---------------------------------------------------------------------- .../stc/StaticTypeCheckingVisitor.java | 84 +++++++++++++++++++ src/test/groovy/NamedParameterHelper.java | 40 +++++++++ src/test/groovy/NamedParameterTest.groovy | 87 ++++++++++++++++++++ .../apache/groovy/parser/antlr4/AstBuilder.java | 3 +- 4 files changed, 212 insertions(+), 2 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/groovy/blob/3bdea65e/src/main/java/org/codehaus/groovy/transform/stc/StaticTypeCheckingVisitor.java ---------------------------------------------------------------------- diff --git a/src/main/java/org/codehaus/groovy/transform/stc/StaticTypeCheckingVisitor.java b/src/main/java/org/codehaus/groovy/transform/stc/StaticTypeCheckingVisitor.java index 3e6d945..72ea3bc 100644 --- a/src/main/java/org/codehaus/groovy/transform/stc/StaticTypeCheckingVisitor.java +++ b/src/main/java/org/codehaus/groovy/transform/stc/StaticTypeCheckingVisitor.java @@ -22,6 +22,8 @@ import groovy.lang.Closure; import groovy.lang.DelegatesTo; import groovy.lang.IntRange; import groovy.lang.Range; +import groovy.transform.NamedParam; +import groovy.transform.NamedParams; import groovy.transform.TypeChecked; import groovy.transform.TypeCheckingMode; import groovy.transform.stc.ClosureParams; @@ -43,6 +45,7 @@ import org.codehaus.groovy.ast.MethodNode; import org.codehaus.groovy.ast.Parameter; import org.codehaus.groovy.ast.PropertyNode; import org.codehaus.groovy.ast.Variable; +import org.codehaus.groovy.ast.expr.AnnotationConstantExpression; import org.codehaus.groovy.ast.expr.ArgumentListExpression; import org.codehaus.groovy.ast.expr.AttributeExpression; import org.codehaus.groovy.ast.expr.BinaryExpression; @@ -285,6 +288,7 @@ public class StaticTypeCheckingVisitor extends ClassCodeVisitorSupport { protected static final ClassNode DELEGATES_TO_TARGET = ClassHelper.make(DelegatesTo.Target.class); protected static final ClassNode LINKEDHASHMAP_CLASSNODE = make(LinkedHashMap.class); protected static final ClassNode CLOSUREPARAMS_CLASSNODE = make(ClosureParams.class); + protected static final ClassNode NAMED_PARAMS_CLASSNODE = make(NamedParams.class); protected static final ClassNode MAP_ENTRY_TYPE = make(Map.Entry.class); protected static final ClassNode ENUMERATION_TYPE = make(Enumeration.class); @@ -2609,6 +2613,86 @@ public class StaticTypeCheckingVisitor extends ClassCodeVisitorSupport { } } } + if (expressions.size() > 0 && expressions.get(0) instanceof MapExpression && params.length > 0) { + checkNamedParamsAnnotation(params[0], (MapExpression) expressions.get(0)); + } + } + + private void checkNamedParamsAnnotation(Parameter param, MapExpression args) { + if (!param.getType().isDerivedFrom(ClassHelper.MAP_TYPE)) return; + List<MapEntryExpression> entryExpressions = args.getMapEntryExpressions(); + Map<Object, Expression> entries = new LinkedHashMap<Object, Expression>(); + for (MapEntryExpression entry : entryExpressions) { + Object key = entry.getKeyExpression(); + if (key instanceof ConstantExpression) { + key = ((ConstantExpression) key).getValue(); + } + entries.put(key, entry.getValueExpression()); + } + List<AnnotationNode> annotations = param.getAnnotations(NAMED_PARAMS_CLASSNODE); + if (annotations != null && !annotations.isEmpty()) { + AnnotationNode an = null; + for (AnnotationNode next : annotations) { + if (next.getClassNode().getName().equals(NamedParams.class.getName())) { + an = next; + } + } + List<String> collectedNames = new ArrayList<String>(); + if (an != null) { + Expression value = an.getMember("value"); + if (value instanceof AnnotationConstantExpression) { + processNamedParam((AnnotationConstantExpression) value, entries, args, collectedNames); + } else if (value instanceof ListExpression) { + ListExpression le = (ListExpression) value; + for (Expression next : le.getExpressions()) { + if (next instanceof AnnotationConstantExpression) { + processNamedParam((AnnotationConstantExpression) next, entries, args, collectedNames); + } + } + } + for (Map.Entry<Object, Expression> entry : entries.entrySet()) { + if (!collectedNames.contains(entry.getKey())) { + addStaticTypeError("unexpected named arg: " + entry.getKey(), args); + } + } + } + } + } + + private void processNamedParam(AnnotationConstantExpression value, Map<Object, Expression> entries, Expression expression, List<String> collectedNames) { + AnnotationNode namedParam = (AnnotationNode) value.getValue(); + if (!namedParam.getClassNode().getName().equals(NamedParam.class.getName())) return; + String name = null; + boolean required = false; + ClassNode expectedType = null; + ConstantExpression constX = (ConstantExpression) namedParam.getMember("value"); + if (constX != null) { + name = (String) constX.getValue(); + collectedNames.add(name); + } + constX = (ConstantExpression) namedParam.getMember("required"); + if (constX != null) { + required = (Boolean) constX.getValue(); + } + ClassExpression typeX = (ClassExpression) namedParam.getMember("type"); + if (typeX != null) { + expectedType = typeX.getType(); + } + if (!entries.keySet().contains(name)) { + if (required) { + addStaticTypeError("required named arg '" + name + "' not found.", expression); + } + } else { + Expression supplied = entries.get(name); + if (isCompatibleType(expectedType, expectedType != null, supplied.getType())) { + addStaticTypeError("parameter for named arg '" + name + "' has type '" + prettyPrintType(supplied.getType()) + + "' but expected '" + prettyPrintType(expectedType) + "'.", expression); + } + } + } + + private boolean isCompatibleType(ClassNode expectedType, boolean b, ClassNode type) { + return b && !isAssignableTo(type, expectedType); } /** http://git-wip-us.apache.org/repos/asf/groovy/blob/3bdea65e/src/test/groovy/NamedParameterHelper.java ---------------------------------------------------------------------- diff --git a/src/test/groovy/NamedParameterHelper.java b/src/test/groovy/NamedParameterHelper.java new file mode 100644 index 0000000..1d2aad5 --- /dev/null +++ b/src/test/groovy/NamedParameterHelper.java @@ -0,0 +1,40 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package groovy; + +import groovy.transform.NamedParam; +import groovy.transform.NamedParams; + +import java.util.LinkedHashMap; + +public class NamedParameterHelper { + public static String myJavaMethod(@NamedParams({ + @NamedParam(value = "foo"), + @NamedParam(value = "bar", type = String.class, required = true) + }) LinkedHashMap params) { + return "foo = " + params.get("foo") + ", bar = " + params.get("bar"); + } + + public static String myJavaMethod(@NamedParams({ + @NamedParam(value = "foo", type = String.class, required = true), + @NamedParam(value = "bar", type = Integer.class) + }) LinkedHashMap params, int num) { + return "foo = " + params.get("foo") + ", bar = " + params.get("bar") + ", num = " + num; + } +} http://git-wip-us.apache.org/repos/asf/groovy/blob/3bdea65e/src/test/groovy/NamedParameterTest.groovy ---------------------------------------------------------------------- diff --git a/src/test/groovy/NamedParameterTest.groovy b/src/test/groovy/NamedParameterTest.groovy index 36bdde4..b0cc07b 100644 --- a/src/test/groovy/NamedParameterTest.groovy +++ b/src/test/groovy/NamedParameterTest.groovy @@ -18,6 +18,12 @@ */ package groovy +import groovy.transform.NamedParam +import groovy.transform.NamedParams +import groovy.transform.TypeChecked + +import static groovy.NamedParameterHelper.myJavaMethod + class NamedParameterTest extends GroovyTestCase { void testPassingNamedParametersToMethod() { @@ -48,4 +54,85 @@ class NamedParameterTest extends GroovyTestCase { times: 2 } + + @TypeChecked + void testNamedParamsAnnotation() { + assert myJavaMethod(foo: 'FOO', bar: 'BAR') == 'foo = FOO, bar = BAR' + assert myJavaMethod(bar: 'BAR') == 'foo = null, bar = BAR' + assert myJavaMethod(foo: 'FOO', bar: 25, 42) == 'foo = FOO, bar = 25, num = 42' + assert myJavaMethod(foo: 'FOO', 142) == 'foo = FOO, bar = null, num = 142' + assert myMethod(foo: 'FOO', bar: 'BAR') == 'foo = FOO, bar = BAR' + assert myMethod(bar: 'BAR') == 'foo = null, bar = BAR' + assert myMethod(foo: 'FOO', bar: 35,242) == 'foo = FOO, bar = 35, num = 242' + assert myMethod(foo: 'FOO', 342) == 'foo = FOO, bar = null, num = 342' + assertScript ''' + import groovy.transform.TypeChecked + import static groovy.NamedParameterTest.myMethod + + @TypeChecked + def method() { + assert myMethod(foo: 'FOO', bar: 'BAR') == 'foo = FOO, bar = BAR' + assert myMethod(bar: 'BAR') == 'foo = null, bar = BAR' + assert myMethod(foo: 'FOO', bar: 45, 442) == 'foo = FOO, bar = 45, num = 442' + assert myMethod(foo: 'FOO', 542) == 'foo = FOO, bar = null, num = 542' + } + method() + ''' + } + + void testMissingRequiredName() { + def message = shouldFail ''' + import groovy.transform.TypeChecked + import static groovy.NamedParameterTest.myMethod + + @TypeChecked + def method() { + myMethod(foo: 'FOO') + } + method() + ''' + assert message.contains("required named arg 'bar' not found") + } + + void testUnknownName() { + def message = shouldFail ''' + import groovy.transform.TypeChecked + import static groovy.NamedParameterTest.myMethod + + @TypeChecked + def method() { + myMethod(bar: 'BAR', baz: 'BAZ') + } + method() + ''' + assert message.contains("unexpected named arg: baz") + } + + void testInvalidType() { + def message = shouldFail ''' + import groovy.transform.TypeChecked + import static groovy.NamedParameterTest.myMethod + + @TypeChecked + def method() { + myMethod(foo: 42, 42) + } + method() + ''' + assert message.contains("parameter for named arg 'foo' has type 'int' but expected 'java.lang.String'") + } + + static String myMethod(@NamedParams([ + @NamedParam(value = "foo"), + @NamedParam(value = "bar", type = String, required = true) + ]) Map params) { + "foo = $params.foo, bar = $params.bar" + } + + static String myMethod(@NamedParams([ + @NamedParam(value = "foo", type = String, required = true), + @NamedParam(value = "bar", type = Integer) + ]) Map params, int num) { + "foo = $params.foo, bar = $params.bar, num = $num" + } } http://git-wip-us.apache.org/repos/asf/groovy/blob/3bdea65e/subprojects/parser-antlr4/src/main/java/org/apache/groovy/parser/antlr4/AstBuilder.java ---------------------------------------------------------------------- diff --git a/subprojects/parser-antlr4/src/main/java/org/apache/groovy/parser/antlr4/AstBuilder.java b/subprojects/parser-antlr4/src/main/java/org/apache/groovy/parser/antlr4/AstBuilder.java index e3dd188..2b0dd50 100644 --- a/subprojects/parser-antlr4/src/main/java/org/apache/groovy/parser/antlr4/AstBuilder.java +++ b/subprojects/parser-antlr4/src/main/java/org/apache/groovy/parser/antlr4/AstBuilder.java @@ -2632,8 +2632,7 @@ public class AstBuilder extends GroovyParserBaseVisitor<Object> implements Groov if (asBoolean(mapEntryExpressionList) && asBoolean(expressionList)) { // e.g. arguments like x: 1, 'a', y: 2, 'b', z: 3 ArgumentListExpression argumentListExpression = new ArgumentListExpression(expressionList); - argumentListExpression.getExpressions().add(0, new MapExpression(mapEntryExpressionList)); // TODO: confirm BUG OR NOT? All map entries will be put at first, which is not friendly to Groovy developers - + argumentListExpression.getExpressions().add(0, configureAST(new MapExpression(mapEntryExpressionList), ctx)); return configureAST(argumentListExpression, ctx); }