From ba647fcbf1642a0d4c6e0fb9d8ef95cf82197083 Mon Sep 17 00:00:00 2001 From: David Thompson Date: Fri, 18 Oct 2024 10:32:10 -0400 Subject: [PATCH] Improve field completion (`name.|`) - Use bindings to build out a list of possible suggestions - Handle access modifiers properly (`private`, `static`, `abstract`, ...) - Exclude members in grandparent types whose access was narrowed in parent types - Fix completion preceeding another statement eg. ```java { name.| Object myObject = null; } ``` - Fix `super.to|` - Prevent `super.|` and `super.to|` from suggesting methods that are abstract in the parent type Fixes #891 Signed-off-by: David Thompson --- .../internal/javac/JavacProblemConverter.java | 2 + .../codeassist/DOMCompletionContext.java | 5 +- .../codeassist/DOMCompletionEngine.java | 262 ++++++++++++++---- 3 files changed, 216 insertions(+), 53 deletions(-) diff --git a/org.eclipse.jdt.core.javac/src/org/eclipse/jdt/internal/javac/JavacProblemConverter.java b/org.eclipse.jdt.core.javac/src/org/eclipse/jdt/internal/javac/JavacProblemConverter.java index c3e9dba88c7..f58850183df 100644 --- a/org.eclipse.jdt.core.javac/src/org/eclipse/jdt/internal/javac/JavacProblemConverter.java +++ b/org.eclipse.jdt.core.javac/src/org/eclipse/jdt/internal/javac/JavacProblemConverter.java @@ -767,6 +767,7 @@ yield switch (rootCauseCode) { case "compiler.err.cant.ref.before.ctor.called" -> IProblem.InstanceFieldDuringConstructorInvocation; // TODO different according to target node case "compiler.err.not.def.public.cant.access" -> IProblem.NotVisibleType; // TODO different according to target node case "compiler.err.already.defined" -> IProblem.DuplicateMethod; // TODO different according to target node + case "compiler.warn.underscore.as.identifier" -> IProblem.IllegalUseOfUnderscoreAsAnIdentifier; case "compiler.err.var.might.not.have.been.initialized" -> { VarSymbol symbol = getDiagnosticArgumentByType(diagnostic, VarSymbol.class); yield symbol.owner instanceof ClassSymbol ? @@ -1072,6 +1073,7 @@ yield switch (rootCauseCode) { case "compiler.err.too.many.modules" -> IProblem.ModuleRelated; case "compiler.err.call.must.only.appear.in.ctor" -> IProblem.InvalidExplicitConstructorCall; case "compiler.err.void.not.allowed.here" -> IProblem.ParameterMismatch; + case "compiler.err.abstract.cant.be.accessed.directly" -> IProblem.DirectInvocationOfAbstractMethod; default -> { ILog.get().error("Could not accurately convert diagnostic (" + diagnostic.getCode() + ")\n" + diagnostic); if (diagnostic.getKind() == javax.tools.Diagnostic.Kind.ERROR && diagnostic.getCode().startsWith("compiler.err")) { diff --git a/org.eclipse.jdt.core/codeassist/org/eclipse/jdt/internal/codeassist/DOMCompletionContext.java b/org.eclipse.jdt.core/codeassist/org/eclipse/jdt/internal/codeassist/DOMCompletionContext.java index 7107a916036..33d14736c1a 100644 --- a/org.eclipse.jdt.core/codeassist/org/eclipse/jdt/internal/codeassist/DOMCompletionContext.java +++ b/org.eclipse.jdt.core/codeassist/org/eclipse/jdt/internal/codeassist/DOMCompletionContext.java @@ -15,7 +15,6 @@ import java.util.function.Supplier; import java.util.stream.Stream; - import org.eclipse.jdt.core.CompletionContext; import org.eclipse.jdt.core.IJavaElement; import org.eclipse.jdt.core.Signature; @@ -57,6 +56,9 @@ public IJavaElement getEnclosingElement() { public IJavaElement[] getVisibleElements(String typeSignature) { return this.bindingsAcquirer.get() // .filter(binding -> { + if (typeSignature == null) { + return binding instanceof IVariableBinding || binding instanceof IMethodBinding; + } if (binding instanceof IVariableBinding variableBinding) { return castCompatable(variableBinding.getType(), typeSignature); @@ -69,6 +71,7 @@ public IJavaElement[] getVisibleElements(String typeSignature) { return false; }) // .map(binding -> binding.getJavaElement()) // + .filter(obj -> obj != null) // eg. ArrayList.getFirst() when working with a Java 8 project .toArray(IJavaElement[]::new); } 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 d9356dff28a..15b21645c98 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 @@ -10,13 +10,8 @@ *******************************************************************************/ package org.eclipse.jdt.internal.codeassist; -import java.util.ArrayList; -import java.util.Arrays; -import java.util.Collection; -import java.util.HashSet; -import java.util.List; -import java.util.Objects; -import java.util.Set; +import java.util.*; +import java.util.function.Predicate; import java.util.stream.Collectors; import java.util.stream.Stream; import org.eclipse.core.runtime.ILog; @@ -141,6 +136,19 @@ private Collection visibleBindings(ASTNode node) { .map(VariableDeclaration::resolveBinding).toList()); } + if (node instanceof AbstractTypeDeclaration typeDecl) { + visibleBindings.addAll(typeDecl.bodyDeclarations().stream() + .flatMap(bodyDecl -> { + if (bodyDecl instanceof FieldDeclaration fieldDecl) { + return ((List)fieldDecl.fragments()).stream().map(fragment -> fragment.resolveBinding()); + } + if (bodyDecl instanceof MethodDeclaration methodDecl) { + return Stream.of(methodDecl.resolveBinding()); + } + return Stream.of(); + }).toList()); + } + if (node instanceof Block block) { var bindings = ((List) block.statements()).stream() .filter(statement -> statement.getStartPosition() < this.offset) @@ -201,7 +209,8 @@ public void run() { completeAfter = simpleName.getIdentifier().substring(0, charCount); } if (simpleName.getParent() instanceof FieldAccess || simpleName.getParent() instanceof MethodInvocation - || simpleName.getParent() instanceof VariableDeclaration || simpleName.getParent() instanceof QualifiedName) { + || simpleName.getParent() instanceof VariableDeclaration || simpleName.getParent() instanceof QualifiedName + || simpleName.getParent() instanceof SuperFieldAccess) { context = this.toComplete.getParent(); } } else if (this.toComplete instanceof SimpleType simpleType) { @@ -215,12 +224,13 @@ public void run() { this.qualifiedPrefix = this.prefix; if (this.toComplete instanceof QualifiedName qualifiedName) { this.qualifiedPrefix = qualifiedName.getQualifier().toString(); - } else if (this.toComplete != null && this.toComplete.getParent () instanceof QualifiedName qualifiedName) { + } else if (this.toComplete != null && this.toComplete.getParent() instanceof QualifiedName qualifiedName) { this.qualifiedPrefix = qualifiedName.getQualifier().toString(); } - Bindings scope = new Bindings(); + Bindings defaultCompletionBindings = new Bindings(); + Bindings specificCompletionBindings = new Bindings(); var completionContext = new DOMCompletionContext(this.offset, completeAfter.toCharArray(), - computeEnclosingElement(), scope::stream); + computeEnclosingElement(), defaultCompletionBindings::stream); this.requestor.acceptContext(completionContext); // some flags to controls different applicable completion search strategies @@ -230,9 +240,9 @@ public void run() { if (context instanceof FieldAccess fieldAccess) { statementLikeKeywords(); - processMembers(fieldAccess.getExpression().resolveTypeBinding(), scope, true, isNodeInStaticContext(fieldAccess)); - if (scope.stream().findAny().isPresent()) { - scope.stream() + processMembers(fieldAccess, fieldAccess.getExpression().resolveTypeBinding(), specificCompletionBindings, false); + if (specificCompletionBindings.stream().findAny().isPresent()) { + specificCompletionBindings.stream() .filter(binding -> this.pattern.matchesName(this.prefix.toCharArray(), binding.getName().toCharArray())) .map(binding -> toProposal(binding)) .forEach(this.requestor::accept); @@ -259,8 +269,8 @@ public void run() { } // complete name ITypeBinding type = expression.resolveTypeBinding(); - processMembers(type, scope, true, isNodeInStaticContext(invocation)); - scope.stream() + processMembers(expression, type, specificCompletionBindings, false); + specificCompletionBindings.stream() .filter(binding -> this.pattern.matchesName(this.prefix.toCharArray(), binding.getName().toCharArray())) .filter(IMethodBinding.class::isInstance) .map(binding -> toProposal(binding)) @@ -271,7 +281,7 @@ public void run() { if (context instanceof VariableDeclaration declaration) { var binding = declaration.resolveBinding(); if (binding != null) { - this.variableDeclHandler.findVariableNames(binding, completeAfter, scope).stream() + this.variableDeclHandler.findVariableNames(binding, completeAfter, specificCompletionBindings).stream() .map(name -> toProposal(binding, name)).forEach(this.requestor::accept); } // seems we are completing a variable name, no need for further completion search. @@ -317,8 +327,8 @@ public void run() { if (context instanceof QualifiedName qualifiedName) { IBinding qualifiedNameBinding = qualifiedName.getQualifier().resolveBinding(); if (qualifiedNameBinding instanceof ITypeBinding qualifierTypeBinding && !qualifierTypeBinding.isRecovered()) { - processMembers(qualifierTypeBinding, scope, false, isNodeInStaticContext(qualifiedName)); - publishFromScope(scope); + processMembers(qualifiedName, qualifierTypeBinding, specificCompletionBindings, true); + publishFromScope(specificCompletionBindings); int startPos = this.offset; int endPos = this.offset; if ((qualifiedName.getName().getFlags() & ASTNode.MALFORMED) != 0) { @@ -330,18 +340,52 @@ public void run() { this.requestor.accept(createClassKeywordProposal(qualifierTypeBinding, startPos, endPos)); suggestDefaultCompletions = false; - } else if (qualifiedNameBinding instanceof IPackageBinding qualifierPackageBinding && !qualifierPackageBinding.isRecovered()) { - // start of a known package - suggestPackages(); - // suggests types in the package - suggestTypesInPackage(qualifierPackageBinding.getName()); + } else if (qualifiedNameBinding instanceof IPackageBinding qualifierPackageBinding) { + if (!qualifierPackageBinding.isRecovered()) { + // start of a known package + suggestPackages(); + // suggests types in the package + suggestTypesInPackage(qualifierPackageBinding.getName()); + suggestDefaultCompletions = false; + } else { + // likely the start of an incomplete field/method access + Bindings tempScope = new Bindings(); + scrapeAccessibleBindings(tempScope); + Optional potentialBinding = tempScope.stream() // + .filter(binding -> { + IJavaElement elt = binding.getJavaElement(); + if (elt == null) { + return false; + } + return elt.getElementName().equals(qualifiedName.getQualifier().toString()); + }) // + .map(binding -> { + if (binding instanceof IVariableBinding variableBinding) { + return variableBinding.getType(); + } else if (binding instanceof ITypeBinding typeBinding) { + return typeBinding; + } + throw new IllegalStateException("method, type var, etc. are likely not interpreted as a package"); //$NON-NLS-1$ + }) + .map(ITypeBinding.class::cast) + .findFirst(); + if (potentialBinding.isPresent()) { + processMembers(qualifiedName, potentialBinding.get(), specificCompletionBindings, false); + publishFromScope(specificCompletionBindings); + suggestDefaultCompletions = false; + } + } + } else if (qualifiedNameBinding instanceof IVariableBinding variableBinding) { + ITypeBinding typeBinding = variableBinding.getType(); + processMembers(qualifiedName, typeBinding, specificCompletionBindings, false); + publishFromScope(specificCompletionBindings); suggestDefaultCompletions = false; } } if (context instanceof SuperFieldAccess superFieldAccess) { ITypeBinding superTypeBinding = superFieldAccess.resolveTypeBinding(); - processMembers(superTypeBinding, scope, false, isNodeInStaticContext(superFieldAccess)); - publishFromScope(scope); + processMembers(superFieldAccess, superTypeBinding, specificCompletionBindings, false); + publishFromScope(specificCompletionBindings); suggestDefaultCompletions = false; } if (context instanceof MarkerAnnotation) { @@ -349,7 +393,7 @@ public void run() { suggestDefaultCompletions = false; } if (context instanceof NormalAnnotation normalAnnotation) { - completeNormalAnnotationParams(normalAnnotation, scope); + completeNormalAnnotationParams(normalAnnotation, specificCompletionBindings); suggestDefaultCompletions = false; } if (context instanceof MethodDeclaration methodDeclaration) { @@ -359,35 +403,26 @@ public void run() { if (methodDeclaration.getReturnType2() == null) { ASTNode current = this.toComplete; while (current != null) { - scope.addAll(visibleTypeBindings(current)); + specificCompletionBindings.addAll(visibleTypeBindings(current)); current = current.getParent(); } - publishFromScope(scope); + publishFromScope(specificCompletionBindings); } suggestDefaultCompletions = false; } else if (methodDeclaration.getBody() != null && this.offset <= methodDeclaration.getBody().getStartPosition()) { - completeThrowsClause(methodDeclaration, scope); + completeThrowsClause(methodDeclaration, specificCompletionBindings); suggestDefaultCompletions = false; } } + // check for accessible bindings to potentially turn into completions. + // currently, this is always run, even when not using the default completion, + // because method argument guessing uses it. + scrapeAccessibleBindings(defaultCompletionBindings); + if (suggestDefaultCompletions) { - ASTNode current = this.toComplete; - while (current != null) { - scope.addAll(visibleBindings(current)); - // break if following conditions match, otherwise we get all visible symbols which is unwanted in this - // completion context. - if (current instanceof NormalAnnotation normalAnnotation) { - completeNormalAnnotationParams(normalAnnotation, scope); - break; - } - if (current instanceof AbstractTypeDeclaration typeDecl) { - processMembers(typeDecl.resolveBinding(), scope, true, isNodeInStaticContext(this.toComplete)); - } - current = current.getParent(); - } statementLikeKeywords(); - publishFromScope(scope); + publishFromScope(defaultCompletionBindings); if (!completeAfter.isBlank()) { final int typeMatchRule = this.toComplete.getParent() instanceof Annotation ? IJavaSearchConstants.ANNOTATION_TYPE @@ -410,6 +445,24 @@ public void run() { } } + private void scrapeAccessibleBindings(Bindings scope) { + ASTNode current = this.toComplete; + while (current != null) { + Collection gottenVisibleBindings = visibleBindings(current); + scope.addAll(gottenVisibleBindings); + // break if following conditions match, otherwise we get all visible symbols which is unwanted in this + // completion context. + if (current instanceof NormalAnnotation normalAnnotation) { + completeNormalAnnotationParams(normalAnnotation, scope); + break; + } + if (current instanceof AbstractTypeDeclaration typeDecl) { + processMembers(this.toComplete, typeDecl.resolveBinding(), scope, false); + } + current = current.getParent(); + } + } + private void completeMethodModifiers(MethodDeclaration methodDeclaration) { List keywords = new ArrayList<>(); @@ -529,6 +582,19 @@ private void suggestTypesInPackage(String packageName) { } } + 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; + } + } + cursor = cursor.getParent(); + } + return false; + } + private static ASTNode findParent(ASTNode nodeToSearch, int[] kindsToFind) { ASTNode cursor = nodeToSearch; while (cursor != null) { @@ -665,23 +731,115 @@ public void acceptTypeNameMatch(org.eclipse.jdt.core.search.TypeNameMatch match) return types.stream(); } - private void processMembers(ITypeBinding typeBinding, Bindings scope, boolean includePrivate, boolean isStaticContext) { + private void processMembers(ASTNode referencedFrom, ITypeBinding typeBinding, Bindings scope, boolean isStaticContext) { + AbstractTypeDeclaration parentType = (AbstractTypeDeclaration)findParent(referencedFrom, new int[] {ASTNode.ANNOTATION_TYPE_DECLARATION, ASTNode.TYPE_DECLARATION, ASTNode.ENUM_DECLARATION, ASTNode.RECORD_DECLARATION}); + if (parentType == null) { + return; + } + ITypeBinding referencedFromBinding = parentType.resolveBinding(); + boolean includePrivate = referencedFromBinding.getKey().equals(typeBinding.getKey()); + MethodDeclaration methodDeclaration = (MethodDeclaration)findParent(referencedFrom, new int[] {ASTNode.METHOD_DECLARATION}); + // you can reference protected fields/methods from a static method, + // as long as those protected fields/methods are declared in the current class. + // otherwise, the (inherited) fields/methods can only be accessed in non-static methods. + boolean includeProtected; + if (referencedFromBinding.getKey().equals(typeBinding.getKey())) { + includeProtected = true; + } else if (methodDeclaration != null + && (methodDeclaration.getModifiers() & Flags.AccStatic) != 0) { + includeProtected = false; + } else { + includeProtected = findInSupers(referencedFromBinding, typeBinding); + } + processMembers(typeBinding, scope, includePrivate, includeProtected, referencedFromBinding.getPackage().getKey(), isStaticContext, false, + new HashSet<>(), new HashSet<>()); + } + + private void processMembers(ITypeBinding typeBinding, Bindings scope, + boolean includePrivate, + boolean includeProtected, + String originalPackageKey, + boolean isStaticContext, + boolean canUseAbstract, + Set impossibleMethods, + Set impossibleFields) { if (typeBinding == null) { return; } + Predicate accessFilter = binding -> { + boolean field = binding instanceof IVariableBinding; + if (field) { + if (impossibleFields.contains(binding.getName())) { + return false; + } + } else { + if (impossibleMethods.contains(binding.getName())) { + return false; + } + } + if ( + // check private + (!includePrivate && (binding.getModifiers() & Flags.AccPrivate) != 0) + // check protected + || (!includeProtected && (binding.getModifiers() & Flags.AccProtected) != 0) + // check package private + || ((binding.getModifiers() & (Flags.AccPublic | Flags.AccProtected | Flags.AccPrivate)) == 0 && !originalPackageKey.equals(typeBinding.getPackage().getKey())) + // check static + || (isStaticContext && (binding.getModifiers() & Flags.AccStatic) == 0) + // check abstract + || (!canUseAbstract && (binding.getModifiers() & Flags.AccAbstract) != 0) + ) { + if (field) { + impossibleFields.add(binding.getName()); + } else { + impossibleMethods.add(binding.getName()); + } + return false; + } + return true; + }; Arrays.stream(typeBinding.getDeclaredFields()) // - .filter(field -> (includePrivate || (field.getModifiers() & Flags.AccPrivate) == 0) - && (!isStaticContext || (field.getModifiers() & Flags.AccStatic) != 0)) // + .filter(accessFilter) // .forEach(scope::add); Arrays.stream(typeBinding.getDeclaredMethods()) // - .filter(method -> includePrivate || (method.getModifiers() & Flags.AccPrivate) == 0 - && (!isStaticContext || (method.getModifiers() & Flags.AccStatic) != 0)) // + .filter(accessFilter) // .forEach(scope::add); if (typeBinding.getInterfaces() != null) { - Arrays.stream(typeBinding.getInterfaces()).forEach(member -> processMembers(member, scope, false, isStaticContext)); + for (ITypeBinding superinterfaceBinding : typeBinding.getInterfaces()) { + processMembers(superinterfaceBinding, scope, false, includeProtected, originalPackageKey, isStaticContext, true, impossibleMethods, impossibleFields); + } + } + ITypeBinding superclassBinding = typeBinding.getSuperclass(); + if (superclassBinding != null) { + processMembers(superclassBinding, scope, false, includeProtected, originalPackageKey, isStaticContext, true, impossibleMethods, impossibleFields); + } + } + + private static boolean findInSupers(ITypeBinding root, ITypeBinding toFind) { + String keyToFind = toFind.getErasure().getKey(); + Queue toCheck = new LinkedList<>(); + Set alreadyChecked = new HashSet<>(); + toCheck.add(root.getErasure()); + while (!toCheck.isEmpty()) { + ITypeBinding current = toCheck.poll(); + String currentKey = current.getErasure().getKey(); + if (alreadyChecked.contains(currentKey)) { + continue; + } + alreadyChecked.add(currentKey); + if (currentKey.equals(keyToFind)) { + return true; + } + for (ITypeBinding superInterface : current.getInterfaces()) { + toCheck.add(superInterface); + } + if (current.getSuperclass() != null) { + toCheck.add(current.getSuperclass()); + } } - processMembers(typeBinding.getSuperclass(), scope, false, isStaticContext); + return false; } + private CompletionProposal toProposal(IBinding binding) { return toProposal(binding, binding.getName()); }