From eb98570f7c34f4a4e64b563029ac3211bb5a91d9 Mon Sep 17 00:00:00 2001 From: David Thompson Date: Thu, 24 Oct 2024 08:26:32 -0400 Subject: [PATCH] Improve completion in extends/implements - Change dom conversion logic to recover incomplete type names properly - Restrict the suggested classes based on if they are a class or interface - Do not suggest final classes - Do not suggest the current class Signed-off-by: David Thompson --- .../eclipse/jdt/core/dom/JavacConverter.java | 6 +- .../internal/javac/dom/JavacTypeBinding.java | 19 +-- .../codeassist/DOMCompletionEngine.java | 117 ++++++++++++++++-- 3 files changed, 124 insertions(+), 18 deletions(-) diff --git a/org.eclipse.jdt.core.javac/src/org/eclipse/jdt/core/dom/JavacConverter.java b/org.eclipse.jdt.core.javac/src/org/eclipse/jdt/core/dom/JavacConverter.java index 678b59f5f17..dfbb960f1bd 100644 --- a/org.eclipse.jdt.core.javac/src/org/eclipse/jdt/core/dom/JavacConverter.java +++ b/org.eclipse.jdt.core.javac/src/org/eclipse/jdt/core/dom/JavacConverter.java @@ -2904,7 +2904,9 @@ Type convertToType(JCTree javac) { // the name second segment is invalid simpleName.delete(); return qualifierType; - } else { // lombok case + } else { + // lombok case + // or empty (eg `test.`) simpleName.setSourceRange(qualifierType.getStartPosition(), 0); } if(qualifierType instanceof SimpleType simpleType && (ast.apiLevel() < AST.JLS8 || simpleType.annotations().isEmpty())) { @@ -2913,7 +2915,7 @@ Type convertToType(JCTree javac) { parentName.setParent(null, null); QualifiedName name = this.ast.newQualifiedName(simpleType.getName(), simpleName); commonSettings(name, javac); - int length = name.getName().getStartPosition() + name.getName().getLength() - name.getStartPosition(); + int length = simpleType.getName().getLength() + 1 + simpleName.getLength(); if (name.getStartPosition() >= 0) { name.setSourceRange(name.getStartPosition(), Math.max(0, length)); } diff --git a/org.eclipse.jdt.core.javac/src/org/eclipse/jdt/internal/javac/dom/JavacTypeBinding.java b/org.eclipse.jdt.core.javac/src/org/eclipse/jdt/internal/javac/dom/JavacTypeBinding.java index c13898dcfdc..07f26320f35 100644 --- a/org.eclipse.jdt.core.javac/src/org/eclipse/jdt/internal/javac/dom/JavacTypeBinding.java +++ b/org.eclipse.jdt.core.javac/src/org/eclipse/jdt/internal/javac/dom/JavacTypeBinding.java @@ -351,6 +351,12 @@ static void getKey(StringBuilder builder, Type typeToBuild, Name n, boolean isLe } } + /* + * TODO - this name 'n' might be something like test0502.A$1 + * but the test suite expects test0502.A$182, + * where 182 is the location in the source of the symbol. + */ + builder.append(n.toString().replace('.', '/')); // This is a hack and will likely need to be enhanced if (typeToBuild.tsym instanceof ClassSymbol classSymbol && !(classSymbol.type instanceof ErrorType) && classSymbol.owner instanceof PackageSymbol) { JavaFileObject sourcefile = classSymbol.sourcefile; @@ -363,17 +369,14 @@ static void getKey(StringBuilder builder, Type typeToBuild, Name n, boolean isLe // probably: uri is not a valid path } if (fileName != null && !fileName.startsWith(classSymbol.getSimpleName().toString())) { - builder.append(fileName.substring(0, fileName.indexOf(".java"))); - builder.append("~"); + // There are multiple top-level types in this file, + // inject 'FileName~' before the type name to show that this type came from `FileName.java` + // (eg. Lorg/eclipse/jdt/FileName~MyTopLevelType;) + int simpleNameIndex = builder.lastIndexOf(classSymbol.getSimpleName().toString()); + builder.insert(simpleNameIndex, fileName.substring(0, fileName.indexOf(".java")) + "~"); } } } - /* - * TODO - this name 'n' might be something like test0502.A$1 - * but the test suite expects test0502.A$182, - * where 182 is the location in the source of the symbol. - */ - builder.append(n.toString().replace('.', '/')); boolean b1 = typeToBuild.isParameterized(); diff --git a/org.eclipse.jdt.core/codeassist/org/eclipse/jdt/internal/codeassist/DOMCompletionEngine.java b/org.eclipse.jdt.core/codeassist/org/eclipse/jdt/internal/codeassist/DOMCompletionEngine.java index c21db9e25c5..0c0c813c71a 100644 --- a/org.eclipse.jdt.core/codeassist/org/eclipse/jdt/internal/codeassist/DOMCompletionEngine.java +++ b/org.eclipse.jdt.core/codeassist/org/eclipse/jdt/internal/codeassist/DOMCompletionEngine.java @@ -216,6 +216,8 @@ public void run() { } else if (this.toComplete instanceof SimpleType simpleType) { if (FAKE_IDENTIFIER.equals(simpleType.getName().toString())) { context = this.toComplete.getParent(); + } else if (simpleType.getName() instanceof QualifiedName qualifiedName) { + context = qualifiedName; } } else if (this.toComplete instanceof Block block && this.offset == block.getStartPosition()) { context = this.toComplete.getParent(); @@ -226,6 +228,8 @@ public void run() { this.qualifiedPrefix = qualifiedName.getQualifier().toString(); } else if (this.toComplete != null && this.toComplete.getParent() instanceof QualifiedName qualifiedName) { this.qualifiedPrefix = qualifiedName.getQualifier().toString(); + } else if (this.toComplete instanceof SimpleType simpleType && simpleType.getName() instanceof QualifiedName qualifiedName) { + this.qualifiedPrefix = qualifiedName.getQualifier().toString(); } Bindings defaultCompletionBindings = new Bindings(); Bindings specificCompletionBindings = new Bindings(); @@ -387,6 +391,12 @@ public void run() { processMembers(qualifiedName, potentialBinding.get(), specificCompletionBindings, false); publishFromScope(specificCompletionBindings); suggestDefaultCompletions = false; + } else { + // maybe it is actually a package? + suggestPackages(); + // suggests types in the package + suggestTypesInPackage(qualifierPackageBinding.getName()); + suggestDefaultCompletions = false; } } } else if (qualifiedNameBinding instanceof IVariableBinding variableBinding) { @@ -441,9 +451,13 @@ public void run() { final int typeMatchRule = this.toComplete.getParent() instanceof Annotation ? IJavaSearchConstants.ANNOTATION_TYPE : IJavaSearchConstants.TYPE; + ExtendsOrImplementsInfo extendsOrImplementsInfo = isInExtendsOrImplements(this.toComplete); findTypes(completeAfter, typeMatchRule, null) .filter(type -> this.pattern.matchesName(this.prefix.toCharArray(), type.getElementName().toCharArray())) + .filter(type -> { + return filterBasedOnExtendsOrImplementsInfo(type, extendsOrImplementsInfo); + }) .map(this::toProposal).forEach(this.requestor::accept); } checkCancelled(); @@ -578,7 +592,7 @@ private void suggestPackages() { .filter(name -> !name.isBlank()) // the qualifier must match exactly. only the last segment is (potentially) fuzzy matched. // However, do not match the already completed package name! - .filter(name -> CharOperation.prefixEquals(this.qualifiedPrefix.toCharArray(), name.toCharArray()) && name.length() > this.qualifiedPrefix.length()) + .filter(name -> CharOperation.prefixEquals((this.qualifiedPrefix + ".").toCharArray(), name.toCharArray()) && name.length() > this.qualifiedPrefix.length()) //$NON-NLS-1$ .filter(name -> this.pattern.matchesName(this.prefix.toCharArray(), name.toCharArray())) .map(pack -> toPackageProposal(pack, this.toComplete)).forEach(this.requestor::accept); } catch (JavaModelException ex) { @@ -589,24 +603,111 @@ private void suggestPackages() { private void suggestTypesInPackage(String packageName) { if (!this.requestor.isIgnored(CompletionProposal.TYPE_REF)) { List foundTypes = findTypes(this.prefix, packageName).toList(); + ExtendsOrImplementsInfo extendsOrImplementsInfo = isInExtendsOrImplements(this.toComplete); for (IType foundType : foundTypes) { if (this.pattern.matchesName(this.prefix.toCharArray(), foundType.getElementName().toCharArray())) { - this.requestor.accept(this.toProposal(foundType)); + if (filterBasedOnExtendsOrImplementsInfo(foundType, extendsOrImplementsInfo)) { + this.requestor.accept(this.toProposal(foundType)); + } } } } } - private static boolean canAccessPrivate(ASTNode currentNode, ITypeBinding typeToCheck) { - ASTNode cursor = currentNode; - while (cursor != null) { - if (cursor instanceof AbstractTypeDeclaration typeDecl) { - if (typeDecl.resolveBinding().getKey().equals(typeToCheck.getErasure().getKey())) { - return true; + private boolean filterBasedOnExtendsOrImplementsInfo(IType toFilter, ExtendsOrImplementsInfo info) { + if (info == null) { + return true; + } + try { + if (!(info.typeDecl instanceof TypeDeclaration typeDeclaration) + || (toFilter.getFlags() & Flags.AccFinal) != 0 + || typeDeclaration.resolveBinding().getKey().equals(toFilter.getKey())) { + return false; + } + if (typeDeclaration.isInterface() + // in an interface extends clause, we should rule out non-interfaces + && toFilter.isInterface() + // prevent double extending + && !extendsOrImplementsGivenType(typeDeclaration, toFilter)) { + return true; + } else if (!typeDeclaration.isInterface() + // in an extends clause, only accept non-interfaces + // in an implements clause, only accept interfaces + && (info.isImplements == toFilter.isInterface()) + // prevent double extending + && !extendsOrImplementsGivenType(typeDeclaration, toFilter)) { + return true; + } + return false; + } catch (JavaModelException e) { + // we can't really tell if it's appropriate + return true; + } + } + + /** + * Returns info if the given node is in an extends or implements clause, or null if not in either clause + * + * @see ExtendsOrImplementsInfo + * @param completion the node to check + * @return info if the given node is in an extends or implements clause, or null if not in either clause + */ + private static ExtendsOrImplementsInfo isInExtendsOrImplements(ASTNode completion) { + ASTNode cursor = completion; + while (cursor != null + && cursor.getNodeType() != ASTNode.TYPE_DECLARATION + && cursor.getNodeType() != ASTNode.ENUM_DECLARATION + && cursor.getNodeType() != ASTNode.RECORD_DECLARATION + && cursor.getNodeType() != ASTNode.ANNOTATION_TYPE_DECLARATION) { + StructuralPropertyDescriptor locationInParent = cursor.getLocationInParent(); + if (locationInParent == null) { + return null; + } + if (locationInParent.isChildListProperty()) { + String locationId = locationInParent.getId(); + if (TypeDeclaration.SUPER_INTERFACE_TYPES_PROPERTY.getId().equals(locationId) + || EnumDeclaration.SUPER_INTERFACE_TYPES_PROPERTY.getId().equals(locationId) + || RecordDeclaration.SUPER_INTERFACE_TYPES_PROPERTY.getId().equals(locationId)) { + return new ExtendsOrImplementsInfo((AbstractTypeDeclaration)cursor.getParent(), true); + } + } else if (locationInParent.isChildProperty()) { + String locationId = locationInParent.getId(); + if (TypeDeclaration.SUPERCLASS_TYPE_PROPERTY.getId().equals(locationId)) { + return new ExtendsOrImplementsInfo((AbstractTypeDeclaration)cursor.getParent(), false); } } cursor = cursor.getParent(); } + return null; + } + + /** + * @param typeDecl the type declaration that holds the completion node + * @param isImplements true if the node to complete is in an implements clause, or false if the node + */ + private static record ExtendsOrImplementsInfo(AbstractTypeDeclaration typeDecl, boolean isImplements) { + } + + /** + * Returns true if the given declaration already extends or implements the given reference + * + * @param typeDecl the declaration to check the extends and implements of + * @param typeRef the reference to check for in the extends and implements + * @return true if the given declaration already extends or implements the given reference + */ + private static boolean extendsOrImplementsGivenType(TypeDeclaration typeDecl, IType typeRef) { + String refKey = typeRef.getKey(); + if (typeDecl.getSuperclassType() != null + && typeDecl.getSuperclassType().resolveBinding() != null + && refKey.equals(typeDecl.getSuperclassType().resolveBinding().getKey())) { + return true; + } + for (var superInterface : typeDecl.superInterfaceTypes()) { + ITypeBinding superInterfaceBinding = ((Type)superInterface).resolveBinding(); + if (superInterfaceBinding != null && refKey.equals(superInterfaceBinding.getKey())) { + return true; + } + } return false; }