Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adds support for workspaceSymbol/resolve #2008

Conversation

fvclaus
Copy link
Contributor

@fvclaus fvclaus commented Feb 15, 2022

Signed-off-by: Frederik Claus f.v.claus@googlemail.com

Signed-off-by: Frederik Claus <f.v.claus@googlemail.com>
Signed-off-by: Frederik Claus <f.v.claus@googlemail.com>
Signed-off-by: Frederik Claus <f.v.claus@googlemail.com>
Signed-off-by: Frederik Claus <f.v.claus@googlemail.com>
@fvclaus
Copy link
Contributor Author

fvclaus commented Mar 2, 2022

@testforstephen Shouldn't we add a test? I came up with the following:

	@Test
	public void testResolveWorkspaceSymbol() throws Exception {
		importProjects("maven/salut-java11");
		IProject project = WorkspaceHelper.getProject("salut-java11");
		String className = "StringUtils";
		String uri = buildClassfileUri(project, "org.apache.commons.commons-lang3", "org.apache.commons.lang3", className);
		SymbolInformation requestedSymbol = new SymbolInformation();
		requestedSymbol.setLocation(new Location(uri, START_OF_DOCUMENT));
		requestedSymbol.setName(className);
		requestedSymbol.setKind(SymbolKind.Class);
		SymbolInformation resolvedSymbol = ProjectCommand.resolve(requestedSymbol);
		assertEquals(resolvedSymbol.getLocation().getRange(), new Range(new Position(115, 13), new Position(115, 24)));
	}

	private String buildClassfileUri(IProject project, String mavenId, String packageName, String className) throws Exception {
		Map<String, JarPackageFragmentRoot> mavenIdToJarMap = buildMavenIdToJarMap(project);
		JarPackageFragmentRoot jar = mavenIdToJarMap.get(mavenId);
		if (jar == null) {
			throw new IllegalStateException("Did not find jar with maven id " + mavenId + " in project. Project only has the following maven ids: " + mavenIdToJarMap.keySet());
		}
		String classHandle = jar.getHandleIdentifier() + JavaElement.JEM_PACKAGEFRAGMENT + packageName + JavaElement.JEM_CLASSFILE + className + ".class";
		return new URI("jdt", "contents", null, classHandle, null).toASCIIString();
	}

	private Map<String, JarPackageFragmentRoot> buildMavenIdToJarMap(IProject project) throws Exception {
		JavaProject javaProject = (JavaProject) JavaCore.create("=" + project.getFullPath().toString());
		Map<String, JarPackageFragmentRoot> mavenIdToJar = new HashMap<>();
		for (IPackageFragmentRoot pkg : javaProject.getAllPackageFragmentRoots()) {
			if (!(pkg instanceof JarPackageFragmentRoot)) {
				continue;
			}
			JarPackageFragmentRoot jar = (JarPackageFragmentRoot) pkg;
			Optional<String> mavenId = buildMavenId(jar);
			if (mavenId.isPresent()) {
				mavenIdToJar.put(mavenId.get(), jar);
			}

		}
		return mavenIdToJar;
	}

	private Optional<String> buildMavenId(JarPackageFragmentRoot jar) throws JavaModelException {
		String groupId = null;
		String artifactId = null;
		for (IClasspathAttribute attribute : jar.getResolvedClasspathEntry().getExtraAttributes()) {
			if (attribute.getName().equals("maven.groupId")) {
				groupId = attribute.getValue();
			} else if (attribute.getName().equals("maven.artifactId")) {
				artifactId = attribute.getValue();
			}
		}
		if (groupId != null && artifactId != null) {
			return Optional.of(groupId + "." + artifactId);
		} else {
			return Optional.empty();
		}
	}
	

Maybe there is a better way to generate a classfile URI?

The idea of the test is to generate a classfile URI for an existing project and check whether the resolve algorithm works. I was thinking that we should have at least two test cases: One resolving a regular class (match on the root level) and one resolving a nested class.

@testforstephen
Copy link
Contributor

yes, it's better to have some unit test.

Maybe there is a better way to generate a classfile URI?

You can resolve the Java element first, and then generate its uri.

IProject project = WorkspaceHelper.getProject("salut-java11");
IJavaProject javaProject = ProjectUtils.getJavaProject(project);
IType type = javaProject.findType("org.apache.commons.lang3.StringUtils");
if (type.getClassFile() != null) {
  String uriString = JDTUtils.toUri(type.getClassFile());
}
...

Signed-off-by: Frederik Claus <f.v.claus@googlemail.com>
@fvclaus
Copy link
Contributor Author

fvclaus commented Mar 3, 2022

I have added two test cases, but, while debugging, discovered, that the nested and root type test case take the exact same execution path. This is because the symbol.location.uri references the type directly, whereas, when resolving a nested type in a source file (as opposed to a class file), the symbol.location.uri references the enclosing class and the symbol.name contains the name of the nested type. I originally implemented the bfs, because it implemented workspaceSymbols/resolve, that got called by the languageclient for every symbol (not just types in class files). Now, vscode-java is using a custom implementation of WorkspaceSymbolProvider.resolve that resolves only symbols for types in class files.

@testforstephen Should I remove the nested type test and the bfs, because we don't need it right now with the (upcoming) vscode integration or should I adapt the test to resolve a nested type in a souce file, because we will need it when the LSP version is out?

Resolving nested type in class file (removed range to improve readability):

{
  "name": "ProxyInvocationHandler",
  "kind": "Class",
  "location": {
    "uri": "jdt://contents/commons-lang3-3.11.jar/org.apache.commons.lang3.event/EventListenerSupport%24ProxyInvocationHandler.class?%3Didp-shared%2F%5C%2Fhome%5C%2Fdev%5C%2F.m2%5C%2Frepository%5C%2Forg%5C%2Fapache%5C%2Fcommons%5C%2Fcommons-lang3%5C%2F3.11%5C%2Fcommons-lang3-3.11.jar%3D%2Fmaven.pomderived%3D%2Ftrue%3D%2F%3D%2Fmaven.pomderived%3D%2Ftrue%3D%2F%3D%2Fmaven.groupId%3D%2Forg.apache.commons%3D%2F%3D%2Fmaven.artifactId%3D%2Fcommons-lang3%3D%2F%3D%2Fmaven.version%3D%2F3.11%3D%2F%3D%2Fmaven.scope%3D%2Fcompile%3D%2F%3Corg.apache.commons.lang3.event%28EventListenerSupport%24ProxyInvocationHandler.class",
  "containerName": "org.apache.commons.lang3.event.EventListenerSupport"
}

Resolving nested type in source file:

{
  "name": "NestedClass",
  "kind": "Class",
  "location": {
    "uri": "file:///home/dev/workspace/shibboleth-idpv3-vanilla/idp/src/main/java/de/akdb/idp/mock/registration/ExecuteMockRegistration.java",
  },
  "containerName": "de.akdb.idp.mock.registration.ExecuteMockRegistration"
}

@testforstephen
Copy link
Contributor

Should I remove the nested type test and the bfs, because we don't need it right now with the (upcoming) vscode integration or should I adapt the test to resolve a nested type in a souce file, because we will need it when the LSP version is out?

I'd keep current implementation and add a test for nested source type.

Signed-off-by: Frederik Claus <f.v.claus@googlemail.com>
@fvclaus
Copy link
Contributor Author

fvclaus commented Mar 7, 2022

Done

@testforstephen testforstephen added this to the End March 2022 milestone Mar 8, 2022
@testforstephen testforstephen merged commit d27d206 into eclipse-jdtls:master Mar 8, 2022
@testforstephen
Copy link
Contributor

@fvclaus Thank you for driving the implementation for this feature. It works well now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants