Skip to content

Commit

Permalink
Improve completion in extends/implements
Browse files Browse the repository at this point in the history 
- 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 <[email protected]>
  • Loading branch information
@datho7561
datho7561 committed Oct 25, 2024
1 parent cace049 commit eb98570
Show file tree
Hide file tree
Showing 3 changed files with 124 additions and 18 deletions.
6 changes: 4 additions & 2 deletions org.eclipse.jdt.core.javac/src/org/eclipse/jdt/core/dom/JavacConverter.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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())) {
Expand All @@ -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));
}
Expand Down
19 changes: 11 additions & 8 deletions org.eclipse.jdt.core.javac/src/org/eclipse/jdt/internal/javac/dom/JavacTypeBinding.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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();
Expand Down
117 changes: 109 additions & 8 deletions org.eclipse.jdt.core/codeassist/org/eclipse/jdt/internal/codeassist/DOMCompletionEngine.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand All @@ -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();
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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) {
Expand All @@ -589,24 +603,111 @@ private void suggestPackages() {
private void suggestTypesInPackage(String packageName) {
if (!this.requestor.isIgnored(CompletionProposal.TYPE_REF)) {
List<IType> 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;
}

Expand Down

0 comments on commit eb98570

Please sign in to comment.