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.

Sign up for GitHub

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

fix the NPE in DocumentSymbolHandler when source/location is not avai… #851

Closed
wants to merge 2 commits into from
Closed

fix the NPE in DocumentSymbolHandler when source/location is not avai… #851

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4196cf4
fix the NPE in DocumentSymbolHandler when source/location is not avai…
andxu Nov 15, 2018
5f554cc
use name as the region of the symbol instead of using full region(whi…
andxu Nov 16, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions ...ipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/handlers/DocumentSymbolHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,13 +47,15 @@
import org.eclipse.lsp4j.DocumentSymbol;
import org.eclipse.lsp4j.DocumentSymbolParams;
import org.eclipse.lsp4j.Location;
import org.eclipse.lsp4j.Position;
import org.eclipse.lsp4j.Range;
import org.eclipse.lsp4j.SymbolInformation;
import org.eclipse.lsp4j.SymbolKind;
import org.eclipse.lsp4j.jsonrpc.messages.Either;
import org.eclipse.xtext.xbase.lib.Exceptions;

public class DocumentSymbolHandler {
private static Range DEFAULT_RANGE = new Range(new Position(0, 0), new Position(0, 0));

private boolean hierarchicalDocumentSymbolSupported;

Expand Down Expand Up @@ -171,11 +173,13 @@ private String getName(IJavaElement element) {
}

private Range getRange(IJavaElement element) throws JavaModelException {
return JDTUtils.toLocation(element, FULL_RANGE).getRange();
Location location = JDTUtils.toLocation(element, FULL_RANGE);
return location == null ? DEFAULT_RANGE : location.getRange();
}

private Range getSelectionRange(IJavaElement element) throws JavaModelException {
return JDTUtils.toLocation(element).getRange();
Location location = JDTUtils.toLocation(element, FULL_RANGE);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Location location = JDTUtils.toLocation(element);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed.

return location == null ? DEFAULT_RANGE : location.getRange();
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getRange and getSelectionRange are now strictly identical O_o

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we remove the call of "toLocation" twice to share the location? I don't know why the original purpose of SelectionRange and getRange

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-1 on removing that. It was implemented according to the spec:

/**
* The range enclosing this symbol not including leading/trailing whitespace but everything else
* like comments. This information is typically used to determine if the clients cursor is
* inside the symbol to reveal in the symbol in the UI.
*/
range: Range;

/**
* The range that should be selected and revealed when this symbol is being picked, e.g the name of a function.
* Must be contained by the range.
*/
selectionRange: Range;

You might break other clients by changing those ranges


private boolean isDeprecated(IJavaElement element) throws JavaModelException {
Expand Down
45 changes: 31 additions & 14 deletions ...jdt.ls.tests/src/org/eclipse/jdt/ls/core/internal/handlers/DocumentSymbolHandlerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
import static org.junit.Assert.assertTrue;

import java.io.UnsupportedEncodingException;
import java.util.Arrays;
import java.util.Collection;
import java.util.Collections;
import java.util.List;
Expand All @@ -25,6 +26,7 @@
import java.util.stream.Stream;

import org.eclipse.core.resources.IProject;
import org.eclipse.core.runtime.IProgressMonitor;
import org.eclipse.jdt.core.JavaModelException;
import org.eclipse.jdt.ls.core.internal.ClassFileUtil;
import org.eclipse.jdt.ls.core.internal.WorkspaceHelper;
Expand All @@ -49,11 +51,13 @@
public class DocumentSymbolHandlerTest extends AbstractProjectsManagerBasedTest {

private IProject project;
private IProject noSourceProject;

@Before
public void setup() throws Exception {
importProjects("maven/salut");
importProjects(Arrays.asList("maven/salut", "eclipse/source-attachment"));
project = WorkspaceHelper.getProject("salut");
noSourceProject = WorkspaceHelper.getProject("source-attachment");
}

@Test
Expand Down Expand Up @@ -144,6 +148,29 @@ public void testTypes_hierarchical() throws Exception {
assertHasHierarchicalSymbol("bar() : void", "MyClass", SymbolKind.Method, symbols);
}

@Test
public void testSyntheticMember_hierarchical_noSourceAttached() throws Exception {
String className = "foo.bar";
List<? extends DocumentSymbol> symbols = asStream(internalGetHierarchicalSymbols(noSourceProject, monitor, className)).collect(Collectors.toList());
assertHasHierarchicalSymbol("bar()", "bar", SymbolKind.Method, symbols);
assertHasHierarchicalSymbol("add(int...) : int", "bar", SymbolKind.Method, symbols);
}

private static List<? extends DocumentSymbol> internalGetHierarchicalSymbols(IProject project, IProgressMonitor monitor, String className)
throws JavaModelException, UnsupportedEncodingException, InterruptedException, ExecutionException {
String uri = ClassFileUtil.getURI(project, className);
TextDocumentIdentifier identifier = new TextDocumentIdentifier(uri);
DocumentSymbolParams params = new DocumentSymbolParams();
params.setTextDocument(identifier);
//@formatter:off
List<DocumentSymbol> symbols = new DocumentSymbolHandler(true)
.documentSymbol(params, monitor).stream()
.map(Either::getRight).collect(toList());
//@formatter:on
assertTrue(symbols.size() > 0);
return symbols;
}

private void assertHasSymbol(String expectedType, String expectedParent, SymbolKind expectedKind, Collection<? extends SymbolInformation> symbols) {
Optional<? extends SymbolInformation> symbol = symbols.stream()
.filter(s -> expectedType.equals(s.getName()) && expectedParent.equals(s.getContainerName()))
Expand Down Expand Up @@ -216,21 +243,11 @@ private List<? extends SymbolInformation> getSymbols(String className)
return symbols;
}

private List<? extends DocumentSymbol> getHierarchicalSymbols(String className)
throws JavaModelException, UnsupportedEncodingException, InterruptedException, ExecutionException {
String uri = ClassFileUtil.getURI(project, className);
TextDocumentIdentifier identifier = new TextDocumentIdentifier(uri);
DocumentSymbolParams params = new DocumentSymbolParams();
params.setTextDocument(identifier);
//@formatter:off
List<DocumentSymbol> symbols = new DocumentSymbolHandler(true)
.documentSymbol(params, monitor).stream()
.map(Either::getRight).collect(toList());
//@formatter:on
assertTrue(symbols.size() > 0);
return symbols;
private List<? extends DocumentSymbol> getHierarchicalSymbols(String className) throws JavaModelException, UnsupportedEncodingException, InterruptedException, ExecutionException {
return internalGetHierarchicalSymbols(project, monitor, className);
}


private boolean isValid(Range range) {
return range != null && isValid(range.getStart()) && isValid(range.getEnd());
}
Expand Down