Skip to content

Commit

Permalink
Fix logic of LSWrapper.canOperate() when project is null (#949)
Browse files Browse the repository at this point in the history
  • Loading branch information
@joaodinissf
joaodinissf authored Apr 2, 2024
1 parent f4cd505 commit 83954d5
Show file tree
Hide file tree
Showing 7 changed files with 125 additions and 56 deletions.
15 changes: 10 additions & 5 deletions org.eclipse.lsp4e.test/src/org/eclipse/lsp4e/test/LanguageServersTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,14 @@
package org.eclipse.lsp4e.test;

import static org.eclipse.lsp4e.LanguageServiceAccessor.hasActiveLanguageServers;
import static org.eclipse.lsp4e.test.utils.TestUtils.*;
import static org.junit.Assert.*;
import static org.eclipse.lsp4e.test.utils.TestUtils.createUniqueTestFile;
import static org.eclipse.lsp4e.test.utils.TestUtils.openEditor;
import static org.eclipse.lsp4e.test.utils.TestUtils.waitForCondition;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertNotEquals;
import static org.junit.Assert.assertNull;
import static org.junit.Assert.assertTrue;

import java.util.Collections;
import java.util.List;
Expand Down Expand Up @@ -59,7 +65,6 @@
import org.eclipse.swt.widgets.Display;
import org.eclipse.ui.IEditorPart;
import org.eclipse.ui.tests.harness.util.DisplayHelper;
import org.eclipse.ui.texteditor.AbstractTextEditor;
import org.junit.Assume;
import org.junit.Before;
import org.junit.Rule;
Expand Down Expand Up @@ -743,8 +748,8 @@ public void testProjectExecutor() throws Exception {
assertTrue(serversForProject.contains("Server1"));
assertTrue(serversForProject.contains("Server2"));

((AbstractTextEditor) editor1).close(false);
((AbstractTextEditor) editor2).close(false);
editor1.getSite().getPage().closeEditor(editor1, false);
editor2.getSite().getPage().closeEditor(editor2, false);

waitForCondition(5_000, () -> !hasActiveLanguageServers(MATCH_ALL));

Expand Down
24 changes: 16 additions & 8 deletions org.eclipse.lsp4e.test/src/org/eclipse/lsp4e/test/LanguageServiceAccessorTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,15 @@
import static org.eclipse.lsp4e.LanguageServiceAccessor.getLSWrapper;
import static org.eclipse.lsp4e.LanguageServiceAccessor.getLSWrappers;
import static org.eclipse.lsp4e.LanguageServiceAccessor.hasActiveLanguageServers;
import static org.eclipse.lsp4e.test.utils.TestUtils.*;
import static org.eclipse.lsp4e.test.utils.TestUtils.createFile;
import static org.eclipse.lsp4e.test.utils.TestUtils.createProject;
import static org.eclipse.lsp4e.test.utils.TestUtils.createTempFile;
import static org.eclipse.lsp4e.test.utils.TestUtils.createUniqueTestFile;
import static org.eclipse.lsp4e.test.utils.TestUtils.createUniqueTestFileMultiLS;
import static org.eclipse.lsp4e.test.utils.TestUtils.openEditor;
import static org.eclipse.lsp4e.test.utils.TestUtils.openTextViewer;
import static org.eclipse.lsp4e.test.utils.TestUtils.waitForAndAssertCondition;
import static org.eclipse.lsp4e.test.utils.TestUtils.waitForCondition;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertNotNull;
Expand All @@ -43,15 +51,13 @@
import org.eclipse.lsp4e.LanguageServerPlugin;
import org.eclipse.lsp4e.LanguageServers;
import org.eclipse.lsp4e.LanguageServersRegistry;
import org.eclipse.lsp4e.LanguageServiceAccessor;
import org.eclipse.lsp4e.test.utils.AllCleanRule;
import org.eclipse.lsp4e.test.utils.MappingEnablementTester;
import org.eclipse.lsp4e.tests.mock.MockLanguageServer;
import org.eclipse.lsp4e.tests.mock.MockLanguageServerMultiRootFolders;
import org.eclipse.lsp4e.ui.UI;
import org.eclipse.lsp4j.ServerCapabilities;
import org.eclipse.ui.ide.IDE;
import org.eclipse.ui.texteditor.AbstractTextEditor;
import org.eclipse.ui.texteditor.ITextEditor;
import org.junit.Before;
import org.junit.Rule;
Expand Down Expand Up @@ -170,8 +176,8 @@ public void testGetOnlyRunningLanguageServers() throws Exception {

assertTrue(hasActiveLanguageServers(MATCH_ALL));

((AbstractTextEditor) editor1).close(false);
((AbstractTextEditor) editor2).close(false);
editor1.getSite().getPage().closeEditor(editor1, false);
editor2.getSite().getPage().closeEditor(editor2, false);

waitForCondition(5_000, () -> !hasActiveLanguageServers(MATCH_ALL));
assertFalse(hasActiveLanguageServers(MATCH_ALL));
Expand Down Expand Up @@ -385,11 +391,13 @@ public void testLSforExternalThenLocalFile() throws Exception {
return hoverProvider.isLeft() ? hoverProvider.getLeft() : hoverProvider.getRight() != null;
};

assertTrue(LanguageServiceAccessor.hasActiveLanguageServers(LSPEclipseUtils.getFile(getTextViewer(editor).getDocument()) , hasHoverCapabilities));
final IDocument externalDocument = getTextViewer(editor).getDocument();
assertTrue(hasActiveLanguageServers(externalDocument, hasHoverCapabilities));
wb.getActivePage().closeAllEditors(false);

// opening another file should either reuse the LS or spawn another one, but not both
assertTrue(LanguageServiceAccessor.hasActiveLanguageServers( //
LSPEclipseUtils.getFile(openTextViewer(createUniqueTestFile(project, "")).getDocument()), hasHoverCapabilities));
final IDocument internalDocument = openTextViewer(createUniqueTestFile(project, "")).getDocument();
assertTrue(hasActiveLanguageServers(internalDocument, hasHoverCapabilities));
}

@Test
Expand Down
3 changes: 3 additions & 0 deletions org.eclipse.lsp4e.test/src/org/eclipse/lsp4e/test/commands/DynamicRegistrationTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
*******************************************************************************/
package org.eclipse.lsp4e.test.commands;

import static org.eclipse.lsp4e.test.utils.TestUtils.waitForCondition;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertNotNull;
Expand Down Expand Up @@ -64,6 +65,8 @@ public void setUp() throws Exception {
IDocument document = LSPEclipseUtils.getDocument(testFile);
assertNotNull(document);
LanguageServers.forDocument(document).anyMatching();

waitForCondition(5_000, () -> !MockLanguageServer.INSTANCE.getRemoteProxies().isEmpty());
getMockClient();
}

Expand Down
56 changes: 29 additions & 27 deletions org.eclipse.lsp4e/src/org/eclipse/lsp4e/LanguageServerWrapper.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
import java.util.List;
import java.util.Map;
import java.util.Map.Entry;
import java.util.Objects;
import java.util.Timer;
import java.util.TimerTask;
import java.util.concurrent.CancellationException;
Expand Down Expand Up @@ -467,9 +468,10 @@ private URI getRootURI() {
}

private static boolean supportsWorkspaceFolders(ServerCapabilities serverCapabilities) {
return serverCapabilities != null && serverCapabilities.getWorkspace() != null
&& serverCapabilities.getWorkspace().getWorkspaceFolders() != null
&& Boolean.TRUE.equals(serverCapabilities.getWorkspace().getWorkspaceFolders().getSupported());
return serverCapabilities != null
&& serverCapabilities.getWorkspace() != null
&& serverCapabilities.getWorkspace().getWorkspaceFolders() != null
&& Boolean.TRUE.equals(serverCapabilities.getWorkspace().getWorkspaceFolders().getSupported());
}

private void logMessage(Message message) {
Expand Down Expand Up @@ -628,19 +630,37 @@ public IStatus runInWorkspace(IProgressMonitor monitor) throws CoreException {
}

/**
* Check whether this LS is suitable for provided project. Starts the LS if not
* already started.
* Check whether this LS is suitable for provided project.
*
* @return whether this language server can operate on the given project
* @since 0.5
*/
public boolean canOperate(@NonNull IProject project) {
return project.equals(this.initialProject) || serverDefinition.isSingleton || supportsWorkspaceFolderCapability();
public boolean canOperate(@Nullable IProject project) {
return Objects.equals(project, this.initialProject)
|| serverDefinition.isSingleton
|| supportsWorkspaceFolderCapability();
}

public boolean canOperate(@NonNull IDocument document) {
URI documentUri = LSPEclipseUtils.toUri(document);
if (documentUri == null) {
return false;
}
if (this.isConnectedTo(documentUri)) {
return true;
}
if (this.initialProject == null && this.connectedDocuments.isEmpty()) {
return true;
}
IFile file = LSPEclipseUtils.getFile(document);
if (file != null && file.exists() && canOperate(file.getProject())) {
return true;
}
return serverDefinition.isSingleton || supportsWorkspaceFolderCapability();
}

/**
* @return true, if the server supports multi-root workspaces via workspace
* folders
* @return true, if the server supports multi-root workspaces via workspace folders
* @since 0.6
*/
private boolean supportsWorkspaceFolderCapability() {
Expand Down Expand Up @@ -1083,24 +1103,6 @@ public int getTextDocumentVersion(URI uri) {
return -1;
}

public boolean canOperate(@NonNull IDocument document) {
URI documentUri = LSPEclipseUtils.toUri(document);
if (documentUri == null) {
return false;
}
if (this.isConnectedTo(documentUri)) {
return true;
}
if (this.initialProject == null && this.connectedDocuments.isEmpty()) {
return true;
}
IFile file = LSPEclipseUtils.getFile(document);
if (file != null && file.exists() && canOperate(file.getProject())) {
return true;
}
return serverDefinition.isSingleton || supportsWorkspaceFolderCapability();
}

@Override
public String toString() {
final var ph = getProcessHandle();
Expand Down
77 changes: 64 additions & 13 deletions org.eclipse.lsp4e/src/org/eclipse/lsp4e/LanguageServiceAccessor.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -359,7 +359,7 @@ public static LanguageServerWrapper getLSWrapper(@Nullable IProject project,
private static LanguageServerWrapper getLSWrapper(@Nullable IProject project,
@NonNull LanguageServerDefinition serverDefinition, @Nullable IPath initialPath) throws IOException {

final Predicate<LanguageServerWrapper> serverSelector = wrapper -> project != null && wrapper.canOperate(project)
final Predicate<LanguageServerWrapper> serverSelector = wrapper -> wrapper.canOperate(project)
&& wrapper.serverDefinition.equals(serverDefinition);

var matchingServer = startedServers.stream().filter(serverSelector).findFirst();
Expand Down Expand Up @@ -407,47 +407,98 @@ private static interface ServerSupplier {
LanguageServerWrapper get() throws IOException;
}

@NonNull
public static List<@NonNull LanguageServerWrapper> getStartedWrappers(Predicate<ServerCapabilities> request, boolean onlyActiveLS) {
return getStartedWrappers(w -> true, request, onlyActiveLS);
}

@NonNull
public static List<@NonNull LanguageServerWrapper> getStartedWrappers(@Nullable IProject project, Predicate<ServerCapabilities> request, boolean onlyActiveLS) {
return getStartedWrappers(w -> w.canOperate(project), request, onlyActiveLS);
}

@NonNull
public static List<@NonNull LanguageServerWrapper> getStartedWrappers(@NonNull IDocument document, Predicate<ServerCapabilities> request, boolean onlyActiveLS) {
return getStartedWrappers(w -> w.canOperate(document), request, onlyActiveLS);
}

@NonNull
private static List<@NonNull LanguageServerWrapper> getStartedWrappers(Predicate<LanguageServerWrapper> canOperatePredicate, Predicate<ServerCapabilities> capabilitiesPredicate, boolean onlyActiveLS) {
List<@NonNull LanguageServerWrapper> result = new ArrayList<>();
for (LanguageServerWrapper wrapper : startedServers) {
if ((!onlyActiveLS || wrapper.isActive()) && (project == null || wrapper.canOperate(project))) {
if (capabilitiesComply(wrapper, request)) {
if ((!onlyActiveLS || wrapper.isActive()) && canOperatePredicate.test(wrapper) && capabilitiesComply(wrapper, capabilitiesPredicate)) {
result.add(wrapper);
}
}
}
return result;
}

/**
* Returns {@code true} if there are running language servers satisfying a capability predicate. This does not
* start any matching language servers.
* Returns {@code true} if there are running language servers satisfying a capability predicate.
* This does not start any matching language servers.
*
* @param request
* @return {@code true} if there are running language servers satisfying a capability predicate
*/
public static boolean hasActiveLanguageServers(Predicate<ServerCapabilities> request) {
return !getLanguageServers(null, request, true).isEmpty();
public static boolean hasActiveLanguageServers(@NonNull Predicate<ServerCapabilities> request) {
return !getLanguageServers(request).isEmpty();
}

public static boolean hasActiveLanguageServers(@Nullable IFile file, @NonNull Predicate<ServerCapabilities> request) {
final IProject project = file != null ? file.getProject() : null;
return !getLanguageServers(project, request).isEmpty();
}

public static boolean hasActiveLanguageServers(IFile file, Predicate<ServerCapabilities> request) {
return !getLanguageServers(null, request, true).isEmpty();
public static boolean hasActiveLanguageServers(@NonNull IDocument document, @NonNull Predicate<ServerCapabilities> request) {
return !getLanguageServers(document, request).isEmpty();
}

/**
* Gets list of LS initialized for any project
*
* @param onlyActiveLS
* true if this method should return only the already running
* language servers, otherwise previously started language servers
* will be re-activated
* @return list of Language Servers
*/
@NonNull
private static List<@NonNull LanguageServer> getLanguageServers(Predicate<ServerCapabilities> capabilitiesPredicate) {
List<@NonNull LanguageServerWrapper> wrappers = getStartedWrappers(capabilitiesPredicate, true);
return getLanguageServersFromWrappers(wrappers);
}

/**
* Gets list of LS initialized for given project
*
* @param onlyActiveLS
* true if this method should return only the already running
* language servers, otherwise previously started language servers
* will be re-activated
* @return list of Language Servers
*/
@NonNull
private static List<@NonNull LanguageServer> getLanguageServers(@Nullable IProject project, Predicate<ServerCapabilities> capabilitiesPredicate) {
List<@NonNull LanguageServerWrapper> wrappers = getStartedWrappers(project, capabilitiesPredicate, true);
return getLanguageServersFromWrappers(wrappers);
}

/**
* Gets list of LS initialized for given document
*
* @param onlyActiveLS
* true if this method should return only the already running
* language servers, otherwise previously started language servers
* will be re-activated
* @return list of Language Servers
*/
@NonNull
private static List<@NonNull LanguageServer> getLanguageServers(@Nullable IProject project,
Predicate<ServerCapabilities> request, boolean onlyActiveLS) {
List<@NonNull LanguageServerWrapper> wrappers = getStartedWrappers(project, request, onlyActiveLS);
private static List<@NonNull LanguageServer> getLanguageServers(@NonNull IDocument document, Predicate<ServerCapabilities> capabilitiesPredicate) {
List<@NonNull LanguageServerWrapper> wrappers = getStartedWrappers(document, capabilitiesPredicate, true);
return getLanguageServersFromWrappers(wrappers);
}

private static List<@NonNull LanguageServer> getLanguageServersFromWrappers(List<@NonNull LanguageServerWrapper> wrappers) {
final var servers = new ArrayList<@NonNull LanguageServer>(wrappers.size());
for (LanguageServerWrapper wrapper : wrappers) {
@Nullable
Expand Down
4 changes: 2 additions & 2 deletions ...e.lsp4e/src/org/eclipse/lsp4e/operations/symbols/WorkspaceSymbolsQuickAccessProvider.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -47,12 +47,12 @@ public void resetState() {
@Override
public boolean needsRefresh() {
return this.usedLanguageServerWrappers == null
|| !this.usedLanguageServerWrappers.equals(LanguageServiceAccessor.getStartedWrappers(null, capabilities -> LSPEclipseUtils.hasCapability(capabilities.getWorkspaceSymbolProvider()), true));
|| !this.usedLanguageServerWrappers.equals(LanguageServiceAccessor.getStartedWrappers(capabilities -> LSPEclipseUtils.hasCapability(capabilities.getWorkspaceSymbolProvider()), true));
}

@Override
public QuickAccessElement[] computeElements(String query, IProgressMonitor monitor) {
this.usedLanguageServerWrappers = LanguageServiceAccessor.getStartedWrappers(null, capabilities -> LSPEclipseUtils.hasCapability(capabilities.getWorkspaceSymbolProvider()), true);
this.usedLanguageServerWrappers = LanguageServiceAccessor.getStartedWrappers(capabilities -> LSPEclipseUtils.hasCapability(capabilities.getWorkspaceSymbolProvider()), true);
if (usedLanguageServerWrappers.isEmpty()) {
return new QuickAccessElement[0];
}
Expand Down
2 changes: 1 addition & 1 deletion org.eclipse.lsp4e/src/org/eclipse/lsp4e/ui/LanguageServersView.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -244,7 +244,7 @@ public void setFocus() {

private void updateViewerInput() {
final var currentElements = (Object[]) viewer.getInput();
final var newElements = LanguageServiceAccessor.getStartedWrappers(null, capability -> true, true).toArray();
final var newElements = LanguageServiceAccessor.getStartedWrappers(capability -> true, true).toArray();
if (!Arrays.equals(currentElements, newElements)) {
UI.getDisplay().execute(() -> {
actionButtons.values().forEach(Widget::dispose);
Expand Down

0 comments on commit 83954d5

Please sign in to comment.