Skip to content

Commit

Permalink
Fix for #1148: normalize handling for map fields, methods and properties
Browse files Browse the repository at this point in the history 
- disable code hover/select for map key
- highlight property expression as MAP_KEY
- propose only fields for "map.@x"
- propose only methods for "map.&x" or "map::x"
- propose only methods for "map.x" (no Java-bean variants)
- change inferred type of getAt(Object,String) extension method

#1143
  • Loading branch information
@eric-milles
eric-milles committed Jul 29, 2020
1 parent 2ef5443 commit 9651089
Show file tree
Hide file tree
Showing 16 changed files with 451 additions and 127 deletions.
235 changes: 199 additions & 36 deletions ...e.tests.builder/src/org/eclipse/jdt/core/groovy/tests/search/GenericInferencingTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -382,6 +382,192 @@ public void testMap16() {
assertType(contents, "xxx", "java.util.Map<java.lang.Integer,java.lang.Boolean>");
}

@Test
public void testMap17() {
String contents =
"def map = [key:'val']\n" +
"map.getAt('key').toUpperCase()\n" +
"map.get('key').toUpperCase()\n" +
"map['key'].toUpperCase()\n" +
"map['key'] = 'value'\n" +
"map.key.toUpperCase()\n" +
"map.key = 'value'\n";

int offset = contents.indexOf("map");
assertType(contents, offset, offset + 3, "java.util.Map<java.lang.String,java.lang.String>");

offset = contents.indexOf("toUpperCase");
assertDeclaringType(contents, offset, offset + 11, "java.lang.String");

offset = contents.indexOf("toUpperCase", offset + 1);
assertDeclaringType(contents, offset, offset + 11, "java.lang.String");

offset = contents.indexOf("toUpperCase", offset + 1);
assertDeclaringType(contents, offset, offset + 11, "java.lang.String");

offset = contents.indexOf("map['key'] = 'value'", offset + 1);
assertType(contents, offset, offset + "map['key'] = 'value'".length(), "java.lang.String");

offset = contents.indexOf("toUpperCase", offset + 1);
assertDeclaringType(contents, offset, offset + 11, "java.lang.String");

offset = contents.indexOf("map.key = 'value'", offset + 1);
assertType(contents, offset, offset + "map.key = 'value'".length(), "java.lang.String");
}

@Test
public void testMap18() {
String contents =
"import groovy.transform.stc.*\n" +
"def map = [key:'val']\n" +
"with(map) {\n" +
" it.getAt('key').toUpperCase()\n" +
" getAt('key').toUpperCase()\n" +
" it.get('key').toUpperCase()\n" +
" get('key').toUpperCase()\n" +
" it['key'].toUpperCase()\n" +
" it['key'] = 'value'\n" +
" it.key.toUpperCase()\n" +
" key.toUpperCase()\n" +
" it.key = 'value'\n" +
" key = 'value'\n" +
"}\n" +
"private <T> void with(@DelegatesTo.Target T map, @DelegatesTo(strategy=Closure.DELEGATE_FIRST) @ClosureParams(FirstParam) Closure block) {\n" +
" map.with(block)\n" +
"}\n";

int offset = contents.indexOf("it."); // line 4
assertType(contents, offset, offset + 2, "java.util.Map<java.lang.String,java.lang.String>");

offset = contents.indexOf("toUpperCase");
assertDeclaringType(contents, offset, offset + 11, "java.lang.String");

offset = contents.indexOf("toUpperCase", offset + 1);
assertDeclaringType(contents, offset, offset + 11, "java.lang.String");

offset = contents.indexOf("toUpperCase", offset + 1);
assertDeclaringType(contents, offset, offset + 11, "java.lang.String");

offset = contents.indexOf("toUpperCase", offset + 1);
assertDeclaringType(contents, offset, offset + 11, "java.lang.String");

offset = contents.indexOf("toUpperCase", offset + 1);
assertDeclaringType(contents, offset, offset + 11, "java.lang.String");

offset = contents.indexOf("it['key'] = 'value'", offset + 1);
assertType(contents, offset, offset + "it['key'] = 'value'".length(), "java.lang.String");

offset = contents.indexOf("toUpperCase", offset + 1);
assertDeclaringType(contents, offset, offset + 11, "java.lang.String");

offset = contents.indexOf("toUpperCase", offset + 1);
assertDeclaringType(contents, offset, offset + 11, "java.lang.String");

offset = contents.indexOf("it.key = 'value'", offset + 1);
assertType(contents, offset, offset + "it.key = 'value'".length(), "java.lang.String");

offset = contents.lastIndexOf("key = 'value'");
assertType(contents, offset, offset + "key = 'value'".length(), "java.lang.String");
}

@Test // methods and property resolution differs
public void testMap19() {
String contents =
"def map = [foo:'bar']\n" +
"map.getMetaClass()\n" +
"map.metaClass\n" +
"map.getClass()\n" +
"map.class\n" +
"map.with {\n" +
" getMetaClass()\n" +
" metaClass\n" +
" isEmpty()\n" +
" empty\n" +
"}\n";

int offset = contents.indexOf("map");
assertType(contents, offset, offset + 3, "java.util.Map<java.lang.String,java.lang.String>");

offset = contents.indexOf("getMetaClass");
assertType(contents, offset, offset + "getMetaClass".length(), "groovy.lang.MetaClass");

offset = contents.indexOf("metaClass");
assertType(contents, offset, offset + "metaClass".length(), "java.lang.String");

offset = contents.indexOf("getClass");
assertType(contents, offset, offset + "getClass".length(), "java.lang.Class<? extends java.util.Map<java.lang.String,java.lang.String>>");

offset = contents.indexOf("class");
assertType(contents, offset, offset + "class".length(), "java.lang.String");

//

offset = contents.lastIndexOf("getMetaClass");
assertType(contents, offset, offset + "getMetaClass".length(), "groovy.lang.MetaClass");

offset = contents.lastIndexOf("metaClass");
assertType(contents, offset, offset + "metaClass".length(), "java.lang.String");

offset = contents.indexOf("isEmpty");
assertType(contents, offset, offset + "isEmpty".length(), "java.lang.Boolean");

offset = contents.indexOf("empty");
assertType(contents, offset, offset + "empty".length(), "java.lang.String");
}

@Test
public void testMap20() {
String contents =
"LinkedHashMap<String,String> map = [foo:'bar']\n" +
"def put = map.&put\n" +
"put('key', 'val')\n" +
"map.@accessOrder\n" +
"map.@class\n" +
"map.&getAt\n" +
"map.with {\n" +
" put = it.&put\n" +
" put('key', 'val')\n" +
" it.@accessOrder\n" +
" it.@class\n" +
" it.&getAt\n" +
"}\n";

int offset = contents.indexOf("map");
assertType(contents, offset, offset + 3, "java.util.LinkedHashMap<java.lang.String,java.lang.String>");

offset = contents.indexOf(".&put") + 2;
assertDeclaration(contents, offset, offset + 3, "java.util.HashMap<java.lang.String,java.lang.String>", "put", DeclarationKind.METHOD);

offset = contents.indexOf("put(");
assertType(contents, offset, offset + 3, "groovy.lang.Closure<java.lang.String>");

offset = contents.indexOf("accessOrder");
assertType(contents, offset, offset + "accessOrder".length(), "java.lang.Boolean");

offset = contents.indexOf("class");
assertUnknownConfidence(contents, offset, offset + "class".length());

offset = contents.indexOf("getAt");
assertDeclaringType(contents, offset, offset + "getAt".length(), "org.codehaus.groovy.runtime.DefaultGroovyMethods");

//

offset = contents.lastIndexOf(".&put") + 2;
assertDeclaration(contents, offset, offset + 3, "java.util.HashMap<java.lang.String,java.lang.String>", "put", DeclarationKind.METHOD);

offset = contents.lastIndexOf("put(");
assertType(contents, offset, offset + 3, "groovy.lang.Closure<java.lang.String>");

offset = contents.lastIndexOf("accessOrder");
assertType(contents, offset, offset + "accessOrder".length(), "java.lang.Boolean");

offset = contents.lastIndexOf("class");
assertUnknownConfidence(contents, offset, offset + "class".length());

offset = contents.lastIndexOf("getAt");
assertDeclaringType(contents, offset, offset + "getAt".length(), "org.codehaus.groovy.runtime.DefaultGroovyMethods");
}

@Test
public void testListOfMap1() {
String contents =
Expand Down Expand Up @@ -663,10 +849,7 @@ public void testNestedGenerics1() {
"class MyMap extends HashMap<String,Class> {\n}\n" +
"MyMap m\n" +
"m.get('')";
String toFind = "get";
int start = contents.lastIndexOf(toFind);
int end = start + toFind.length();
assertType(contents, start, end, "java.lang.Class");
assertType(contents, "get", "java.lang.Class");
}

@Test // GRECLIPSE-997
Expand All @@ -675,10 +858,7 @@ public void testNestedGenerics2() {
"class MyMap extends HashMap<String,Class> {\n}\n" +
"MyMap m\n" +
"m.entrySet()";
String toFind = "entrySet";
int start = contents.lastIndexOf(toFind);
int end = start + toFind.length();
assertType(contents, start, end, "java.util.Set<java.util.Map$Entry<java.lang.String,java.lang.Class>>");
assertType(contents, "entrySet", "java.util.Set<java.util.Map$Entry<java.lang.String,java.lang.Class>>");
}

@Test // GRECLIPSE-997
Expand All @@ -688,10 +868,7 @@ public void testNestedGenerics3() {
"class MyMap<K,V> extends HashMap<K,WeakReference<V>>{\n}\n" +
"MyMap<String,Class> m\n" +
"m.entrySet()";
String toFind = "entrySet";
int start = contents.lastIndexOf(toFind);
int end = start + toFind.length();
assertType(contents, start, end, "java.util.Set<java.util.Map$Entry<java.lang.String,java.lang.ref.WeakReference<java.lang.Class>>>");
assertType(contents, "entrySet", "java.util.Set<java.util.Map$Entry<java.lang.String,java.lang.ref.WeakReference<java.lang.Class>>>");
}

@Test // GRECLIPSE-997
Expand All @@ -702,26 +879,20 @@ public void testNestedGenerics4() {
"class MySubMap extends MyMap<String,Class>{\n}\n" +
"MySubMap m\n" +
"m.entrySet()";
String toFind = "entrySet";
int start = contents.lastIndexOf(toFind);
int end = start + toFind.length();
assertType(contents, start, end, "java.util.Set<java.util.Map$Entry<java.lang.String,java.lang.ref.WeakReference<java.lang.Class>>>");
assertType(contents, "entrySet", "java.util.Set<java.util.Map$Entry<java.lang.String,java.lang.ref.WeakReference<java.lang.Class>>>");
}

@Test // GRECLIPSE-997
public void testNestedGenerics5() {
String contents =
"import java.lang.ref.WeakReference\n" +
"class MyMap<K,V> extends HashMap<K,WeakReference<V>>{\n}\n" +
"class MySubMap<L> extends MyMap<String,Class>{ \n" +
"class MySubMap<L> extends MyMap<String,Class>{\n" +
" Map<L,Class> val\n" +
"}\n" +
"MySubMap<Integer> m\n" +
"m.val";
String toFind = "val";
int start = contents.lastIndexOf(toFind);
int end = start + toFind.length();
assertType(contents, start, end, "java.util.Map<java.lang.Integer,java.lang.Class>");
"m.@val";
assertType(contents, "val", "java.util.Map<java.lang.Integer,java.lang.Class>");
}

@Test // GRECLIPSE-997
Expand All @@ -732,10 +903,7 @@ public void testNestedGenerics6() {
"class MySubMap extends MyMap<String,Class>{\n}\n" +
"MySubMap m\n" +
"m.entrySet()";
String toFind = "entrySet";
int start = contents.lastIndexOf(toFind);
int end = start + toFind.length();
assertType(contents, start, end, "java.util.Set<java.util.Map$Entry<java.lang.String,java.lang.ref.WeakReference<java.util.List<java.lang.String>>>>");
assertType(contents, "entrySet", "java.util.Set<java.util.Map$Entry<java.lang.String,java.lang.ref.WeakReference<java.util.List<java.lang.String>>>>");
}

@Test // GRECLIPSE-997
Expand All @@ -744,23 +912,18 @@ public void testNestedGenerics7() {
"class MyMap<K,V> extends HashMap<V,K>{\n}\n" +
"MyMap<Integer,Class> m\n" +
"m.get(Object)";
String toFind = "get";
int start = contents.lastIndexOf(toFind);
int end = start + toFind.length();
assertType(contents, start, end, "java.lang.Integer");
assertType(contents, "get", "java.lang.Integer");
}

@Test // GRECLIPSE-997
public void testNestedGenerics8() {
String contents =
"class MyMap<K,V> extends HashMap<K,V>{\n" +
"Map<V,Class<K>> val}\n" +
" Map<V,Class<K>> val\n" +
"}\n" +
"MyMap<Integer,Class> m\n" +
"m.val";
String toFind = "val";
int start = contents.lastIndexOf(toFind);
int end = start + toFind.length();
assertType(contents, start, end, "java.util.Map<java.lang.Class,java.lang.Class<java.lang.Integer>>");
"m.@val";
assertType(contents, "val", "java.util.Map<java.lang.Class,java.lang.Class<java.lang.Integer>>");
}

@Test
Expand Down
4 changes: 1 addition & 3 deletions ...lipse.jdt.groovy.core/src/org/codehaus/jdt/groovy/internal/compiler/ast/JDTClassNode.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -581,8 +581,7 @@ public List<PropertyNode> getProperties() {

for (PropertyNode node : groovyTypeDecl.getClassNode().getProperties()) {
FieldNode field = getField(node.getName());
boolean synth = (field == null);
if (synth) {
if (field == null) {
field = new FieldNode(node.getName(), Flags.AccPrivate | (node.getModifiers() & Flags.AccStatic), resolver.resolve(node.getType().getName()), this, null);
field.setDeclaringClass(this);
field.setSourcePosition(node.getField());
Expand All @@ -592,7 +591,6 @@ public List<PropertyNode> getProperties() {
PropertyNode clone = new PropertyNode(field, node.getModifiers(), null, null);
clone.setDeclaringClass(this);
clone.setSourcePosition(node);
clone.setSynthetic(synth);

nodes.add(clone);
}
Expand Down
27 changes: 26 additions & 1 deletion base/org.eclipse.jdt.groovy.core/src/org/eclipse/jdt/groovy/search/CategoryTypeLookup.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,13 +15,16 @@
*/
package org.eclipse.jdt.groovy.search;

import static org.codehaus.groovy.ast.tools.GeneralUtils.isOrImplements;
import static org.codehaus.groovy.runtime.DefaultGroovyMethods.first;
import static org.codehaus.groovy.runtime.DefaultGroovyMethods.tail;

import java.util.ArrayList;
import java.util.List;
import java.util.Map;

import org.codehaus.groovy.ast.ClassNode;
import org.codehaus.groovy.ast.GenericsType;
import org.codehaus.groovy.ast.MethodNode;
import org.codehaus.groovy.ast.Parameter;
import org.codehaus.groovy.ast.expr.AttributeExpression;
Expand Down Expand Up @@ -86,7 +89,25 @@ public TypeLookupResult lookupType(final Expression node, final VariableScope sc

MethodNode method = selectBestMatch(candidates, argumentTypes, scope);
if (method != null) {
TypeLookupResult result = new TypeLookupResult(method.getReturnType(), method.getDeclaringClass(), method,
ClassNode resolvedType = method.getReturnType();
// getAt(Object,String):Object supersedes getAt(Map<K,V>,Object):V when first param is String or GString; restore return type V for user experience
if ("getAt".equals(simpleName) && VariableScope.OBJECT_CLASS_NODE.equals(resolvedType) && isOrImplements(selfType, VariableScope.MAP_CLASS_NODE)) {
for (ClassNode face : selfType.getAllInterfaces()) {
if (face.equals(VariableScope.MAP_CLASS_NODE)) { // Map<K,V>
GenericsType[] generics = GroovyUtils.getGenericsTypes(face);
if (generics.length == 2) resolvedType = generics[1].getType();
break;
}
}
}

// must resolve generics here because TypeLookupResult uses declaring class (instead of self type)
if (org.codehaus.groovy.transform.stc.StaticTypeCheckingSupport.missesGenericsTypes(resolvedType)) {
GenericsMapper mapper = GenericsMapper.gatherGenerics(selfType, selfType.redirect());
resolvedType = VariableScope.resolveTypeParameterization(mapper, VariableScope.clone(resolvedType));
}

TypeLookupResult result = new TypeLookupResult(resolvedType, method.getDeclaringClass(), method,
isDefaultGroovyMethod(method, scope) ? TypeConfidence.LOOSELY_INFERRED : TypeConfidence.INFERRED, scope);
result.isGroovy = true; // enable semantic highlighting as Groovy method
return result;
Expand Down Expand Up @@ -179,6 +200,10 @@ private static MethodNode selectBestMatch(final List<MethodNode> candidates, fin
}
}

if (scope.getEnclosingNode() instanceof MethodPointerExpression) {
return first(m.values());
}

// Phase 2: find best set of parameters for given call arguments
MethodNode method = null;
for (MethodNode candidate : m.values()) {
Expand Down
Loading

0 comments on commit 9651089

Please sign in to comment.