[GitHub] [groovy] paulk-asert commented on a change in pull request #1498: GROOVY-9951: set implicit-this to false for method call expressions

[GitBox <gi...@apache.org>]

paulk-asert commented on a change in pull request #1498:
URL: https://github.com/apache/groovy/pull/1498#discussion_r582710921



##########
File path: src/main/java/org/apache/groovy/ast/tools/ConstructorNodeUtils.java
##########
@@ -88,17 +91,20 @@ public static Statement checkPropNamesS(final VariableExpression namedArgs, fina
         if (!pojo) {
             return stmt(callX(IMMUTABLE_TYPE, "checkPropNames", args(varX("this"), namedArgs)));
         }
-        BlockStatement block = new BlockStatement();
-        ListExpression knownPropNames = new ListExpression();
-        for (PropertyNode pNode : props) {
-            knownPropNames.addExpression(constX(pNode.getName()));
-        }
-        VariableExpression validNames = localVarX("validNames", ClassHelper.LIST_TYPE);
+
+        Expression validNames = localVarX("validNames", ClassHelper.LIST_TYPE);
         Parameter name = param(ClassHelper.STRING_TYPE, "arg");
-        Statement loopS = ifS(notX(callX(validNames, "contains", varX(name))),
-                throwS(ctorX(EXCEPTION, plusX(constX("Unknown named argument: "), varX(name)))));
-        block.addStatement(declS(validNames, knownPropNames));
-        block.addStatement(forS(name, callX(namedArgs, "keySet"), loopS));
-        return block;
+
+        MethodCallExpression names = callX(namedArgs, "keySet");
+        names.setImplicitThis(false);
+
+        MethodCallExpression isNameValid = callX(validNames, "contains", varX(name));
+        isNameValid.setImplicitThis(false);
+
+        return block(
+            declS(validNames, listX(props.stream().map(p -> constX(p.getName())).collect(toList()))),
+            forS(name, names, ifS(notX(isNameValid),
+                    throwS(ctorX(EXCEPTION, plusX(constX("Unknown named argument: "), varX(name))))))

Review comment:
       This all LGTM but we can probably go one step further for the plusX expression if `pojo` is true. At the moment it ends up with a call to `StringGroovyMethods#plus` but we could do such a thing without needing the Groovy runtime (for the `pojo` case). There might be multiple ways to do that and we can do that as a next step.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org