You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@groovy.apache.org by danielsun1106 <gi...@git.apache.org> on 2018/05/17 01:50:06 UTC
[GitHub] groovy pull request #708: GROOVY-6668: Static compiler doesn't coerce GStrin...
GitHub user danielsun1106 opened a pull request:
https://github.com/apache/groovy/pull/708
GROOVY-6668: Static compiler doesn't coerce GString for getAt() call
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/danielsun1106/groovy GROOVY-6668
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/groovy/pull/708.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #708
----
commit f3c3939b473c33acf0de5d95074bc2eb8bd61555
Author: Daniel Sun <re...@...>
Date: 2018-05-17T01:49:00Z
GROOVY-6668: Static compiler doesn't coerce GString for getAt() call
----
---
[GitHub] groovy pull request #708: GROOVY-6668: Static compiler doesn't coerce GStrin...
Posted by paulk-asert <gi...@git.apache.org>.
Github user paulk-asert commented on a diff in the pull request:
https://github.com/apache/groovy/pull/708#discussion_r188832656
--- Diff: src/test/groovy/bugs/Groovy6668Bug.groovy ---
@@ -0,0 +1,65 @@
+/*
+ * 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.bugs
+
+class Groovy6668Bug extends GroovyTestCase{
+ void testGroovy6668() {
+ assertScript '''
+ @groovy.transform.CompileStatic
+ class OtherThing {
+ OtherThing() {
+ Map<String, String> m = [:]
+ def k = "foo"
+ m["$k"].toUpperCase()
+ }
+ }
+
+ OtherThing
--- End diff --
why not just shouldCompile then?
---
[GitHub] groovy pull request #708: GROOVY-6668: Static compiler doesn't coerce GStrin...
Posted by danielsun1106 <gi...@git.apache.org>.
Github user danielsun1106 commented on a diff in the pull request:
https://github.com/apache/groovy/pull/708#discussion_r188837462
--- Diff: src/test/groovy/bugs/Groovy6668Bug.groovy ---
@@ -0,0 +1,65 @@
+/*
+ * 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.bugs
+
+class Groovy6668Bug extends GroovyTestCase{
+ void testGroovy6668() {
+ assertScript '''
+ @groovy.transform.CompileStatic
+ class OtherThing {
+ OtherThing() {
+ Map<String, String> m = [:]
+ def k = "foo"
+ m["$k"].toUpperCase()
+ }
+ }
+
+ OtherThing
--- End diff --
I've fixed that according your suggestion :-)
---
[GitHub] groovy pull request #708: GROOVY-6668: Static compiler doesn't coerce GStrin...
Posted by danielsun1106 <gi...@git.apache.org>.
Github user danielsun1106 commented on a diff in the pull request:
https://github.com/apache/groovy/pull/708#discussion_r188825988
--- Diff: src/main/java/org/codehaus/groovy/transform/stc/StaticTypeCheckingVisitor.java ---
@@ -4101,13 +4101,17 @@ protected static ClassNode getGroupOperationResultType(ClassNode a, ClassNode b)
return Number_TYPE;
}
- protected ClassNode inferComponentType(final ClassNode containerType, final ClassNode indexType) {
+ private static ClassNode convertGStringToStringType(ClassNode cn) {
+ return GSTRING_TYPE.equals(cn) ? STRING_TYPE : cn;
+ }
+
+ protected ClassNode inferComponentType(final ClassNode containerType, ClassNode indexType) {
final ClassNode componentType = containerType.getComponentType();
if (componentType == null) {
// GROOVY-5521
// try to identify a getAt method
typeCheckingContext.pushErrorCollector();
- MethodCallExpression vcall = callX(varX("_hash_", containerType), "getAt", varX("_index_", indexType));
+ MethodCallExpression vcall = callX(varX("_hash_", containerType), "getAt", varX("_index_", convertGStringToStringType(indexType)));
--- End diff --
Good idea!
```groovy
/**
* Get the value via the string value of the key
* See GROOVY-6668 and the PR#708 (https://github.com/apache/groovy/pull/708)
*
* @param self a map
* @param key a key
* @param <V> value type
* @return the value
*/
public static <V> V getAt(Map<?, V> self, GString key) {
if (null == key) {
return null;
}
return self.get(key.toString());
}
```
but I wonder why we should not treat GString as String?
As a groovy user, I won't write code like `GString str = "${'someString'}"`, and I just write `String str = "${'someString'}"` or `def str = "${'someString'}"` then treat `str` as a `String`...
---
[GitHub] groovy pull request #708: GROOVY-6668: Static compiler doesn't coerce GStrin...
Posted by danielsun1106 <gi...@git.apache.org>.
Github user danielsun1106 closed the pull request at:
https://github.com/apache/groovy/pull/708
---
[GitHub] groovy pull request #708: GROOVY-6668: Static compiler doesn't coerce GStrin...
Posted by blackdrag <gi...@git.apache.org>.
Github user blackdrag commented on a diff in the pull request:
https://github.com/apache/groovy/pull/708#discussion_r188869080
--- Diff: src/main/java/org/codehaus/groovy/transform/stc/StaticTypeCheckingVisitor.java ---
@@ -4101,13 +4101,17 @@ protected static ClassNode getGroupOperationResultType(ClassNode a, ClassNode b)
return Number_TYPE;
}
- protected ClassNode inferComponentType(final ClassNode containerType, final ClassNode indexType) {
+ private static ClassNode convertGStringToStringType(ClassNode cn) {
+ return GSTRING_TYPE.equals(cn) ? STRING_TYPE : cn;
+ }
+
+ protected ClassNode inferComponentType(final ClassNode containerType, ClassNode indexType) {
final ClassNode componentType = containerType.getComponentType();
if (componentType == null) {
// GROOVY-5521
// try to identify a getAt method
typeCheckingContext.pushErrorCollector();
- MethodCallExpression vcall = callX(varX("_hash_", containerType), "getAt", varX("_index_", indexType));
+ MethodCallExpression vcall = callX(varX("_hash_", containerType), "getAt", varX("_index_", convertGStringToStringType(indexType)));
--- End diff --
we can treat GString as a String if we know the target is a String. Plus the rules for assignment and for method calls are not identical. But my comment was not about adding getAt(GString) for Map. My comment was that this change looks like the following will no longer work:
class X {
int getAt(GString gs) {1}
}
assert new X()["${42}"] == 1
---
[GitHub] groovy pull request #708: GROOVY-6668: Static compiler doesn't coerce GStrin...
Posted by danielsun1106 <gi...@git.apache.org>.
Github user danielsun1106 commented on a diff in the pull request:
https://github.com/apache/groovy/pull/708#discussion_r188826337
--- Diff: src/test/groovy/bugs/Groovy6668Bug.groovy ---
@@ -0,0 +1,65 @@
+/*
+ * 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.bugs
+
+class Groovy6668Bug extends GroovyTestCase{
+ void testGroovy6668() {
+ assertScript '''
+ @groovy.transform.CompileStatic
+ class OtherThing {
+ OtherThing() {
+ Map<String, String> m = [:]
+ def k = "foo"
+ m["$k"].toUpperCase()
+ }
+ }
+
+ OtherThing
--- End diff --
I just keep the code of the issue as it is. the `new OtherThing()` version is `testGroovy6668WithData` ;-)
---
[GitHub] groovy pull request #708: GROOVY-6668: Static compiler doesn't coerce GStrin...
Posted by blackdrag <gi...@git.apache.org>.
Github user blackdrag commented on a diff in the pull request:
https://github.com/apache/groovy/pull/708#discussion_r188821809
--- Diff: src/main/java/org/codehaus/groovy/transform/stc/StaticTypeCheckingVisitor.java ---
@@ -4101,13 +4101,17 @@ protected static ClassNode getGroupOperationResultType(ClassNode a, ClassNode b)
return Number_TYPE;
}
- protected ClassNode inferComponentType(final ClassNode containerType, final ClassNode indexType) {
+ private static ClassNode convertGStringToStringType(ClassNode cn) {
+ return GSTRING_TYPE.equals(cn) ? STRING_TYPE : cn;
+ }
+
+ protected ClassNode inferComponentType(final ClassNode containerType, ClassNode indexType) {
final ClassNode componentType = containerType.getComponentType();
if (componentType == null) {
// GROOVY-5521
// try to identify a getAt method
typeCheckingContext.pushErrorCollector();
- MethodCallExpression vcall = callX(varX("_hash_", containerType), "getAt", varX("_index_", indexType));
+ MethodCallExpression vcall = callX(varX("_hash_", containerType), "getAt", varX("_index_", convertGStringToStringType(indexType)));
--- End diff --
what if I add a DGM getAt(GString) for Map? This code looks like I would no longer be able to call it
---
[GitHub] groovy pull request #708: GROOVY-6668: Static compiler doesn't coerce GStrin...
Posted by blackdrag <gi...@git.apache.org>.
Github user blackdrag commented on a diff in the pull request:
https://github.com/apache/groovy/pull/708#discussion_r188821671
--- Diff: src/main/java/org/codehaus/groovy/classgen/asm/sc/StaticInvocationWriter.java ---
@@ -468,7 +469,11 @@ protected void loadArguments(List<Expression> argumentList, Parameter[] para) {
expression.putNodeMetaData(PARAMETER_TYPE, para[i].getType());
expression.visit(acg);
if (!isNullConstant(expression)) {
- operandStack.doGroovyCast(para[i].getType());
+ if (expression instanceof GStringExpression) {
+ operandStack.doGroovyCast(ClassHelper.STRING_TYPE);
--- End diff --
what if the method has a GString parameter? Converting to string is then wrong. You should add a test for this
---
[GitHub] groovy pull request #708: GROOVY-6668: Static compiler doesn't coerce GStrin...
Posted by blackdrag <gi...@git.apache.org>.
Github user blackdrag commented on a diff in the pull request:
https://github.com/apache/groovy/pull/708#discussion_r188822722
--- Diff: src/test/groovy/bugs/Groovy6668Bug.groovy ---
@@ -0,0 +1,65 @@
+/*
+ * 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.bugs
+
+class Groovy6668Bug extends GroovyTestCase{
+ void testGroovy6668() {
+ assertScript '''
+ @groovy.transform.CompileStatic
+ class OtherThing {
+ OtherThing() {
+ Map<String, String> m = [:]
+ def k = "foo"
+ m["$k"].toUpperCase()
+ }
+ }
+
+ OtherThing
--- End diff --
new OtherThing()?
---