From 888d1da9a7dee7e33cd69481ea52af16a771af46 Mon Sep 17 00:00:00 2001 From: Miles Ziemer Date: Sun, 22 Dec 2024 20:22:06 -0500 Subject: [PATCH 1/9] Hide ServerState data members --- .../smithy/lsp/FileWatcherRegistrations.java | 1 + .../amazon/smithy/lsp/ServerState.java | 103 +++-- .../smithy/lsp/SmithyLanguageServer.java | 60 +-- .../amazon/smithy/lsp/WorkspaceChanges.java | 9 +- .../amazon/smithy/lsp/project/Project.java | 14 + .../smithy/lsp/project/ProjectLoader.java | 4 +- .../amazon/smithy/lsp/ServerStateTest.java | 42 -- .../smithy/lsp/SmithyLanguageServerTest.java | 389 ++++++++---------- .../lsp/SmithyVersionRefactoringTest.java | 4 +- 9 files changed, 287 insertions(+), 339 deletions(-) delete mode 100644 src/test/java/software/amazon/smithy/lsp/ServerStateTest.java diff --git a/src/main/java/software/amazon/smithy/lsp/FileWatcherRegistrations.java b/src/main/java/software/amazon/smithy/lsp/FileWatcherRegistrations.java index d45ae276..a52acdfd 100644 --- a/src/main/java/software/amazon/smithy/lsp/FileWatcherRegistrations.java +++ b/src/main/java/software/amazon/smithy/lsp/FileWatcherRegistrations.java @@ -56,6 +56,7 @@ private FileWatcherRegistrations() { */ static List getSmithyFileWatcherRegistrations(Collection projects) { List smithyFileWatchers = projects.stream() + .filter(project -> project.type() == Project.Type.NORMAL) .flatMap(project -> FilePatterns.getSmithyFileWatchPatterns(project).stream()) .map(pattern -> new FileSystemWatcher(Either.forLeft(pattern), WATCH_FILE_KIND)) .toList(); diff --git a/src/main/java/software/amazon/smithy/lsp/ServerState.java b/src/main/java/software/amazon/smithy/lsp/ServerState.java index 9481d38d..04a41033 100644 --- a/src/main/java/software/amazon/smithy/lsp/ServerState.java +++ b/src/main/java/software/amazon/smithy/lsp/ServerState.java @@ -10,6 +10,7 @@ import java.nio.file.Path; import java.nio.file.Paths; import java.util.ArrayList; +import java.util.Collection; import java.util.HashMap; import java.util.HashSet; import java.util.List; @@ -27,34 +28,55 @@ /** * Keeps track of the state of the server. - * - * @param detachedProjects Map of smithy file **uri** to detached project - * for that file - * @param attachedProjects Map of directory **path** to attached project roots - * @param workspacePaths Paths to roots of each workspace open in the client - * @param managedUris Uris of each file managed by the server/client, i.e. - * files which may be updated by didChange - * @param lifecycleManager Container for ongoing tasks */ -public record ServerState( - Map detachedProjects, - Map attachedProjects, - Set workspacePaths, - Set managedUris, - DocumentLifecycleManager lifecycleManager -) { +public final class ServerState { private static final Logger LOGGER = Logger.getLogger(ServerState.class.getName()); + private final Map detachedProjects; + private final Map attachedProjects; + private final Set workspacePaths; + private final Set managedUris; + private final DocumentLifecycleManager lifecycleManager; + /** * Create a new, empty server state. */ public ServerState() { - this( - new HashMap<>(), - new HashMap<>(), - new HashSet<>(), - new HashSet<>(), - new DocumentLifecycleManager()); + this.detachedProjects = new HashMap<>(); + this.attachedProjects = new HashMap<>(); + this.workspacePaths = new HashSet<>(); + this.managedUris = new HashSet<>(); + this.lifecycleManager = new DocumentLifecycleManager(); + } + + public Collection getAllProjects() { + List allProjects = new ArrayList<>(attachedProjects.values()); + allProjects.addAll(detachedProjects.values()); + return allProjects; + } + + public Collection getAllManaged() { + List allManaged = new ArrayList<>(managedUris.size()); + for (String uri : managedUris) { + allManaged.add(findManaged(uri)); + } + return allManaged; + } + + public Set workspacePaths() { + return workspacePaths; + } + + public DocumentLifecycleManager lifecycleManager() { + return lifecycleManager; + } + + public Project findProjectByRoot(String root) { + Project attached = attachedProjects.get(root); + if (attached == null) { + return detachedProjects.get(root); + } + return attached; } /** @@ -105,15 +127,36 @@ ProjectAndFile findProjectAndFile(String uri) { return null; } - boolean isDetached(String uri) { - if (detachedProjects.containsKey(uri)) { - ProjectAndFile attached = findAttachedAndRemoveDetached(uri); - // The file is only truly detached if the above didn't find an attached project - // for the given file - return attached == null; + ProjectAndFile findManaged(String uri) { + if (managedUris.contains(uri)) { + return findProjectAndFile(uri); + } + return null; + } + + ProjectAndFile open(String uri, String text) { + managedUris.add(uri); + + ProjectAndFile projectAndFile = findProjectAndFile(uri); + if (projectAndFile != null) { + projectAndFile.file().document().applyEdit(null, text); + } else { + createDetachedProject(uri, text); + projectAndFile = findProjectAndFile(uri); // Note: This will always be present } - return false; + return projectAndFile; + } + + void close(String uri) { + managedUris.remove(uri); + + ProjectAndFile projectAndFile = findProjectAndFile(uri); + if (projectAndFile != null && projectAndFile.project().type() == Project.Type.DETACHED) { + // Only cancel tasks for detached projects, since we're dropping the project + lifecycleManager.cancelTask(uri); + detachedProjects.remove(uri); + } } /** @@ -226,9 +269,7 @@ private void resolveDetachedProjects(Project oldProject, Project updatedProject) addedPaths.removeAll(currentProjectSmithyPaths); for (String addedPath : addedPaths) { String addedUri = LspAdapter.toUri(addedPath); - if (isDetached(addedUri)) { - detachedProjects.remove(addedUri); - } + detachedProjects.remove(addedUri); } Set removedPaths = new HashSet<>(currentProjectSmithyPaths); diff --git a/src/main/java/software/amazon/smithy/lsp/SmithyLanguageServer.java b/src/main/java/software/amazon/smithy/lsp/SmithyLanguageServer.java index 5257ebab..d2565c5d 100644 --- a/src/main/java/software/amazon/smithy/lsp/SmithyLanguageServer.java +++ b/src/main/java/software/amazon/smithy/lsp/SmithyLanguageServer.java @@ -20,10 +20,8 @@ import java.io.IOException; import java.nio.file.Path; import java.util.ArrayList; -import java.util.Collection; import java.util.Collections; import java.util.List; -import java.util.Map; import java.util.Objects; import java.util.Optional; import java.util.Properties; @@ -31,7 +29,6 @@ import java.util.concurrent.CompletableFuture; import java.util.logging.Logger; import java.util.stream.Collectors; -import java.util.stream.Stream; import org.eclipse.lsp4j.ClientCapabilities; import org.eclipse.lsp4j.CodeAction; import org.eclipse.lsp4j.CodeActionOptions; @@ -149,10 +146,6 @@ public class SmithyLanguageServer implements SmithyLanguageServer() { } - Project getFirstProject() { - return state.attachedProjects().values().stream().findFirst().orElse(null); - } - ServerState getState() { return state; } @@ -235,7 +228,7 @@ private void tryInitProject(Path root) { private CompletableFuture registerSmithyFileWatchers() { List registrations = FileWatcherRegistrations.getSmithyFileWatcherRegistrations( - state.attachedProjects().values()); + state.getAllProjects()); return client.registerCapability(new RegistrationParams(registrations)); } @@ -375,11 +368,7 @@ public CompletableFuture> selectorCommand(SelectorParam return completedFuture(Collections.emptyList()); } - // Select from all available projects - Collection detached = state.detachedProjects().values(); - Collection nonDetached = state.attachedProjects().values(); - - return completedFuture(Stream.concat(detached.stream(), nonDetached.stream()) + return completedFuture(state.getAllProjects().stream() .flatMap(project -> project.modelResult().getResult().stream()) .map(selector::select) .flatMap(shapes -> shapes.stream() @@ -392,22 +381,14 @@ public CompletableFuture> selectorCommand(SelectorParam @Override public CompletableFuture serverStatus() { List openProjects = new ArrayList<>(); - for (Project project : state.attachedProjects().values()) { + for (Project project : state.getAllProjects()) { openProjects.add(new OpenProject( LspAdapter.toUri(project.root().toString()), project.smithyFiles().keySet().stream() .map(LspAdapter::toUri) .toList(), - false)); - } - - for (Map.Entry entry : state.detachedProjects().entrySet()) { - openProjects.add(new OpenProject( - entry.getKey(), - Collections.singletonList(entry.getKey()), - true)); + project.type() == Project.Type.DETACHED)); } - return completedFuture(new ServerStatus(openProjects)); } @@ -420,7 +401,7 @@ public void didChangeWatchedFiles(DidChangeWatchedFilesParams params) { WorkspaceChanges changes = WorkspaceChanges.computeWorkspaceChanges(params.getChanges(), state); changes.byProject().forEach((projectName, projectChange) -> { - Project project = state.attachedProjects().get(projectName); + Project project = state.findProjectByRoot(projectName); if (!projectChange.changedBuildFileUris().isEmpty()) { client.info("Build files changed, reloading project"); @@ -482,7 +463,7 @@ public void didChange(DidChangeTextDocumentParams params) { state.lifecycleManager().cancelTask(uri); - ProjectAndFile projectAndFile = state.findProjectAndFile(uri); + ProjectAndFile projectAndFile = state.findManaged(uri); if (projectAndFile == null) { client.unknownFileError(uri, "change"); return; @@ -523,16 +504,8 @@ public void didOpen(DidOpenTextDocumentParams params) { String uri = params.getTextDocument().getUri(); state.lifecycleManager().cancelTask(uri); - state.managedUris().add(uri); - String text = params.getTextDocument().getText(); - ProjectAndFile projectAndFile = state.findProjectAndFile(uri); - if (projectAndFile != null) { - projectAndFile.file().document().applyEdit(null, text); - } else { - state.createDetachedProject(uri, text); - projectAndFile = state.findProjectAndFile(uri); // Note: This will always be present - } + ProjectAndFile projectAndFile = state.open(uri, params.getTextDocument().getText()); state.lifecycleManager().putTask(uri, sendFileDiagnostics(projectAndFile)); } @@ -542,15 +515,7 @@ public void didClose(DidCloseTextDocumentParams params) { LOGGER.finest("DidClose"); String uri = params.getTextDocument().getUri(); - state.managedUris().remove(uri); - - if (state.isDetached(uri)) { - // Only cancel tasks for detachedProjects projects, since we're dropping the project - state.lifecycleManager().cancelTask(uri); - state.detachedProjects().remove(uri); - } - - // TODO: Clear diagnostics? Can do this by sending an empty list + state.close(uri); } @Override @@ -560,10 +525,8 @@ public void didSave(DidSaveTextDocumentParams params) { String uri = params.getTextDocument().getUri(); state.lifecycleManager().cancelTask(uri); - ProjectAndFile projectAndFile = state.findProjectAndFile(uri); + ProjectAndFile projectAndFile = state.findManaged(uri); if (projectAndFile == null) { - // TODO: Could also load a detachedProjects project here, but I don't know how this would - // actually happen in practice client.unknownFileError(uri, "save"); return; } @@ -711,9 +674,8 @@ public CompletableFuture> formatting(DocumentFormatting } private void sendFileDiagnosticsForManagedDocuments() { - for (String managedDocumentUri : state.managedUris()) { - ProjectAndFile projectAndFile = state.findProjectAndFile(managedDocumentUri); - state.lifecycleManager().putOrComposeTask(managedDocumentUri, sendFileDiagnostics(projectAndFile)); + for (ProjectAndFile managed : state.getAllManaged()) { + state.lifecycleManager().putOrComposeTask(managed.uri(), sendFileDiagnostics(managed)); } } diff --git a/src/main/java/software/amazon/smithy/lsp/WorkspaceChanges.java b/src/main/java/software/amazon/smithy/lsp/WorkspaceChanges.java index 5f1601dc..8bc72487 100644 --- a/src/main/java/software/amazon/smithy/lsp/WorkspaceChanges.java +++ b/src/main/java/software/amazon/smithy/lsp/WorkspaceChanges.java @@ -32,9 +32,12 @@ private WorkspaceChanges() { static WorkspaceChanges computeWorkspaceChanges(List events, ServerState state) { WorkspaceChanges changes = new WorkspaceChanges(); - List projectFileMatchers = new ArrayList<>(state.attachedProjects().size()); - state.attachedProjects().forEach((projectName, project) -> - projectFileMatchers.add(createProjectFileMatcher(projectName, project))); + List projectFileMatchers = new ArrayList<>(); + state.getAllProjects().forEach(project -> { + if (project.type() == Project.Type.NORMAL) { + projectFileMatchers.add(createProjectFileMatcher(project.root().toString(), project)); + } + }); List workspaceBuildFileMatchers = new ArrayList<>(state.workspacePaths().size()); state.workspacePaths().forEach(workspacePath -> diff --git a/src/main/java/software/amazon/smithy/lsp/project/Project.java b/src/main/java/software/amazon/smithy/lsp/project/Project.java index baae3773..acffce34 100644 --- a/src/main/java/software/amazon/smithy/lsp/project/Project.java +++ b/src/main/java/software/amazon/smithy/lsp/project/Project.java @@ -40,6 +40,7 @@ public final class Project { private final Map smithyFiles; private final Supplier assemblerFactory; private final Map> definedShapesByFile; + private final Type type; private ValidatedResult modelResult; // TODO: Move this into SmithyFileDependenciesIndex private Map> perFileMetadata; @@ -51,6 +52,7 @@ public final class Project { Map smithyFiles, Supplier assemblerFactory, Map> definedShapesByFile, + Type type, ValidatedResult modelResult, Map> perFileMetadata, SmithyFileDependenciesIndex smithyFileDependenciesIndex) { @@ -60,11 +62,18 @@ public final class Project { this.smithyFiles = smithyFiles; this.assemblerFactory = assemblerFactory; this.definedShapesByFile = definedShapesByFile; + this.type = type; this.modelResult = modelResult; this.perFileMetadata = perFileMetadata; this.smithyFileDependenciesIndex = smithyFileDependenciesIndex; } + public enum Type { + NORMAL, + DETACHED, + EMPTY; + } + /** * Create an empty project with no Smithy files, dependencies, or loaded model. * @@ -78,6 +87,7 @@ public static Project empty(Path root) { new HashMap<>(), Model::assembler, new HashMap<>(), + Type.EMPTY, ValidatedResult.empty(), new HashMap<>(), new SmithyFileDependenciesIndex()); @@ -140,6 +150,10 @@ public Map> definedShapesByFile() { return this.definedShapesByFile; } + public Type type() { + return type; + } + /** * @return The latest result of loading this project */ diff --git a/src/main/java/software/amazon/smithy/lsp/project/ProjectLoader.java b/src/main/java/software/amazon/smithy/lsp/project/ProjectLoader.java index 538b5cca..6c39cead 100644 --- a/src/main/java/software/amazon/smithy/lsp/project/ProjectLoader.java +++ b/src/main/java/software/amazon/smithy/lsp/project/ProjectLoader.java @@ -94,12 +94,13 @@ public static Project loadDetached(String uri, String text) { } }); - return new Project(path.getParent(), + return new Project(path, ProjectConfig.builder().sources(List.of(asPath)).build(), List.of(), smithyFiles, assemblerFactory, definedShapesByFile, + Project.Type.DETACHED, modelResult, computePerFileMetadata(modelResult), new SmithyFileDependenciesIndex()); @@ -186,6 +187,7 @@ public static Result> load(Path root, ServerState state smithyFiles, assemblerFactory, definedShapesByFile, + Project.Type.NORMAL, modelResult, computePerFileMetadata(modelResult), SmithyFileDependenciesIndex.compute(modelResult))); diff --git a/src/test/java/software/amazon/smithy/lsp/ServerStateTest.java b/src/test/java/software/amazon/smithy/lsp/ServerStateTest.java deleted file mode 100644 index c079935d..00000000 --- a/src/test/java/software/amazon/smithy/lsp/ServerStateTest.java +++ /dev/null @@ -1,42 +0,0 @@ -/* - * Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. - * SPDX-License-Identifier: Apache-2.0 - */ - -package software.amazon.smithy.lsp; - -import static org.hamcrest.CoreMatchers.notNullValue; -import static org.hamcrest.CoreMatchers.nullValue; -import static org.hamcrest.MatcherAssert.assertThat; - -import java.nio.file.Path; -import org.junit.jupiter.api.Test; -import software.amazon.smithy.lsp.project.Project; -import software.amazon.smithy.lsp.project.ProjectLoader; -import software.amazon.smithy.lsp.project.ProjectTest; -import software.amazon.smithy.lsp.protocol.LspAdapter; - -public class ServerStateTest { - @Test - public void canCheckIfAFileIsTracked() { - Path attachedRoot = ProjectTest.toPath(getClass().getResource("project/flat")); - ServerState manager = new ServerState(); - Project mainProject = ProjectLoader.load(attachedRoot, manager).unwrap(); - - manager.attachedProjects().put("main", mainProject); - - String detachedUri = LspAdapter.toUri("/foo/bar"); - manager.createDetachedProject(detachedUri, ""); - - String mainUri = LspAdapter.toUri(attachedRoot.resolve("main.smithy").toString()); - - assertThat(manager.findProjectAndFile(mainUri), notNullValue()); - assertThat(manager.findProjectAndFile(mainUri).project().getSmithyFile(mainUri), notNullValue()); - - assertThat(manager.findProjectAndFile(detachedUri), notNullValue()); - assertThat(manager.findProjectAndFile(detachedUri).project().getSmithyFile(detachedUri), notNullValue()); - - String untrackedUri = LspAdapter.toUri("/bar/baz.smithy"); - assertThat(manager.findProjectAndFile(untrackedUri), nullValue()); - } -} diff --git a/src/test/java/software/amazon/smithy/lsp/SmithyLanguageServerTest.java b/src/test/java/software/amazon/smithy/lsp/SmithyLanguageServerTest.java index f2b36628..4ac34dda 100644 --- a/src/test/java/software/amazon/smithy/lsp/SmithyLanguageServerTest.java +++ b/src/test/java/software/amazon/smithy/lsp/SmithyLanguageServerTest.java @@ -1,7 +1,7 @@ package software.amazon.smithy.lsp; import static org.hamcrest.MatcherAssert.assertThat; -import static org.hamcrest.Matchers.anEmptyMap; +import static org.hamcrest.Matchers.allOf; import static org.hamcrest.Matchers.contains; import static org.hamcrest.Matchers.containsInAnyOrder; import static org.hamcrest.Matchers.containsString; @@ -35,10 +35,7 @@ import java.util.Collections; import java.util.List; import java.util.Map; -import java.util.Set; import java.util.concurrent.CompletableFuture; -import java.util.logging.Logger; -import java.util.stream.Collectors; import org.eclipse.lsp4j.CompletionItem; import org.eclipse.lsp4j.CompletionParams; import org.eclipse.lsp4j.Diagnostic; @@ -112,11 +109,16 @@ public void formatting() throws Exception { String uri = workspace.getUri("main.smithy"); + server.didOpen(RequestBuilders.didOpen() + .uri(uri) + .text(model) + .build()); + TextDocumentIdentifier id = new TextDocumentIdentifier(uri); DocumentFormattingParams params = new DocumentFormattingParams(id, new FormattingOptions()); List edits = server.formatting(params).get(); - Document document = server.getFirstProject().getDocument(uri); + Document document = server.getState().getManagedDocument(uri); assertThat(edits, containsInAnyOrder(makesEditedDocument(document, safeString(""" $version: "2" @@ -177,7 +179,7 @@ public void didChange() throws Exception { server.getState().lifecycleManager().waitForAllTasks(); // mostly so you can see what it looks like - assertThat(server.getFirstProject().getDocument(uri).copyText(), equalTo(safeString(""" + assertThat(server.getState().getManagedDocument(uri).copyText(), equalTo(safeString(""" $version: "2" namespace com.foo @@ -218,7 +220,7 @@ public void didChangeReloadsModel() throws Exception { .text(model) .build(); server.didOpen(openParams); - assertThat(server.getFirstProject().modelResult().getValidationEvents(), empty()); + assertThat(server.getState().findProjectAndFile(uri).project().modelResult().getValidationEvents(), empty()); DidChangeTextDocumentParams didChangeParams = new RequestBuilders.DidChange() .uri(uri) @@ -229,13 +231,13 @@ public void didChangeReloadsModel() throws Exception { server.getState().lifecycleManager().getTask(uri).get(); - assertThat(server.getFirstProject().modelResult().getValidationEvents(), + assertThat(server.getState().findProjectAndFile(uri).project().modelResult().getValidationEvents(), containsInAnyOrder(eventWithMessage(containsString("Error creating trait")))); DidSaveTextDocumentParams didSaveParams = new RequestBuilders.DidSave().uri(uri).build(); server.didSave(didSaveParams); - assertThat(server.getFirstProject().modelResult().getValidationEvents(), + assertThat(server.getState().findProjectAndFile(uri).project().modelResult().getValidationEvents(), containsInAnyOrder(eventWithMessage(containsString("Error creating trait")))); } @@ -260,7 +262,7 @@ public void diagnosticsOnMemberTarget() { Diagnostic diagnostic = diagnostics.get(0); assertThat(diagnostic.getMessage(), startsWith("Target.UnresolvedShape")); - Document document = server.getFirstProject().getDocument(uri); + Document document = server.getState().findProjectAndFile(uri).file().document(); assertThat(diagnostic.getRange(), hasText(document, equalTo("Bar"))); } @@ -283,7 +285,6 @@ public void diagnosticsOnInvalidStructureMember() { assertThat(diagnostics, hasSize(1)); Diagnostic diagnostic = diagnostics.getFirst(); - Document document = server.getFirstProject().getDocument(uri); assertThat(diagnostic.getRange(), equalTo( new Range( @@ -310,7 +311,7 @@ public void diagnosticsOnUse() { server.getState().findProjectAndFile(uri), server.getMinimumSeverity()); Diagnostic diagnostic = diagnostics.getFirst(); - Document document = server.getFirstProject().getDocument(uri); + Document document = server.getState().findProjectAndFile(uri).file().document(); assertThat(diagnostic.getRange(), hasText(document, equalTo("mything#SomeUnknownThing"))); @@ -343,7 +344,7 @@ public void diagnosticOnTrait() { Diagnostic diagnostic = diagnostics.get(0); assertThat(diagnostic.getMessage(), startsWith("Model.UnresolvedTrait")); - Document document = server.getFirstProject().getDocument(uri); + Document document = server.getState().findProjectAndFile(uri).file().document(); assertThat(diagnostic.getRange(), hasText(document, equalTo("@bar"))); } @@ -425,7 +426,6 @@ public void insideJar() throws Exception { String preludeUri = preludeLocation.getUri(); assertThat(preludeUri, startsWith("smithyjar")); - Logger.getLogger(getClass().getName()).severe("DOCUMENT LINES: " + server.getFirstProject().getDocument(preludeUri).fullRange()); Hover appliedTraitInPreludeHover = server.hover(RequestBuilders.positionRequest() .uri(preludeUri) @@ -468,11 +468,8 @@ public void addingWatchedFile() throws Exception { assertThat(future, notNullValue()); future.get(); - assertThat(server.getState().managedUris().contains(uri), is(true)); - assertThat(server.getState().isDetached(uri), is(false)); - assertThat(server.getFirstProject().getSmithyFile(uri), notNullValue()); - assertThat(server.getState().findProjectAndFile(uri), notNullValue()); - assertThat(server.getState().findProjectAndFile(uri).file().document().copyText(), equalTo("$")); + assertManagedMatches(server, uri, Project.Type.NORMAL, workspace.getRoot()); + assertThat(server.getState().findManaged(uri).file().document().copyText(), equalTo("$")); } @Test @@ -501,8 +498,7 @@ public void removingWatchedFile() { .event(uri, FileChangeType.Deleted) .build()); - assertThat(server.getState().managedUris().contains(uri), is(false)); - assertThat(server.getState().findProjectAndFile(uri), nullValue()); + assertThat(server.getState().findManaged(uri), nullValue()); } @Test @@ -524,9 +520,7 @@ public void addingDetachedFile() { .text(modelText) .build()); - assertThat(server.getState().managedUris().contains(uri), is(true)); - assertThat(server.getState().isDetached(uri), is(true)); - assertThat(server.getState().findProjectAndFile(uri), notNullValue()); + assertManagedMatches(server, uri, Project.Type.DETACHED, uri); String movedFilename = "model/main.smithy"; workspace.moveModel(filename, movedFilename); @@ -542,12 +536,8 @@ public void addingDetachedFile() { .event(movedUri, FileChangeType.Created) .build()); - assertThat(server.getState().managedUris().contains(uri), is(false)); - assertThat(server.getState().isDetached(uri), is(false)); - assertThat(server.getState().findProjectAndFile(uri), nullValue()); - assertThat(server.getState().managedUris().contains(movedUri), is(true)); - assertThat(server.getState().isDetached(movedUri), is(false)); - assertThat(server.getState().findProjectAndFile(movedUri), notNullValue()); + assertThat(server.getState().findManaged(uri), nullValue()); + assertManagedMatches(server, movedUri, Project.Type.NORMAL, workspace.getRoot()); } @Test @@ -569,9 +559,7 @@ public void removingAttachedFile() { .text(modelText) .build()); - assertThat(server.getState().managedUris().contains(uri), is(true)); - assertThat(server.getState().isDetached(uri), is(false)); - assertThat(server.getState().findProjectAndFile(uri), notNullValue()); + assertManagedMatches(server, uri, Project.Type.NORMAL, workspace.getRoot()); String movedFilename = "main.smithy"; workspace.moveModel(filename, movedFilename); @@ -588,12 +576,8 @@ public void removingAttachedFile() { .event(uri, FileChangeType.Deleted) .build()); - assertThat(server.getState().managedUris().contains(uri), is(false)); - assertThat(server.getState().isDetached(uri), is(false)); - assertThat(server.getState().findProjectAndFile(uri), nullValue()); - assertThat(server.getState().managedUris().contains(movedUri), is(true)); - assertThat(server.getState().isDetached(movedUri), is(true)); - assertThat(server.getState().findProjectAndFile(movedUri), notNullValue()); + assertThat(server.getState().findManaged(uri), nullValue()); + assertManagedMatches(server, movedUri, Project.Type.DETACHED, movedUri); } @Test @@ -624,9 +608,7 @@ public void loadsProjectWithUnNormalizedSourcesDirs() { .text(modelText) .build()); - assertThat(server.getState().managedUris().contains(uri), is(true)); - assertThat(server.getState().isDetached(uri), is(false)); - assertThat(server.getState().findProjectAndFile(uri), notNullValue()); + assertManagedMatches(server, uri, Project.Type.NORMAL, workspace.getRoot()); } @Test @@ -654,7 +636,7 @@ public void reloadingProjectWithArrayMetadataValues() throws Exception { TestWorkspace workspace = TestWorkspace.multipleModels(modelText1, modelText2); SmithyLanguageServer server = initFromWorkspace(workspace); - Map metadataBefore = server.getFirstProject().modelResult().unwrap().getMetadata(); + Map metadataBefore = server.getState().findProjectByRoot(workspace.getRoot().toString()).modelResult().unwrap().getMetadata(); assertThat(metadataBefore, hasKey("foo")); assertThat(metadataBefore, hasKey("bar")); assertThat(metadataBefore.get("foo"), instanceOf(ArrayNode.class)); @@ -676,7 +658,7 @@ public void reloadingProjectWithArrayMetadataValues() throws Exception { server.getState().lifecycleManager().getTask(uri).get(); - Map metadataAfter = server.getFirstProject().modelResult().unwrap().getMetadata(); + Map metadataAfter = server.getState().findProjectByRoot(workspace.getRoot().toString()).modelResult().unwrap().getMetadata(); assertThat(metadataAfter, hasKey("foo")); assertThat(metadataAfter, hasKey("bar")); assertThat(metadataAfter.get("foo"), instanceOf(ArrayNode.class)); @@ -690,7 +672,7 @@ public void reloadingProjectWithArrayMetadataValues() throws Exception { server.getState().lifecycleManager().getTask(uri).get(); - Map metadataAfter2 = server.getFirstProject().modelResult().unwrap().getMetadata(); + Map metadataAfter2 = server.getState().findProjectByRoot(workspace.getRoot().toString()).modelResult().unwrap().getMetadata(); assertThat(metadataAfter2, hasKey("foo")); assertThat(metadataAfter2, hasKey("bar")); assertThat(metadataAfter2.get("foo"), instanceOf(ArrayNode.class)); @@ -722,7 +704,7 @@ public void changingWatchedFilesWithMetadata() throws Exception { TestWorkspace workspace = TestWorkspace.multipleModels(modelText1, modelText2); SmithyLanguageServer server = initFromWorkspace(workspace); - Map metadataBefore = server.getFirstProject().modelResult().unwrap().getMetadata(); + Map metadataBefore = server.getState().findProjectByRoot(workspace.getRoot().toString()).modelResult().unwrap().getMetadata(); assertThat(metadataBefore, hasKey("foo")); assertThat(metadataBefore, hasKey("bar")); assertThat(metadataBefore.get("foo"), instanceOf(ArrayNode.class)); @@ -737,14 +719,13 @@ public void changingWatchedFilesWithMetadata() throws Exception { server.getState().lifecycleManager().waitForAllTasks(); - Map metadataAfter = server.getFirstProject().modelResult().unwrap().getMetadata(); + Map metadataAfter = server.getState().findProjectByRoot(workspace.getRoot().toString()).modelResult().unwrap().getMetadata(); assertThat(metadataAfter, hasKey("foo")); assertThat(metadataAfter, hasKey("bar")); assertThat(metadataAfter.get("foo"), instanceOf(ArrayNode.class)); assertThat(metadataAfter.get("foo").expectArrayNode().size(), equalTo(2)); } - // TODO: Somehow this is flaky @Test public void addingOpenedDetachedFile() throws Exception { TestWorkspace workspace = TestWorkspace.emptyWithDirSource(); @@ -761,18 +742,14 @@ public void addingOpenedDetachedFile() throws Exception { String uri = workspace.getUri("main.smithy"); - assertThat(server.getState().managedUris(), not(hasItem(uri))); - assertThat(server.getState().isDetached(uri), is(false)); - assertThat(server.getState().findProjectAndFile(uri), nullValue()); + assertThat(server.getState().findManaged(uri), nullValue()); server.didOpen(RequestBuilders.didOpen() .uri(uri) .text(modelText) .build()); - assertThat(server.getState().managedUris(), hasItem(uri)); - assertThat(server.getState().isDetached(uri), is(true)); - assertThat(server.getState().findProjectAndFile(uri), notNullValue()); + assertManagedMatches(server, uri, Project.Type.DETACHED, uri); server.didChange(RequestBuilders.didChange() .uri(uri) @@ -794,11 +771,11 @@ public void addingOpenedDetachedFile() throws Exception { server.getState().lifecycleManager().waitForAllTasks(); - assertThat(server.getState().managedUris(), hasItem(uri)); - assertThat(server.getState().isDetached(uri), is(false)); - assertThat(server.getFirstProject().getSmithyFile(uri), notNullValue()); - assertThat(server.getFirstProject().modelResult().unwrap(), hasShapeWithId("com.foo#Foo")); - assertThat(server.getFirstProject().modelResult().unwrap(), hasShapeWithId("com.foo#Bar")); + assertManagedMatches(server, uri, Project.Type.NORMAL, workspace.getRoot()); + assertThat(server.getState().findManaged(uri).project().modelResult().unwrap(), allOf( + hasShapeWithId("com.foo#Foo"), + hasShapeWithId("com.foo#Bar") + )); } @Test @@ -834,12 +811,11 @@ public void detachingOpenedFile() throws Exception { server.getState().lifecycleManager().waitForAllTasks(); - assertThat(server.getState().managedUris(), hasItem(uri)); - assertThat(server.getState().isDetached(uri), is(true)); - ProjectAndFile projectAndFile = server.getState().findProjectAndFile(uri); - assertThat(projectAndFile, notNullValue()); - assertThat(projectAndFile.project().modelResult(), hasValue(hasShapeWithId("com.foo#Foo"))); - assertThat(projectAndFile.project().modelResult(), hasValue(hasShapeWithId("com.foo#Bar"))); + assertManagedMatches(server, uri, Project.Type.DETACHED, uri); + assertThat(server.getState().findManaged(uri).project().modelResult(), hasValue(allOf( + hasShapeWithId("com.foo#Foo"), + hasShapeWithId("com.foo#Bar") + ))); } @Test @@ -878,12 +854,8 @@ public void movingDetachedFile() throws Exception { server.getState().lifecycleManager().waitForAllTasks(); - assertThat(server.getState().managedUris().contains(uri), is(false)); - assertThat(server.getState().findProjectAndFile(uri), nullValue()); - assertThat(server.getState().isDetached(uri), is(false)); - assertThat(server.getState().managedUris().contains(movedUri), is(true)); - assertThat(server.getState().findProjectAndFile(movedUri), notNullValue()); - assertThat(server.getState().isDetached(movedUri), is(true)); + assertThat(server.getState().findManaged(uri), nullValue()); + assertManagedMatches(server, movedUri, Project.Type.DETACHED, movedUri); } @Test @@ -957,12 +929,19 @@ public void invalidSyntaxModelPartiallyLoads() { TestWorkspace workspace = TestWorkspace.multipleModels(modelText1, modelText2); SmithyLanguageServer server = initFromWorkspace(workspace); - String uri = workspace.getUri("model-0.smithy"); + Project project = server.getState().findProjectByRoot(workspace.getRoot().toString()); + assertThat(project, notNullValue()); + assertThat(project.modelResult().isBroken(), is(true)); + assertThat(project.modelResult().getResult().isPresent(), is(true)); + assertThat(project.modelResult().getResult().get(), hasShapeWithId("com.foo#Foo")); - assertThat(server.getFirstProject().getSmithyFile(uri), notNullValue()); - assertThat(server.getFirstProject().modelResult().isBroken(), is(true)); - assertThat(server.getFirstProject().modelResult().getResult().isPresent(), is(true)); - assertThat(server.getFirstProject().modelResult().getResult().get(), hasShapeWithId("com.foo#Foo")); + String uri = workspace.getUri("model-1.smithy"); + server.didOpen(RequestBuilders.didOpen() + .uri(uri) + .text(modelText2) + .build()); + + assertManagedMatches(server, uri, Project.Type.NORMAL, workspace.getRoot()); } @Test @@ -982,7 +961,7 @@ public void invalidSyntaxDetachedProjectBecomesValid() throws Exception { server.getState().lifecycleManager().waitForAllTasks(); - assertThat(server.getState().isDetached(uri), is(true)); + assertManagedMatches(server, uri, Project.Type.DETACHED, uri); ProjectAndFile projectAndFile = server.getState().findProjectAndFile(uri); assertThat(projectAndFile, notNullValue()); assertThat(projectAndFile.project().modelResult().isBroken(), is(true)); @@ -1000,7 +979,7 @@ public void invalidSyntaxDetachedProjectBecomesValid() throws Exception { server.getState().lifecycleManager().waitForAllTasks(); - assertThat(server.getState().isDetached(uri), is(true)); + assertManagedMatches(server, uri, Project.Type.DETACHED, uri); ProjectAndFile projectAndFile1 = server.getState().findProjectAndFile(uri); assertThat(projectAndFile1, notNullValue()); assertThat(projectAndFile1.project().modelResult().isBroken(), is(false)); @@ -1009,7 +988,6 @@ public void invalidSyntaxDetachedProjectBecomesValid() throws Exception { assertThat(projectAndFile1.project().modelResult().unwrap(), hasShapeWithId("com.foo#Foo")); } - // TODO: apparently flaky @Test public void addingDetachedFileWithInvalidSyntax() throws Exception { TestWorkspace workspace = TestWorkspace.emptyWithDirSource(); @@ -1027,10 +1005,7 @@ public void addingDetachedFileWithInvalidSyntax() throws Exception { server.getState().lifecycleManager().waitForAllTasks(); - assertThat(server.getState().isDetached(uri), is(true)); - ProjectAndFile projectAndFile = server.getState().findProjectAndFile(uri); - assertThat(projectAndFile, notNullValue()); - assertThat(projectAndFile.project().getSmithyFile(uri), notNullValue()); + assertManagedMatches(server, uri, Project.Type.DETACHED, uri); List updatedSources = new ArrayList<>(workspace.getConfig().getSources()); updatedSources.add(filename); @@ -1061,10 +1036,8 @@ public void addingDetachedFileWithInvalidSyntax() throws Exception { server.getState().lifecycleManager().waitForAllTasks(); - assertThat(server.getState().isDetached(uri), is(false)); - assertThat(server.getState().detachedProjects().keySet(), empty()); - assertThat(server.getFirstProject().getSmithyFile(uri), notNullValue()); - assertThat(server.getFirstProject().modelResult(), hasValue(hasShapeWithId("com.foo#Foo"))); + assertManagedMatches(server, uri, Project.Type.NORMAL, workspace.getRoot()); + assertThat(server.getState().findManaged(uri).project().modelResult(), hasValue(hasShapeWithId("com.foo#Foo"))); } @Test @@ -1097,8 +1070,12 @@ public void appliedTraitsAreMaintainedInPartialLoad() throws Exception { server.getState().lifecycleManager().waitForAllTasks(); - assertThat(server.getFirstProject().modelResult(), hasValue(hasShapeWithId("com.foo#Bar"))); - Shape foo = server.getFirstProject().modelResult().getResult().get().expectShape(ShapeId.from("com.foo#Foo")); + ProjectAndFile projectAndFile = server.getState().findProjectAndFile(uri2); + assertThat(projectAndFile, notNullValue()); + assertThat(projectAndFile.project().modelResult(), hasValue(hasShapeWithId("com.foo#Foo"))); + assertThat(projectAndFile.project().modelResult(), hasValue(hasShapeWithId("com.foo#Bar"))); + + Shape foo = projectAndFile.project().modelResult().getResult().get().expectShape(ShapeId.from("com.foo#Foo")); assertThat(foo.getIntroducedTraits().keySet(), containsInAnyOrder(LengthTrait.ID)); assertThat(foo.expectTrait(LengthTrait.class).getMin(), anOptionalOf(equalTo(2L))); @@ -1116,9 +1093,13 @@ public void appliedTraitsAreMaintainedInPartialLoad() throws Exception { server.getState().lifecycleManager().waitForAllTasks(); - assertThat(server.getFirstProject().modelResult(), hasValue(hasShapeWithId("com.foo#Bar"))); - assertThat(server.getFirstProject().modelResult(), hasValue(hasShapeWithId("com.foo#Another"))); - foo = server.getFirstProject().modelResult().getResult().get().expectShape(ShapeId.from("com.foo#Foo")); + projectAndFile = server.getState().findProjectAndFile(uri1); + assertThat(projectAndFile, notNullValue()); + assertThat(projectAndFile.project().modelResult(), hasValue(hasShapeWithId("com.foo#Foo"))); + assertThat(projectAndFile.project().modelResult(), hasValue(hasShapeWithId("com.foo#Bar"))); + assertThat(projectAndFile.project().modelResult(), hasValue(hasShapeWithId("com.foo#Another"))); + + foo = projectAndFile.project().modelResult().getResult().get().expectShape(ShapeId.from("com.foo#Foo")); assertThat(foo.getIntroducedTraits().keySet(), containsInAnyOrder(LengthTrait.ID)); assertThat(foo.expectTrait(LengthTrait.class).getMin(), anOptionalOf(equalTo(2L))); } @@ -1212,14 +1193,11 @@ public void loadsMultipleRoots() { SmithyLanguageServer server = initFromWorkspaces(workspaceFoo, workspaceBar); - assertThat(server.getState().attachedProjects(), hasKey(workspaceFoo.getName())); - assertThat(server.getState().attachedProjects(), hasKey(workspaceBar.getName())); + Project projectFoo = server.getState().findProjectByRoot(workspaceFoo.getName()); + Project projectBar = server.getState().findProjectByRoot(workspaceBar.getName()); - assertThat(server.getState().findProjectAndFile(workspaceFoo.getUri("foo.smithy")), notNullValue()); - assertThat(server.getState().findProjectAndFile(workspaceBar.getUri("bar.smithy")), notNullValue()); - - Project projectFoo = server.getState().attachedProjects().get(workspaceFoo.getName()); - Project projectBar = server.getState().attachedProjects().get(workspaceBar.getName()); + assertThat(projectFoo, notNullValue()); + assertThat(projectBar, notNullValue()); assertThat(projectFoo.smithyFiles(), hasKey(endsWith("foo.smithy"))); assertThat(projectBar.smithyFiles(), hasKey(endsWith("bar.smithy"))); @@ -1280,8 +1258,8 @@ public void multiRootLifecycleManagement() throws Exception { server.getState().lifecycleManager().waitForAllTasks(); - Project projectFoo = server.getState().attachedProjects().get(workspaceFoo.getName()); - Project projectBar = server.getState().attachedProjects().get(workspaceBar.getName()); + Project projectFoo = server.getState().findProjectByRoot(workspaceFoo.getName()); + Project projectBar = server.getState().findProjectByRoot(workspaceBar.getName()); assertThat(projectFoo.modelResult(), SmithyMatchers.hasValue(SmithyMatchers.hasShapeWithId("com.foo#Foo"))); assertThat(projectFoo.modelResult(), SmithyMatchers.hasValue(SmithyMatchers.hasShapeWithId("com.foo#Bar"))); @@ -1351,8 +1329,8 @@ public void multiRootAddingWatchedFile() throws Exception { server.getState().lifecycleManager().waitForAllTasks(); - Project projectFoo = server.getState().attachedProjects().get(workspaceFoo.getName()); - Project projectBar = server.getState().attachedProjects().get(workspaceBar.getName()); + Project projectFoo = server.getState().findProjectByRoot(workspaceFoo.getName()); + Project projectBar = server.getState().findProjectByRoot(workspaceBar.getName()); assertThat(projectFoo.smithyFiles(), hasKey(endsWith("main.smithy"))); assertThat(projectBar.smithyFiles(), hasKey(endsWith("main.smithy"))); @@ -1417,6 +1395,9 @@ public void multiRootChangingBuildFile() throws Exception { .event(workspaceBar.getUri("smithy-build.json"), FileChangeType.Changed) .build()); + server.didOpen(RequestBuilders.didOpen() + .uri(workspaceBar.getUri("model/main.smithy")) + .build()); server.didChange(RequestBuilders.didChange() .uri(workspaceBar.getUri("model/main.smithy")) .text(""" @@ -1431,22 +1412,21 @@ public void multiRootChangingBuildFile() throws Exception { server.getState().lifecycleManager().waitForAllTasks(); - assertThat(server.getState().detachedProjects(), anEmptyMap()); assertThat(server.getState().findProjectAndFile(newUri), notNullValue()); assertThat(server.getState().findProjectAndFile(workspaceBar.getUri("model/main.smithy")), notNullValue()); assertThat(server.getState().findProjectAndFile(workspaceFoo.getUri("model/main.smithy")), notNullValue()); - Project projectFoo = server.getState().attachedProjects().get(workspaceFoo.getName()); - Project projectBar = server.getState().attachedProjects().get(workspaceBar.getName()); + Project projectFoo = server.getState().findProjectByRoot(workspaceFoo.getName()); + Project projectBar = server.getState().findProjectByRoot(workspaceBar.getName()); assertThat(projectFoo.smithyFiles(), hasKey(endsWith("main.smithy"))); assertThat(projectBar.smithyFiles(), hasKey(endsWith("main.smithy"))); assertThat(projectBar.smithyFiles(), hasKey(endsWith("other.smithy"))); - assertThat(projectFoo.modelResult(), SmithyMatchers.hasValue(SmithyMatchers.hasShapeWithId("com.foo#Foo"))); - assertThat(projectBar.modelResult(), SmithyMatchers.hasValue(SmithyMatchers.hasShapeWithId("com.bar#Bar"))); - assertThat(projectBar.modelResult(), SmithyMatchers.hasValue(SmithyMatchers.hasShapeWithId("com.bar#Bar$other"))); - assertThat(projectBar.modelResult(), SmithyMatchers.hasValue(SmithyMatchers.hasShapeWithId("com.other#Other"))); + assertThat(projectFoo.modelResult(), hasValue(hasShapeWithId("com.foo#Foo"))); + assertThat(projectBar.modelResult(), hasValue(hasShapeWithId("com.bar#Bar"))); + assertThat(projectBar.modelResult(), hasValue(hasShapeWithId("com.bar#Bar$other"))); + assertThat(projectBar.modelResult(), hasValue(hasShapeWithId("com.other#Other"))); } @Test @@ -1489,14 +1469,11 @@ public void addingWorkspaceFolder() throws Exception { server.getState().lifecycleManager().waitForAllTasks(); - assertThat(server.getState().attachedProjects(), hasKey(workspaceFoo.getName())); - assertThat(server.getState().attachedProjects(), hasKey(workspaceBar.getName())); - - assertThat(server.getState().findProjectAndFile(workspaceFoo.getUri("foo.smithy")), notNullValue()); - assertThat(server.getState().findProjectAndFile(workspaceBar.getUri("bar.smithy")), notNullValue()); + assertManagedMatches(server, workspaceFoo.getUri("foo.smithy"), Project.Type.NORMAL, workspaceFoo.getRoot()); + assertManagedMatches(server, workspaceBar.getUri("bar.smithy"), Project.Type.NORMAL, workspaceBar.getRoot()); - Project projectFoo = server.getState().attachedProjects().get(workspaceFoo.getName()); - Project projectBar = server.getState().attachedProjects().get(workspaceBar.getName()); + Project projectFoo = server.getState().findProjectByRoot(workspaceFoo.getName()); + Project projectBar = server.getState().findProjectByRoot(workspaceBar.getName()); assertThat(projectFoo.smithyFiles(), hasKey(endsWith("foo.smithy"))); assertThat(projectBar.smithyFiles(), hasKey(endsWith("bar.smithy"))); @@ -1529,13 +1506,15 @@ public void removingWorkspaceFolder() { SmithyLanguageServer server = initFromWorkspaces(workspaceFoo, workspaceBar); + String fooUri = workspaceFoo.getUri("foo.smithy"); server.didOpen(RequestBuilders.didOpen() - .uri(workspaceFoo.getUri("foo.smithy")) + .uri(fooUri) .text(fooModel) .build()); + String barUri = workspaceBar.getUri("bar.smithy"); server.didOpen(RequestBuilders.didOpen() - .uri(workspaceBar.getUri("bar.smithy")) + .uri(barUri) .text(barModel) .build()); @@ -1543,22 +1522,17 @@ public void removingWorkspaceFolder() { .removed(workspaceBar.getRoot().toUri().toString(), "bar") .build()); - assertThat(server.getState().attachedProjects(), hasKey(workspaceFoo.getName())); - assertThat(server.getState().attachedProjects(), not(hasKey(workspaceBar.getName()))); - assertThat(server.getState().detachedProjects(), hasKey(endsWith("bar.smithy"))); - assertThat(server.getState().isDetached(workspaceBar.getUri("bar.smithy")), is(true)); - - assertThat(server.getState().findProjectAndFile(workspaceFoo.getUri("foo.smithy")), notNullValue()); - assertThat(server.getState().findProjectAndFile(workspaceBar.getUri("bar.smithy")), notNullValue()); + assertManagedMatches(server, fooUri, Project.Type.NORMAL, workspaceFoo.getRoot()); + assertManagedMatches(server, barUri, Project.Type.DETACHED, barUri); - Project projectFoo = server.getState().attachedProjects().get(workspaceFoo.getName()); - Project projectBar = server.getState().findProjectAndFile(workspaceBar.getUri("bar.smithy")).project(); + Project projectFoo = server.getState().findProjectByRoot(workspaceFoo.getName()); + Project projectBar = server.getState().findProjectByRoot(barUri); assertThat(projectFoo.smithyFiles(), hasKey(endsWith("foo.smithy"))); assertThat(projectBar.smithyFiles(), hasKey(endsWith("bar.smithy"))); - assertThat(projectFoo.modelResult(), SmithyMatchers.hasValue(SmithyMatchers.hasShapeWithId("com.foo#Foo"))); - assertThat(projectBar.modelResult(), SmithyMatchers.hasValue(SmithyMatchers.hasShapeWithId("com.bar#Bar"))); + assertThat(projectFoo.modelResult(), hasValue(hasShapeWithId("com.foo#Foo"))); + assertThat(projectBar.modelResult(), hasValue(hasShapeWithId("com.bar#Bar"))); } @Test @@ -1590,9 +1564,8 @@ public void singleWorkspaceMultiRoot() throws Exception { SmithyLanguageServer server = initFromRoot(root); - assertThat(server.getState().attachedProjects().keySet(), containsInAnyOrder( - workspaceFoo.getName(), - workspaceBar.getName())); + assertThat(server.getState().findProjectByRoot(workspaceFoo.getName()), notNullValue()); + assertThat(server.getState().findProjectByRoot(workspaceBar.getName()), notNullValue()); assertThat(server.getState().workspacePaths(), contains(root)); } @@ -1631,9 +1604,8 @@ public void addingRootsToWorkspace() throws Exception { .build()); assertThat(server.getState().workspacePaths(), contains(root)); - assertThat(server.getState().attachedProjects().keySet(), containsInAnyOrder( - workspaceFoo.getName(), - workspaceBar.getName())); + assertThat(server.getState().findProjectByRoot(workspaceFoo.getName()), notNullValue()); + assertThat(server.getState().findProjectByRoot(workspaceBar.getName()), notNullValue()); } @Test @@ -1665,10 +1637,9 @@ public void removingRootsFromWorkspace() throws Exception { SmithyLanguageServer server = initFromRoot(root); - assertThat(server.getState().attachedProjects().keySet(), containsInAnyOrder( - workspaceFoo.getName(), - workspaceBar.getName())); assertThat(server.getState().workspacePaths(), contains(root)); + assertThat(server.getState().findProjectByRoot(workspaceFoo.getName()), notNullValue()); + assertThat(server.getState().findProjectByRoot(workspaceBar.getName()), notNullValue()); workspaceFoo.deleteModel("smithy-build.json"); @@ -1677,7 +1648,8 @@ public void removingRootsFromWorkspace() throws Exception { .build()); assertThat(server.getState().workspacePaths(), contains(root)); - assertThat(server.getState().attachedProjects().keySet(), contains(workspaceBar.getName())); + assertThat(server.getState().findProjectByRoot(workspaceFoo.getName()), nullValue()); + assertThat(server.getState().findProjectByRoot(workspaceBar.getName()), notNullValue()); } @Test @@ -1709,22 +1681,34 @@ public void addingConfigFile() throws Exception { SmithyLanguageServer server = initFromRoot(root); + String fooUri = workspaceFoo.getUri("foo.smithy"); + server.didOpen(RequestBuilders.didOpen() + .uri(fooUri) + .text(fooModel) + .build()); + + String barUri = workspaceBar.getUri("bar.smithy"); + server.didOpen(RequestBuilders.didOpen() + .uri(barUri) + .text(barModel) + .build()); + String bazModel = """ $version: "2" namespace com.baz structure Baz {} """; workspaceFoo.addModel("baz.smithy", bazModel); + String bazUri = workspaceFoo.getUri("baz.smithy"); server.didOpen(RequestBuilders.didOpen() - .uri(workspaceFoo.getUri("baz.smithy")) + .uri(bazUri) .text(bazModel) .build()); assertThat(server.getState().workspacePaths(), contains(root)); - assertThat(server.getState().attachedProjects().keySet(), containsInAnyOrder( - workspaceFoo.getName(), - workspaceBar.getName())); - assertThat(server.getState().detachedProjects().keySet(), contains(workspaceFoo.getUri("baz.smithy"))); + assertManagedMatches(server, fooUri, Project.Type.NORMAL, workspaceFoo.getRoot()); + assertManagedMatches(server, barUri, Project.Type.NORMAL, workspaceBar.getRoot()); + assertManagedMatches(server, bazUri, Project.Type.DETACHED, bazUri); workspaceFoo.addModel(".smithy-project.json", """ { @@ -1734,10 +1718,9 @@ public void addingConfigFile() throws Exception { .event(workspaceFoo.getUri(".smithy-project.json"), FileChangeType.Created) .build()); - assertThat(server.getState().attachedProjects().keySet(), containsInAnyOrder( - workspaceFoo.getName(), - workspaceBar.getName())); - assertThat(server.getState().detachedProjects().keySet(), empty()); + assertManagedMatches(server, fooUri, Project.Type.NORMAL, workspaceFoo.getRoot()); + assertManagedMatches(server, barUri, Project.Type.NORMAL, workspaceBar.getRoot()); + assertManagedMatches(server, bazUri, Project.Type.NORMAL, workspaceFoo.getRoot()); } @Test @@ -1779,26 +1762,37 @@ public void removingConfigFile() throws Exception { SmithyLanguageServer server = initFromRoot(root); + String fooUri = workspaceFoo.getUri("foo.smithy"); server.didOpen(RequestBuilders.didOpen() - .uri(workspaceFoo.getUri("baz.smithy")) + .uri(fooUri) + .text(fooModel) + .build()); + + String barUri = workspaceBar.getUri("bar.smithy"); + server.didOpen(RequestBuilders.didOpen() + .uri(barUri) + .text(barModel) + .build()); + + String bazUri = workspaceFoo.getUri("baz.smithy"); + server.didOpen(RequestBuilders.didOpen() + .uri(bazUri) .text(bazModel) .build()); assertThat(server.getState().workspacePaths(), contains(root)); - assertThat(server.getState().attachedProjects().keySet(), containsInAnyOrder( - workspaceFoo.getName(), - workspaceBar.getName())); - assertThat(server.getState().detachedProjects().keySet(), empty()); + assertManagedMatches(server, fooUri, Project.Type.NORMAL, workspaceFoo.getRoot()); + assertManagedMatches(server, barUri, Project.Type.NORMAL, workspaceBar.getRoot()); + assertManagedMatches(server, bazUri, Project.Type.NORMAL, workspaceFoo.getRoot()); workspaceFoo.deleteModel(".smithy-project.json"); server.didChangeWatchedFiles(RequestBuilders.didChangeWatchedFiles() .event(workspaceFoo.getUri(".smithy-project.json"), FileChangeType.Deleted) .build()); - assertThat(server.getState().attachedProjects().keySet(), containsInAnyOrder( - workspaceFoo.getName(), - workspaceBar.getName())); - assertThat(server.getState().detachedProjects().keySet(), contains(workspaceFoo.getUri("baz.smithy"))); + assertManagedMatches(server, fooUri, Project.Type.NORMAL, workspaceFoo.getRoot()); + assertManagedMatches(server, barUri, Project.Type.NORMAL, workspaceBar.getRoot()); + assertManagedMatches(server, bazUri, Project.Type.DETACHED, bazUri); } @Test @@ -1816,14 +1810,8 @@ public void tracksJsonFiles() { """); SmithyLanguageServer server = initFromWorkspaces(workspace); - assertServerState(server, new ServerState( - Map.of( - workspace.getName(), - new ProjectState( - Set.of(workspace.getUri("model/main.json")), - Set.of(workspace.getUri("smithy-build.json")))), - Map.of() - )); + Project project = server.getState().findProjectByRoot(workspace.getName()); + assertThat(project.modelResult(), hasValue(hasShapeWithId("com.foo#Foo"))); } @Test @@ -1839,8 +1827,7 @@ public void tracksBuildFileChanges() { .text(smithyBuildJson) .build()); - assertThat(server.getState().managedUris(), contains(uri)); - assertThat(server.getState().getManagedDocument(uri), notNullValue()); + assertManagedMatches(server, uri, Project.Type.NORMAL, workspace.getRoot()); assertThat(server.getState().getManagedDocument(uri).copyText(), equalTo(smithyBuildJson)); String updatedSmithyBuildJson = """ @@ -1862,8 +1849,7 @@ public void tracksBuildFileChanges() { .uri(uri) .build()); - assertThat(server.getState().managedUris(), not(contains(uri))); - assertThat(server.getState().getManagedDocument(uri), nullValue()); + assertThat(server.getState().findManaged(uri), nullValue()); } @Test @@ -1884,12 +1870,13 @@ public void reloadsProjectOnBuildFileSave() { string Foo """; workspace.addModel("foo.smithy", model); + String fooUri = workspace.getUri("foo.smithy"); server.didOpen(RequestBuilders.didOpen() - .uri(workspace.getUri("foo.smithy")) + .uri(fooUri) .text(model) .build()); - assertThat(server.getState().detachedProjects().keySet(), contains(workspace.getUri("foo.smithy"))); + assertManagedMatches(server, fooUri, Project.Type.DETACHED, fooUri); String updatedBuildJson = """ { @@ -1905,14 +1892,7 @@ public void reloadsProjectOnBuildFileSave() { .uri(buildJsonUri) .build()); - assertThat(server.getState().managedUris(), containsInAnyOrder( - buildJsonUri, - workspace.getUri("foo.smithy"))); - assertServerState(server, new ServerState( - Map.of(workspace.getName(), new ProjectState( - Set.of(workspace.getUri("foo.smithy")), - Set.of(buildJsonUri))), - Map.of())); + assertManagedMatches(server, fooUri, Project.Type.NORMAL, workspace.getRoot()); } @Test @@ -1962,41 +1942,28 @@ public void testFromInitializeParamsWithPartialOptions() { assertThat(options.getOnlyReloadOnSave(), equalTo(true)); // Explicitly set value } - private void assertServerState(SmithyLanguageServer server, ServerState expected) { - ServerState actual = ServerState.from(server); - assertThat(actual, equalTo(expected)); - } - - record ServerState( - Map attached, - Map detached + private void assertManagedMatches( + SmithyLanguageServer server, + String uri, + Project.Type expectedType, + String expectedRootUri ) { - static ServerState from(SmithyLanguageServer server) { - return new ServerState( - server.getState().attachedProjects().entrySet().stream() - .collect(Collectors.toMap(Map.Entry::getKey, e -> ProjectState.from(e.getValue()))), - server.getState().detachedProjects().entrySet().stream() - .collect(Collectors.toMap(Map.Entry::getKey, e -> ProjectState.from(e.getValue())))); - } + ProjectAndFile projectAndFile = server.getState().findManaged(uri); + assertThat(projectAndFile, notNullValue()); + assertThat(projectAndFile.project().type(), equalTo(expectedType)); + assertThat(projectAndFile.project().root().toString(), equalTo(LspAdapter.toPath(expectedRootUri))); } - record ProjectState( - Set smithyFileUris, - Set buildFileUris + private void assertManagedMatches( + SmithyLanguageServer server, + String uri, + Project.Type expectedType, + Path expectedRootPath ) { - static ProjectState from(Project project) { - Set smithyFileUris = project.smithyFiles().keySet() - .stream() - .map(LspAdapter::toUri) - // Ignore these to make comparisons simpler - .filter(uri -> !LspAdapter.isSmithyJarFile(uri)) - .collect(Collectors.toSet()); - Set buildFileUris = project.config().buildFiles().keySet() - .stream() - .map(LspAdapter::toUri) - .collect(Collectors.toSet()); - return new ProjectState(smithyFileUris, buildFileUris); - } + ProjectAndFile projectAndFile = server.getState().findManaged(uri); + assertThat(projectAndFile, notNullValue()); + assertThat(projectAndFile.project().type(), equalTo(expectedType)); + assertThat(projectAndFile.project().root(), equalTo(expectedRootPath)); } public static SmithyLanguageServer initFromWorkspace(TestWorkspace workspace) { diff --git a/src/test/java/software/amazon/smithy/lsp/SmithyVersionRefactoringTest.java b/src/test/java/software/amazon/smithy/lsp/SmithyVersionRefactoringTest.java index a806b492..9af70e0c 100644 --- a/src/test/java/software/amazon/smithy/lsp/SmithyVersionRefactoringTest.java +++ b/src/test/java/software/amazon/smithy/lsp/SmithyVersionRefactoringTest.java @@ -87,7 +87,7 @@ public void noVersionDiagnostic() throws Exception { List edits = action.getEdit().getChanges().get(uri); assertThat(edits, hasSize(1)); TextEdit edit = edits.get(0); - Document document = server.getFirstProject().getDocument(uri); + Document document = server.getState().findProjectAndFile(uri).file().document(); document.applyEdit(edit.getRange(), edit.getNewText()); assertThat(document.copyText(), equalTo(safeString(""" $version: "1" @@ -141,7 +141,7 @@ public void oldVersionDiagnostic() throws Exception { List edits = action.getEdit().getChanges().get(uri); assertThat(edits, hasSize(1)); TextEdit edit = edits.get(0); - Document document = server.getFirstProject().getDocument(uri); + Document document = server.getState().findProjectAndFile(uri).file().document(); document.applyEdit(edit.getRange(), edit.getNewText()); assertThat(document.copyText(), equalTo(""" $version: "2" From 350f0aa6a19769c5977c79124c14d9dbf97c2923 Mon Sep 17 00:00:00 2001 From: Miles Ziemer Date: Mon, 23 Dec 2024 10:24:17 -0500 Subject: [PATCH 2/9] Store only one collection of projects --- .../amazon/smithy/lsp/ServerState.java | 117 +++++++++--------- .../smithy/lsp/SmithyLanguageServer.java | 45 ++----- .../lsp/diagnostics/SmithyDiagnostics.java | 2 +- .../smithy/lsp/project/ProjectAndFile.java | 3 +- 4 files changed, 67 insertions(+), 100 deletions(-) diff --git a/src/main/java/software/amazon/smithy/lsp/ServerState.java b/src/main/java/software/amazon/smithy/lsp/ServerState.java index 04a41033..7adabf0a 100644 --- a/src/main/java/software/amazon/smithy/lsp/ServerState.java +++ b/src/main/java/software/amazon/smithy/lsp/ServerState.java @@ -17,10 +17,12 @@ import java.util.Map; import java.util.Set; import java.util.logging.Logger; +import org.eclipse.lsp4j.FileEvent; import org.eclipse.lsp4j.WorkspaceFolder; import software.amazon.smithy.lsp.document.Document; import software.amazon.smithy.lsp.project.Project; import software.amazon.smithy.lsp.project.ProjectAndFile; +import software.amazon.smithy.lsp.project.ProjectChange; import software.amazon.smithy.lsp.project.ProjectFile; import software.amazon.smithy.lsp.project.ProjectLoader; import software.amazon.smithy.lsp.protocol.LspAdapter; @@ -32,8 +34,7 @@ public final class ServerState { private static final Logger LOGGER = Logger.getLogger(ServerState.class.getName()); - private final Map detachedProjects; - private final Map attachedProjects; + private final Map projects; private final Set workspacePaths; private final Set managedUris; private final DocumentLifecycleManager lifecycleManager; @@ -42,17 +43,14 @@ public final class ServerState { * Create a new, empty server state. */ public ServerState() { - this.detachedProjects = new HashMap<>(); - this.attachedProjects = new HashMap<>(); + this.projects = new HashMap<>(); this.workspacePaths = new HashSet<>(); this.managedUris = new HashSet<>(); this.lifecycleManager = new DocumentLifecycleManager(); } public Collection getAllProjects() { - List allProjects = new ArrayList<>(attachedProjects.values()); - allProjects.addAll(detachedProjects.values()); - return allProjects; + return projects.values(); } public Collection getAllManaged() { @@ -72,11 +70,7 @@ public DocumentLifecycleManager lifecycleManager() { } public Project findProjectByRoot(String root) { - Project attached = attachedProjects.get(root); - if (attached == null) { - return detachedProjects.get(root); - } - return attached; + return projects.get(root); } /** @@ -108,17 +102,12 @@ public Document getManagedDocument(Path path) { } ProjectAndFile findProjectAndFile(String uri) { - ProjectAndFile attached = findAttachedAndRemoveDetached(uri); - if (attached != null) { - return attached; - } + String path = LspAdapter.toPath(uri); - Project detachedProject = detachedProjects.get(uri); - if (detachedProject != null) { - String path = LspAdapter.toPath(uri); - ProjectFile projectFile = detachedProject.getProjectFile(path); + for (Project project : projects.values()) { + ProjectFile projectFile = project.getProjectFile(path); if (projectFile != null) { - return new ProjectAndFile(uri, detachedProject, projectFile, true); + return new ProjectAndFile(uri, project, projectFile); } } @@ -155,37 +144,8 @@ void close(String uri) { if (projectAndFile != null && projectAndFile.project().type() == Project.Type.DETACHED) { // Only cancel tasks for detached projects, since we're dropping the project lifecycleManager.cancelTask(uri); - detachedProjects.remove(uri); - } - } - - /** - * Searches for the given {@code uri} in attached projects, and if found, - * makes sure any old detached projects for that file are removed. - * - * @param uri The uri of the project and file to find - * @return The attached project and file, or null if not found - */ - private ProjectAndFile findAttachedAndRemoveDetached(String uri) { - String path = LspAdapter.toPath(uri); - // We might be in a state where a file was added to a tracked project, - // but was opened before the project loaded. This would result in it - // being placed in a detachedProjects project. Removing it here is basically - // like removing it lazily, although it does feel a little hacky. - for (Project project : attachedProjects.values()) { - ProjectFile projectFile = project.getProjectFile(path); - if (projectFile != null) { - detachedProjects.remove(uri); - return new ProjectAndFile(uri, project, projectFile, false); - } + projects.remove(uri); } - - return null; - } - - void createDetachedProject(String uri, String text) { - Project project = ProjectLoader.loadDetached(uri, text); - detachedProjects.put(uri, project); } List tryInitProject(Path root) { @@ -201,8 +161,8 @@ List tryInitProject(Path root) { if (updatedProject.config().buildFiles().isEmpty()) { removeProjectAndResolveDetached(projectName); } else { - resolveDetachedProjects(attachedProjects.get(projectName), updatedProject); - attachedProjects.put(projectName, updatedProject); + resolveDetachedProjects(projects.get(projectName), updatedProject); + projects.put(projectName, updatedProject); } LOGGER.finest("Initialized project at " + root); @@ -215,7 +175,7 @@ List tryInitProject(Path root) { // if we find a smithy-build.json, etc. // If we overwrite an existing project with an empty one, we lose track of the state of tracked // files. Instead, we will just keep the original project before the reload failure. - attachedProjects.computeIfAbsent(projectName, ignored -> Project.empty(root)); + projects.computeIfAbsent(projectName, ignored -> Project.empty(root)); return loadResult.unwrapErr(); } @@ -240,8 +200,8 @@ void removeWorkspace(WorkspaceFolder folder) { // Have to do the removal separately, so we don't modify project.attachedProjects() // while iterating through it List projectsToRemove = new ArrayList<>(); - for (var entry : attachedProjects.entrySet()) { - if (entry.getValue().root().startsWith(workspaceRoot)) { + for (var entry : projects.entrySet()) { + if (entry.getValue().type() == Project.Type.NORMAL && entry.getValue().root().startsWith(workspaceRoot)) { projectsToRemove.add(entry.getKey()); } } @@ -251,8 +211,43 @@ void removeWorkspace(WorkspaceFolder folder) { } } + List applyFileEvents(List events) { + List errors = new ArrayList<>(); + + var changes = WorkspaceChanges.computeWorkspaceChanges(events, this); + + for (var entry : changes.byProject().entrySet()) { + String projectRoot = entry.getKey(); + ProjectChange projectChange = entry.getValue(); + + Project project = findProjectByRoot(projectRoot); + + if (!projectChange.changedBuildFileUris().isEmpty()) { + // Note: this will take care of removing projects when build files are + // deleted + errors.addAll(tryInitProject(project.root())); + } else { + Set createdUris = projectChange.createdSmithyFileUris(); + Set deletedUris = projectChange.deletedSmithyFileUris(); + + project.updateFiles(createdUris, deletedUris); + + // If any file was previously opened and created a detached project, remove them + for (String createdUri : createdUris) { + projects.remove(createdUri); + } + } + } + + for (var newProjectRoot : changes.newProjectRoots()) { + errors.addAll(tryInitProject(newProjectRoot)); + } + + return errors; + } + private void removeProjectAndResolveDetached(String projectName) { - Project removedProject = attachedProjects.remove(projectName); + Project removedProject = projects.remove(projectName); if (removedProject != null) { resolveDetachedProjects(removedProject, Project.empty(removedProject.root())); } @@ -269,7 +264,7 @@ private void resolveDetachedProjects(Project oldProject, Project updatedProject) addedPaths.removeAll(currentProjectSmithyPaths); for (String addedPath : addedPaths) { String addedUri = LspAdapter.toUri(addedPath); - detachedProjects.remove(addedUri); + projects.remove(addedUri); // Remove any detached projects } Set removedPaths = new HashSet<>(currentProjectSmithyPaths); @@ -279,10 +274,14 @@ private void resolveDetachedProjects(Project oldProject, Project updatedProject) // Only move to a detachedProjects project if the file is managed if (managedUris.contains(removedUri)) { Document removedDocument = oldProject.getDocument(removedUri); - // The copy here is technically unnecessary, if we make ModelAssembler support borrowed strings createDetachedProject(removedUri, removedDocument.copyText()); } } } } + + private void createDetachedProject(String uri, String text) { + Project project = ProjectLoader.loadDetached(uri, text); + projects.put(uri, project); + } } diff --git a/src/main/java/software/amazon/smithy/lsp/SmithyLanguageServer.java b/src/main/java/software/amazon/smithy/lsp/SmithyLanguageServer.java index d2565c5d..c9616d99 100644 --- a/src/main/java/software/amazon/smithy/lsp/SmithyLanguageServer.java +++ b/src/main/java/software/amazon/smithy/lsp/SmithyLanguageServer.java @@ -18,14 +18,12 @@ import static java.util.concurrent.CompletableFuture.completedFuture; import java.io.IOException; -import java.nio.file.Path; import java.util.ArrayList; import java.util.Collections; import java.util.List; import java.util.Objects; import java.util.Optional; import java.util.Properties; -import java.util.Set; import java.util.concurrent.CompletableFuture; import java.util.logging.Logger; import java.util.stream.Collectors; @@ -58,8 +56,6 @@ import org.eclipse.lsp4j.InitializedParams; import org.eclipse.lsp4j.Location; import org.eclipse.lsp4j.LocationLink; -import org.eclipse.lsp4j.MessageParams; -import org.eclipse.lsp4j.MessageType; import org.eclipse.lsp4j.ProgressParams; import org.eclipse.lsp4j.PublishDiagnosticsParams; import org.eclipse.lsp4j.Range; @@ -209,20 +205,15 @@ public CompletableFuture initialize(InitializeParams params) { return completedFuture(new InitializeResult(CAPABILITIES)); } - private void tryInitProject(Path root) { - List loadErrors = state.tryInitProject(root); - if (!loadErrors.isEmpty()) { - String baseMessage = "Failed to load Smithy project at " + root; - StringBuilder errorMessage = new StringBuilder(baseMessage).append(":"); - for (Exception error : loadErrors) { + private void reportProjectLoadErrors(List errors) { + if (!errors.isEmpty()) { + StringBuilder errorMessage = new StringBuilder("Failed to load Smithy projects").append(":"); + for (Exception error : errors) { errorMessage.append(System.lineSeparator()); errorMessage.append('\t'); errorMessage.append(error.getMessage()); } client.error(errorMessage.toString()); - - String showMessage = baseMessage + ". Check server logs to find out what went wrong."; - client.showMessage(new MessageParams(MessageType.Error, showMessage)); } } @@ -395,32 +386,10 @@ public CompletableFuture serverStatus() { @Override public void didChangeWatchedFiles(DidChangeWatchedFilesParams params) { LOGGER.finest("DidChangeWatchedFiles"); + // Smithy files were added or deleted to watched sources/imports (specified by smithy-build.json), // the smithy-build.json itself was changed, added, or deleted. - - WorkspaceChanges changes = WorkspaceChanges.computeWorkspaceChanges(params.getChanges(), state); - - changes.byProject().forEach((projectName, projectChange) -> { - Project project = state.findProjectByRoot(projectName); - - if (!projectChange.changedBuildFileUris().isEmpty()) { - client.info("Build files changed, reloading project"); - // TODO: Handle more granular updates to build files. - // Note: This will take care of removing projects when build files are deleted - tryInitProject(project.root()); - } else { - Set createdUris = projectChange.createdSmithyFileUris(); - Set deletedUris = projectChange.deletedSmithyFileUris(); - client.info("Project files changed, adding files " - + createdUris + " and removing files " + deletedUris); - - // We get this notification for watched files, which only includes project files, - // so we don't need to resolve detachedProjects projects. - project.updateFiles(createdUris, deletedUris); - } - }); - - changes.newProjectRoots().forEach(this::tryInitProject); + reportProjectLoadErrors(state.applyFileEvents(params.getChanges())); // TODO: Update watchers based on specific changes // Note: We don't update build file watchers here - only on workspace changes @@ -537,7 +506,7 @@ public void didSave(DidSaveTextDocumentParams params) { Project project = projectAndFile.project(); if (projectAndFile.file() instanceof BuildFile) { - tryInitProject(project.root()); + reportProjectLoadErrors(state.tryInitProject(project.root())); unregisterSmithyFileWatchers().thenRun(this::registerSmithyFileWatchers); sendFileDiagnosticsForManagedDocuments(); } else { diff --git a/src/main/java/software/amazon/smithy/lsp/diagnostics/SmithyDiagnostics.java b/src/main/java/software/amazon/smithy/lsp/diagnostics/SmithyDiagnostics.java index 54459b17..2edc2034 100644 --- a/src/main/java/software/amazon/smithy/lsp/diagnostics/SmithyDiagnostics.java +++ b/src/main/java/software/amazon/smithy/lsp/diagnostics/SmithyDiagnostics.java @@ -71,7 +71,7 @@ public static List getFileDiagnostics(ProjectAndFile projectAndFile, diagnostics.add(versionDiagnostic); } - if (projectAndFile.isDetached()) { + if (projectAndFile.project().type() == Project.Type.DETACHED) { diagnostics.add(detachedDiagnostic(smithyFile)); } diff --git a/src/main/java/software/amazon/smithy/lsp/project/ProjectAndFile.java b/src/main/java/software/amazon/smithy/lsp/project/ProjectAndFile.java index 0a79da85..7790acab 100644 --- a/src/main/java/software/amazon/smithy/lsp/project/ProjectAndFile.java +++ b/src/main/java/software/amazon/smithy/lsp/project/ProjectAndFile.java @@ -12,7 +12,6 @@ * @param uri The uri of the file * @param project The project, non-nullable * @param file The file within {@code project}, non-nullable - * @param isDetached Whether the project and file represent a detached project */ -public record ProjectAndFile(String uri, Project project, ProjectFile file, boolean isDetached) { +public record ProjectAndFile(String uri, Project project, ProjectFile file) { } From 0a3ab4db150a051476720472eeb25746db77b4d3 Mon Sep 17 00:00:00 2001 From: Miles Ziemer Date: Mon, 23 Dec 2024 11:37:30 -0500 Subject: [PATCH 3/9] Collapse Project data members into RebuildIndex --- .../amazon/smithy/lsp/project/Project.java | 217 ++++++++++++++---- .../smithy/lsp/project/ProjectLoader.java | 43 +--- .../project/SmithyFileDependenciesIndex.java | 127 ---------- 3 files changed, 174 insertions(+), 213 deletions(-) delete mode 100644 src/main/java/software/amazon/smithy/lsp/project/SmithyFileDependenciesIndex.java diff --git a/src/main/java/software/amazon/smithy/lsp/project/Project.java b/src/main/java/software/amazon/smithy/lsp/project/Project.java index acffce34..690353a5 100644 --- a/src/main/java/software/amazon/smithy/lsp/project/Project.java +++ b/src/main/java/software/amazon/smithy/lsp/project/Project.java @@ -6,6 +6,7 @@ package software.amazon.smithy.lsp.project; import java.nio.file.Path; +import java.util.ArrayList; import java.util.Collections; import java.util.HashMap; import java.util.HashSet; @@ -20,9 +21,11 @@ import software.amazon.smithy.model.Model; import software.amazon.smithy.model.SourceLocation; import software.amazon.smithy.model.loader.ModelAssembler; +import software.amazon.smithy.model.node.ArrayNode; import software.amazon.smithy.model.node.Node; import software.amazon.smithy.model.shapes.AbstractShapeBuilder; import software.amazon.smithy.model.shapes.Shape; +import software.amazon.smithy.model.shapes.ShapeId; import software.amazon.smithy.model.shapes.ToShapeId; import software.amazon.smithy.model.traits.Trait; import software.amazon.smithy.model.validation.ValidatedResult; @@ -34,38 +37,34 @@ */ public final class Project { private static final Logger LOGGER = Logger.getLogger(Project.class.getName()); + private final Path root; private final ProjectConfig config; private final List dependencies; private final Map smithyFiles; private final Supplier assemblerFactory; - private final Map> definedShapesByFile; private final Type type; private ValidatedResult modelResult; - // TODO: Move this into SmithyFileDependenciesIndex - private Map> perFileMetadata; - private SmithyFileDependenciesIndex smithyFileDependenciesIndex; + private RebuildIndex rebuildIndex; - Project(Path root, + Project( + Path root, ProjectConfig config, List dependencies, Map smithyFiles, Supplier assemblerFactory, - Map> definedShapesByFile, Type type, ValidatedResult modelResult, - Map> perFileMetadata, - SmithyFileDependenciesIndex smithyFileDependenciesIndex) { + RebuildIndex rebuildIndex + ) { this.root = root; this.config = config; this.dependencies = dependencies; this.smithyFiles = smithyFiles; this.assemblerFactory = assemblerFactory; - this.definedShapesByFile = definedShapesByFile; this.type = type; this.modelResult = modelResult; - this.perFileMetadata = perFileMetadata; - this.smithyFileDependenciesIndex = smithyFileDependenciesIndex; + this.rebuildIndex = rebuildIndex; } public enum Type { @@ -86,11 +85,9 @@ public static Project empty(Path root) { List.of(), new HashMap<>(), Model::assembler, - new HashMap<>(), Type.EMPTY, ValidatedResult.empty(), - new HashMap<>(), - new SmithyFileDependenciesIndex()); + new RebuildIndex()); } /** @@ -147,7 +144,7 @@ public Map smithyFiles() { * @return A map of paths to the set of shape ids defined in the file at that path. */ public Map> definedShapesByFile() { - return this.definedShapesByFile; + return this.rebuildIndex.filesToDefinedShapes(); } public Type type() { @@ -301,7 +298,15 @@ public void updateFiles(Set addUris, Set removeUris, Set } for (String uri : addUris) { - assembler.addImport(LspAdapter.toPath(uri)); + String path = LspAdapter.toPath(uri); + String text = IoUtils.readUtf8File(path); + + // TODO: Inefficient ? + Document document = Document.of(text); + SmithyFile smithyFile = SmithyFile.create(path, document); + this.smithyFiles.put(path, smithyFile); + + assembler.addUnparsedModel(path, text); } if (!validate) { @@ -309,25 +314,7 @@ public void updateFiles(Set addUris, Set removeUris, Set } this.modelResult = assembler.assemble(); - this.perFileMetadata = ProjectLoader.computePerFileMetadata(this.modelResult); - this.smithyFileDependenciesIndex = SmithyFileDependenciesIndex.compute(this.modelResult); - - for (String visitedPath : visited) { - if (!removedPaths.contains(visitedPath)) { - Set currentShapes = definedShapesByFile.getOrDefault(visitedPath, Set.of()); - this.definedShapesByFile.put(visitedPath, getFileShapes(visitedPath, currentShapes)); - } else { - this.definedShapesByFile.remove(visitedPath); - } - } - - for (String uri : addUris) { - String path = LspAdapter.toPath(uri); - Document document = Document.of(IoUtils.readUtf8File(path)); - SmithyFile smithyFile = SmithyFile.create(path, document); - this.smithyFiles.put(path, smithyFile); - this.definedShapesByFile.put(path, getFileShapes(path, Set.of())); - } + this.rebuildIndex = this.rebuildIndex.recompute(this.modelResult); } // This mainly exists to explain why we remove the metadata @@ -351,7 +338,7 @@ private void removeFileForReload( visited.add(path); - for (ToShapeId toShapeId : definedShapesByFile.getOrDefault(path, Set.of())) { + for (ToShapeId toShapeId : this.rebuildIndex.getDefinedShapes(path)) { builder.removeShape(toShapeId.toShapeId()); // This shape may have traits applied to it in other files, @@ -359,12 +346,12 @@ private void removeFileForReload( // those traits. // This shape's dependencies files will be removed and re-loaded - smithyFileDependenciesIndex.getDependenciesFiles(toShapeId).forEach((depPath) -> + this.rebuildIndex.getDependenciesFiles(toShapeId).forEach((depPath) -> removeFileForReload(assembler, builder, depPath, visited)); // Traits applied in other files are re-added to the assembler so if/when the shape // is reloaded, it will have those traits - smithyFileDependenciesIndex.getTraitsAppliedInOtherFiles(toShapeId).forEach((trait) -> + this.rebuildIndex.getTraitsAppliedInOtherFiles(toShapeId).forEach((trait) -> assembler.addTrait(toShapeId.toShapeId(), trait)); } } @@ -379,9 +366,9 @@ private void removeDependentsForReload( // the file would be fine because it would ignore the duplicated trait application coming from the same // source location. But if the apply statement is changed/removed, the old trait isn't removed, so we // could get a duplicate application, or a merged array application. - smithyFileDependenciesIndex.getDependentFiles(path).forEach((depPath) -> + this.rebuildIndex.getDependentFiles(path).forEach((depPath) -> removeFileForReload(assembler, builder, depPath, visited)); - smithyFileDependenciesIndex.getAppliedTraitsInFile(path).forEach((shapeId, traits) -> { + this.rebuildIndex.getAppliedTraitsInFile(path).forEach((shapeId, traits) -> { Shape shape = builder.getCurrentShapes().get(shapeId); if (shape != null) { builder.removeShape(shapeId); @@ -395,18 +382,154 @@ private void removeDependentsForReload( } private void addRemainingMetadataForReload(Model.Builder builder, Set filesToSkip) { - for (Map.Entry> e : this.perFileMetadata.entrySet()) { + for (Map.Entry> e : this.rebuildIndex.filesToMetadata().entrySet()) { if (!filesToSkip.contains(e.getKey())) { e.getValue().forEach(builder::putMetadataProperty); } } } - private Set getFileShapes(String path, Set orDefault) { - return this.modelResult.getResult() - .map(model -> model.shapes() - .filter(shape -> shape.getSourceLocation().getFilename().equals(path)) - .collect(Collectors.toSet())) - .orElse(orDefault); + /** + * An index that caches rebuild dependency relationships between Smithy files, + * shapes, traits, and metadata. + * + *

This is specifically for the following scenarios: + *

+ *
A file applies traits to shapes in other files
+ *
If that file changes, the applied traits need to be removed before the + * file is reloaded, so there aren't duplicate traits.
+ *
A file has shapes with traits applied in other files
+ *
If that file changes, the traits need to be re-applied when the model is + * re-assembled, so they aren't lost.
+ *
Either 1 or 2, but specifically with list traits
+ *
List traits are merged via + * trait conflict resolution . For these traits, all files that contain + * parts of the list trait must be fully reloaded, since we can only remove + * the whole trait, not parts of it.
+ *
A file has metadata
+ *
Metadata for a specific file has to be removed before reloading that + * file, but since array nodes are merged, we also need to keep track of + * other files' metadata that may also need to be reloaded.
+ *
+ */ + record RebuildIndex( + Map> filesToDependentFiles, + Map> shapeIdsToDependenciesFiles, + Map>> filesToTraitsTheyApply, + Map> shapesToAppliedTraitsInOtherFiles, + Map> filesToMetadata, + Map> filesToDefinedShapes + ) { + private RebuildIndex() { + this( + new HashMap<>(0), + new HashMap<>(0), + new HashMap<>(0), + new HashMap<>(0), + new HashMap<>(0), + new HashMap<>(0) + ); + } + + static RebuildIndex create(ValidatedResult modelResult) { + return new RebuildIndex().recompute(modelResult); + } + + Set getDependentFiles(String path) { + return filesToDependentFiles.getOrDefault(path, Collections.emptySet()); + } + + Set getDependenciesFiles(ToShapeId toShapeId) { + return shapeIdsToDependenciesFiles.getOrDefault(toShapeId.toShapeId(), Collections.emptySet()); + } + + Map> getAppliedTraitsInFile(String path) { + return filesToTraitsTheyApply.getOrDefault(path, Collections.emptyMap()); + } + + List getTraitsAppliedInOtherFiles(ToShapeId toShapeId) { + return shapesToAppliedTraitsInOtherFiles.getOrDefault(toShapeId.toShapeId(), Collections.emptyList()); + } + + Set getDefinedShapes(String path) { + return filesToDefinedShapes.getOrDefault(path, Collections.emptySet()); + } + + RebuildIndex recompute(ValidatedResult modelResult) { + var newIndex = new RebuildIndex( + new HashMap<>(filesToDependentFiles.size()), + new HashMap<>(shapeIdsToDependenciesFiles.size()), + new HashMap<>(filesToTraitsTheyApply.size()), + new HashMap<>(shapesToAppliedTraitsInOtherFiles.size()), + new HashMap<>(filesToMetadata.size()), + new HashMap<>(filesToDefinedShapes.size()) + ); + + if (modelResult.getResult().isEmpty()) { + return newIndex; + } + + Model model = modelResult.getResult().get(); + + // This is gross, but necessary to deal with the way that array metadata gets merged. + // When we try to reload a single file, we need to make sure we remove the metadata for + // that file. But if there's array metadata, a single key contains merged elements from + // other files. This splits up the metadata by source file, creating an artificial array + // node for elements that are merged. + for (var metadataEntry : model.getMetadata().entrySet()) { + if (metadataEntry.getValue().isArrayNode()) { + Map arrayByFile = new HashMap<>(); + for (Node node : metadataEntry.getValue().expectArrayNode()) { + String filename = node.getSourceLocation().getFilename(); + arrayByFile.computeIfAbsent(filename, (f) -> ArrayNode.builder()).withValue(node); + } + for (var arrayByFileEntry : arrayByFile.entrySet()) { + newIndex.filesToMetadata.computeIfAbsent(arrayByFileEntry.getKey(), (f) -> new HashMap<>()) + .put(metadataEntry.getKey(), arrayByFileEntry.getValue().build()); + } + } else { + String filename = metadataEntry.getValue().getSourceLocation().getFilename(); + newIndex.filesToMetadata.computeIfAbsent(filename, (f) -> new HashMap<>()) + .put(metadataEntry.getKey(), metadataEntry.getValue()); + } + } + + for (Shape shape : model.toSet()) { + String shapeSourceFilename = shape.getSourceLocation().getFilename(); + newIndex.filesToDefinedShapes.computeIfAbsent(shapeSourceFilename, (f) -> new HashSet<>()) + .add(shape); + + for (Trait traitApplication : shape.getAllTraits().values()) { + // We only care about trait applications in the source files + if (traitApplication.isSynthetic()) { + continue; + } + + Node traitNode = traitApplication.toNode(); + if (traitNode.isArrayNode()) { + for (Node element : traitNode.expectArrayNode()) { + String elementSourceFilename = element.getSourceLocation().getFilename(); + if (!elementSourceFilename.equals(shapeSourceFilename)) { + newIndex.filesToDependentFiles.computeIfAbsent(elementSourceFilename, (f) -> new HashSet<>()) + .add(shapeSourceFilename); + newIndex.shapeIdsToDependenciesFiles.computeIfAbsent(shape.getId(), (i) -> new HashSet<>()) + .add(elementSourceFilename); + } + } + } else { + String traitSourceFilename = traitApplication.getSourceLocation().getFilename(); + if (!traitSourceFilename.equals(shapeSourceFilename)) { + newIndex.shapesToAppliedTraitsInOtherFiles.computeIfAbsent(shape.getId(), (i) -> new ArrayList<>()) + .add(traitApplication); + newIndex.filesToTraitsTheyApply.computeIfAbsent(traitSourceFilename, (f) -> new HashMap<>()) + .computeIfAbsent(shape.getId(), (i) -> new ArrayList<>()) + .add(traitApplication); + } + } + } + } + + return newIndex; + } } } diff --git a/src/main/java/software/amazon/smithy/lsp/project/ProjectLoader.java b/src/main/java/software/amazon/smithy/lsp/project/ProjectLoader.java index 6c39cead..18056dc5 100644 --- a/src/main/java/software/amazon/smithy/lsp/project/ProjectLoader.java +++ b/src/main/java/software/amazon/smithy/lsp/project/ProjectLoader.java @@ -31,8 +31,6 @@ import software.amazon.smithy.model.Model; import software.amazon.smithy.model.loader.ModelAssembler; import software.amazon.smithy.model.loader.ModelDiscovery; -import software.amazon.smithy.model.node.ArrayNode; -import software.amazon.smithy.model.node.Node; import software.amazon.smithy.model.shapes.ToShapeId; import software.amazon.smithy.model.validation.ValidatedResult; import software.amazon.smithy.utils.IoUtils; @@ -41,6 +39,8 @@ * Loads {@link Project}s. * * TODO: There's a lot of duplicated logic and redundant code here to refactor. + * + * TODO: Inefficient creation of smithy files */ public final class ProjectLoader { private static final Logger LOGGER = Logger.getLogger(ProjectLoader.class.getName()); @@ -99,11 +99,9 @@ public static Project loadDetached(String uri, String text) { List.of(), smithyFiles, assemblerFactory, - definedShapesByFile, Project.Type.DETACHED, modelResult, - computePerFileMetadata(modelResult), - new SmithyFileDependenciesIndex()); + Project.RebuildIndex.create(modelResult)); } /** @@ -186,11 +184,9 @@ public static Result> load(Path root, ServerState state dependencies, smithyFiles, assemblerFactory, - definedShapesByFile, Project.Type.NORMAL, modelResult, - computePerFileMetadata(modelResult), - SmithyFileDependenciesIndex.compute(modelResult))); + Project.RebuildIndex.create(modelResult))); } private static Result, Exception> loadModel( @@ -251,37 +247,6 @@ private static Map createSmithyFiles( return smithyFiles; } - // This is gross, but necessary to deal with the way that array metadata gets merged. - // When we try to reload a single file, we need to make sure we remove the metadata for - // that file. But if there's array metadata, a single key contains merged elements from - // other files. This splits up the metadata by source file, creating an artificial array - // node for elements that are merged. - // - // This definitely has the potential to cause a performance hit if there's a huge amount - // of metadata, since we are recomputing this on every change. - static Map> computePerFileMetadata(ValidatedResult modelResult) { - Map metadata = modelResult.getResult().map(Model::getMetadata).orElse(new HashMap<>(0)); - Map> perFileMetadata = new HashMap<>(); - for (Map.Entry entry : metadata.entrySet()) { - if (entry.getValue().isArrayNode()) { - Map arrayByFile = new HashMap<>(); - for (Node node : entry.getValue().expectArrayNode()) { - String filename = node.getSourceLocation().getFilename(); - arrayByFile.computeIfAbsent(filename, (f) -> ArrayNode.builder()).withValue(node); - } - for (Map.Entry arrayByFileEntry : arrayByFile.entrySet()) { - perFileMetadata.computeIfAbsent(arrayByFileEntry.getKey(), (f) -> new HashMap<>()) - .put(entry.getKey(), arrayByFileEntry.getValue().build()); - } - } else { - String filename = entry.getValue().getSourceLocation().getFilename(); - perFileMetadata.computeIfAbsent(filename, (f) -> new HashMap<>()) - .put(entry.getKey(), entry.getValue()); - } - } - return perFileMetadata; - } - private static Supplier createModelAssemblerFactory(List dependencies) throws MalformedURLException { // We don't want the model to be broken when there are unknown traits, diff --git a/src/main/java/software/amazon/smithy/lsp/project/SmithyFileDependenciesIndex.java b/src/main/java/software/amazon/smithy/lsp/project/SmithyFileDependenciesIndex.java deleted file mode 100644 index f6652c16..00000000 --- a/src/main/java/software/amazon/smithy/lsp/project/SmithyFileDependenciesIndex.java +++ /dev/null @@ -1,127 +0,0 @@ -/* - * Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. - * SPDX-License-Identifier: Apache-2.0 - */ - -package software.amazon.smithy.lsp.project; - -import java.util.ArrayList; -import java.util.Collections; -import java.util.HashMap; -import java.util.HashSet; -import java.util.List; -import java.util.Map; -import java.util.Set; -import software.amazon.smithy.model.Model; -import software.amazon.smithy.model.node.Node; -import software.amazon.smithy.model.shapes.Shape; -import software.amazon.smithy.model.shapes.ShapeId; -import software.amazon.smithy.model.shapes.ToShapeId; -import software.amazon.smithy.model.traits.Trait; -import software.amazon.smithy.model.validation.ValidatedResult; - -/** - * An index that caches rebuild dependency relationships between Smithy files, - * shapes, and traits. - * - *

This is specifically for the following scenarios: - *

- *
A file applies traits to shapes in other files
- *
If that file changes, the applied traits need to be removed before the - * file is reloaded, so there aren't duplicate traits.
- *
A file has shapes with traits applied in other files
- *
If that file changes, the traits need to be re-applied when the model is - * re-assembled, so they aren't lost.
- *
Either 1 or 2, but specifically with list traits
- *
List traits are merged via - * trait conflict resolution . For these traits, all files that contain - * parts of the list trait must be fully reloaded, since we can only remove - * the whole trait, not parts of it.
- *
- */ -final class SmithyFileDependenciesIndex { - private final Map> filesToDependentFiles; - private final Map> shapeIdsToDependenciesFiles; - private final Map>> filesToTraitsTheyApply; - private final Map> shapesToAppliedTraitsInOtherFiles; - - SmithyFileDependenciesIndex() { - this.filesToDependentFiles = new HashMap<>(0); - this.shapeIdsToDependenciesFiles = new HashMap<>(0); - this.filesToTraitsTheyApply = new HashMap<>(0); - this.shapesToAppliedTraitsInOtherFiles = new HashMap<>(0); - } - - private SmithyFileDependenciesIndex( - Map> filesToDependentFiles, - Map> shapeIdsToDependenciesFiles, - Map>> filesToTraitsTheyApply, - Map> shapesToAppliedTraitsInOtherFiles - ) { - this.filesToDependentFiles = filesToDependentFiles; - this.shapeIdsToDependenciesFiles = shapeIdsToDependenciesFiles; - this.filesToTraitsTheyApply = filesToTraitsTheyApply; - this.shapesToAppliedTraitsInOtherFiles = shapesToAppliedTraitsInOtherFiles; - } - - Set getDependentFiles(String path) { - return filesToDependentFiles.getOrDefault(path, Collections.emptySet()); - } - - Set getDependenciesFiles(ToShapeId toShapeId) { - return shapeIdsToDependenciesFiles.getOrDefault(toShapeId.toShapeId(), Collections.emptySet()); - } - - Map> getAppliedTraitsInFile(String path) { - return filesToTraitsTheyApply.getOrDefault(path, Collections.emptyMap()); - } - - List getTraitsAppliedInOtherFiles(ToShapeId toShapeId) { - return shapesToAppliedTraitsInOtherFiles.getOrDefault(toShapeId.toShapeId(), Collections.emptyList()); - } - - // TODO: Make this take care of metadata too - static SmithyFileDependenciesIndex compute(ValidatedResult modelResult) { - if (modelResult.getResult().isEmpty()) { - return new SmithyFileDependenciesIndex(); - } - - SmithyFileDependenciesIndex index = new SmithyFileDependenciesIndex( - new HashMap<>(), new HashMap<>(), new HashMap<>(), new HashMap<>()); - - Model model = modelResult.getResult().get(); - for (Shape shape : model.toSet()) { - String shapeSourceFilename = shape.getSourceLocation().getFilename(); - for (Trait traitApplication : shape.getAllTraits().values()) { - // We only care about trait applications in the source files - if (traitApplication.isSynthetic()) { - continue; - } - - Node traitNode = traitApplication.toNode(); - if (traitNode.isArrayNode()) { - for (Node element : traitNode.expectArrayNode()) { - String elementSourceFilename = element.getSourceLocation().getFilename(); - if (!elementSourceFilename.equals(shapeSourceFilename)) { - index.filesToDependentFiles.computeIfAbsent(elementSourceFilename, (k) -> new HashSet<>()) - .add(shapeSourceFilename); - index.shapeIdsToDependenciesFiles.computeIfAbsent(shape.getId(), (k) -> new HashSet<>()) - .add(elementSourceFilename); - } - } - } else { - String traitSourceFilename = traitApplication.getSourceLocation().getFilename(); - if (!traitSourceFilename.equals(shapeSourceFilename)) { - index.shapesToAppliedTraitsInOtherFiles.computeIfAbsent(shape.getId(), (k) -> new ArrayList<>()) - .add(traitApplication); - index.filesToTraitsTheyApply.computeIfAbsent(traitSourceFilename, (k) -> new HashMap<>()) - .computeIfAbsent(shape.getId(), (k) -> new ArrayList<>()) - .add(traitApplication); - } - } - } - } - - return index; - } -} From 9e8b1a471d4ff428a5b9d28c5230a544326b779c Mon Sep 17 00:00:00 2001 From: Miles Ziemer Date: Mon, 30 Dec 2024 11:57:44 -0500 Subject: [PATCH 4/9] Dont read from disk multiple times When loading a project, SmithyFiles were being read from disk multiple times. This commit refactors ProjectLoader to only read from disk once, and also cleans up some stuff. --- .../software/amazon/smithy/lsp/FileCache.java | 40 +++ .../smithy/lsp/project/ProjectLoader.java | 251 +++++++++--------- .../smithy/lsp/project/ProjectTest.java | 53 ++-- 3 files changed, 199 insertions(+), 145 deletions(-) create mode 100644 src/main/java/software/amazon/smithy/lsp/FileCache.java diff --git a/src/main/java/software/amazon/smithy/lsp/FileCache.java b/src/main/java/software/amazon/smithy/lsp/FileCache.java new file mode 100644 index 00000000..a4cea1d3 --- /dev/null +++ b/src/main/java/software/amazon/smithy/lsp/FileCache.java @@ -0,0 +1,40 @@ +/* + * Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. + * SPDX-License-Identifier: Apache-2.0 + */ + +package software.amazon.smithy.lsp; + +import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.ConcurrentMap; +import software.amazon.smithy.lsp.project.ProjectFile; + +/** + * Todo: Implement a data structure that can: + * - keep track of opened files + * - handle creating the right type of ProjectFile + * - close files + * - cache the text of these files to be used when loading models + * + * Notes: + * - One idea I have is to make it cache all the project files on load. + * Right now, ProjectLoader reads from disk every time. This isn't too + * bad, because project changes aren't _that_ frequent, but we end up + * reading the files _again_ to create ProjectFiles for everything that + * was loaded. + * - My concern here is that when there are changes on the filesystem, + * we need to make sure we always stay in sync. Right now, we avoid + * this by just reading from disk every time we do a full project + * reload. + * - Another idea is to just cache managed files, and then just create + * ProjectFiles as they're being added to the ModelAssembler. + * - We still have to handle files in jars though, and I'm _not_ + * re-implementing ModelDiscovery. + * - Yet another idea would be to lazily read these files from disk. This + * avoids most implementation complications. + * - But there's a design problem - when we want to support refactoring, + * we will need to read everything from disk + parse it anyways. + */ +public final class FileCache { + private final ConcurrentMap managedProjectFiles = new ConcurrentHashMap<>(); +} diff --git a/src/main/java/software/amazon/smithy/lsp/project/ProjectLoader.java b/src/main/java/software/amazon/smithy/lsp/project/ProjectLoader.java index 18056dc5..f17c2db7 100644 --- a/src/main/java/software/amazon/smithy/lsp/project/ProjectLoader.java +++ b/src/main/java/software/amazon/smithy/lsp/project/ProjectLoader.java @@ -19,10 +19,8 @@ import java.util.List; import java.util.Map; import java.util.Set; -import java.util.function.Function; import java.util.function.Supplier; import java.util.logging.Logger; -import java.util.stream.Collectors; import java.util.stream.Stream; import software.amazon.smithy.lsp.ServerState; import software.amazon.smithy.lsp.document.Document; @@ -31,16 +29,14 @@ import software.amazon.smithy.model.Model; import software.amazon.smithy.model.loader.ModelAssembler; import software.amazon.smithy.model.loader.ModelDiscovery; -import software.amazon.smithy.model.shapes.ToShapeId; import software.amazon.smithy.model.validation.ValidatedResult; import software.amazon.smithy.utils.IoUtils; +import software.amazon.smithy.utils.TriConsumer; /** * Loads {@link Project}s. * * TODO: There's a lot of duplicated logic and redundant code here to refactor. - * - * TODO: Inefficient creation of smithy files */ public final class ProjectLoader { private static final Logger LOGGER = Logger.getLogger(ProjectLoader.class.getName()); @@ -61,47 +57,40 @@ private ProjectLoader() { */ public static Project loadDetached(String uri, String text) { LOGGER.info("Loading detachedProjects project at " + uri); + String asPath = LspAdapter.toPath(uri); - Supplier assemblerFactory; + Path path = Paths.get(asPath); + List allSmithyFilePaths = List.of(path); + + Document document = Document.of(text); + ManagedFiles managedFiles = (fileUri) -> { + if (uri.equals(fileUri)) { + return document; + } + return null; + }; + + List dependencies = List.of(); + + LoadModelResult result; try { - assemblerFactory = createModelAssemblerFactory(List.of()); - } catch (MalformedURLException e) { + result = doLoad(managedFiles, dependencies, allSmithyFilePaths); + } catch (Exception e) { + // TODO: Clean up this comment // Note: This can't happen because we have no dependencies to turn into URLs throw new RuntimeException(e); } - ValidatedResult modelResult = assemblerFactory.get() - .addUnparsedModel(asPath, text) - .assemble(); - - Path path = Paths.get(asPath); - List sources = Collections.singletonList(path); - - var definedShapesByFile = computeDefinedShapesByFile(sources, modelResult); - var smithyFiles = createSmithyFiles(definedShapesByFile, (filePath) -> { - // NOTE: isSmithyJarFile and isJarFile typically take in a URI (filePath is a path), but - // the model stores jar paths as URIs - if (LspAdapter.isSmithyJarFile(filePath) || LspAdapter.isJarFile(filePath)) { - return Document.of(IoUtils.readUtf8Url(LspAdapter.jarUrl(filePath))); - } else if (filePath.equals(asPath)) { - return Document.of(text); - } else { - // TODO: Make generic 'please file a bug report' exception - throw new IllegalStateException( - "Attempted to load an unknown source file (" - + filePath + ") in detachedProjects project at " - + asPath + ". This is a bug in the language server."); - } - }); - - return new Project(path, - ProjectConfig.builder().sources(List.of(asPath)).build(), - List.of(), - smithyFiles, - assemblerFactory, + return new Project( + path, + ProjectConfig.empty(), + dependencies, + result.smithyFiles(), + result.assemblerFactory(), Project.Type.DETACHED, - modelResult, - Project.RebuildIndex.create(modelResult)); + result.modelResult(), + result.rebuildIndex() + ); } /** @@ -134,117 +123,136 @@ public static Result> load(Path root, ServerState state List dependencies = resolveResult.unwrap(); - // The model assembler factory is used to get assemblers that already have the correct - // dependencies resolved for future loads - Supplier assemblerFactory; + // Note: The model assembler can handle loading all smithy files in a directory, so there's some potential + // here for inconsistent behavior. + List allSmithyFilePaths = collectAllSmithyPaths(root, config.sources(), config.imports()); + + LoadModelResult result; try { - assemblerFactory = createModelAssemblerFactory(dependencies); - } catch (MalformedURLException e) { - return Result.err(List.of(e)); + result = doLoad(state::getManagedDocument, dependencies, allSmithyFilePaths); + } catch (Exception e) { + return Result.err(Collections.singletonList(e)); } - ModelAssembler assembler = assemblerFactory.get(); + return Result.ok(new Project( + root, + config, + dependencies, + result.smithyFiles(), + result.assemblerFactory(), + Project.Type.NORMAL, + result.modelResult(), + result.rebuildIndex() + )); + } - // Note: The model assembler can handle loading all smithy files in a directory, so there's some potential - // here for inconsistent behavior. - List allSmithyFilePaths = collectAllSmithyPaths(root, config.sources(), config.imports()); + private record LoadModelResult( + Supplier assemblerFactory, + ValidatedResult modelResult, + Map smithyFiles, + Project.RebuildIndex rebuildIndex + ) { + } + + private interface ManagedFiles { + Document getManagedDocument(String uri); + } + + private static LoadModelResult doLoad( + ManagedFiles managedFiles, + List dependencies, + List allSmithyFilePaths + ) throws Exception { + // The model assembler factory is used to get assemblers that already have the correct + // dependencies resolved for future loads + Supplier assemblerFactory = createModelAssemblerFactory(dependencies); + + Map smithyFiles = new HashMap<>(allSmithyFilePaths.size()); - Result, Exception> loadModelResult = loadModel(state, allSmithyFilePaths, assembler); // TODO: Assembler can fail if a file is not found. We can be more intelligent about // handling this case to allow partially loading the project, but we will need to // collect and report the errors somehow. For now, using collectAllSmithyPaths skips // any files that don't exist, so we're essentially side-stepping the issue by // coincidence. - if (loadModelResult.isErr()) { - return Result.err(Collections.singletonList(loadModelResult.unwrapErr())); - } + ModelAssembler assembler = assemblerFactory.get(); + ValidatedResult modelResult = loadModel(managedFiles, allSmithyFilePaths, assembler, smithyFiles); - ValidatedResult modelResult = loadModelResult.unwrap(); - var definedShapesByFile = computeDefinedShapesByFile(allSmithyFilePaths, modelResult); - var smithyFiles = createSmithyFiles(definedShapesByFile, (filePath) -> { - // NOTE: isSmithyJarFile and isJarFile typically take in a URI (filePath is a path), but - // the model stores jar paths as URIs - if (LspAdapter.isSmithyJarFile(filePath) || LspAdapter.isJarFile(filePath)) { - // Technically this can throw - return Document.of(IoUtils.readUtf8Url(LspAdapter.jarUrl(filePath))); - } - // TODO: We recompute uri from path and vice-versa very frequently, - // maybe we can cache it. - String uri = LspAdapter.toUri(filePath); - Document managed = state.getManagedDocument(uri); - if (managed != null) { - return managed; - } - // There may be a more efficient way of reading this - return Document.of(IoUtils.readUtf8File(filePath)); - }); + Project.RebuildIndex rebuildIndex = Project.RebuildIndex.create(modelResult); + addExtraSmithyFiles(managedFiles, rebuildIndex.filesToDefinedShapes().keySet(), smithyFiles); - return Result.ok(new Project(root, - config, - dependencies, - smithyFiles, + return new LoadModelResult( assemblerFactory, - Project.Type.NORMAL, modelResult, - Project.RebuildIndex.create(modelResult))); + smithyFiles, + rebuildIndex + ); } - private static Result, Exception> loadModel( - ServerState state, - List models, - ModelAssembler assembler + private static ValidatedResult loadModel( + ManagedFiles managedFiles, + List allSmithyFilePaths, + ModelAssembler assembler, + Map smithyFiles ) { - try { - for (Path path : models) { - Document managed = state.getManagedDocument(path); - if (managed != null) { - assembler.addUnparsedModel(path.toString(), managed.copyText()); - } else { - assembler.addImport(path); - } - } - - return Result.ok(assembler.assemble()); - } catch (Exception e) { - return Result.err(e); + TriConsumer consumer = (filePath, text, document) -> { + assembler.addUnparsedModel(filePath, text.toString()); + smithyFiles.put(filePath, SmithyFile.create(filePath, document)); + }; + + for (Path path : allSmithyFilePaths) { + String pathString = path.toString(); + findOrReadDocument(managedFiles, pathString, consumer); } - } - static Result> load(Path root) { - return load(root, new ServerState()); + return assembler.assemble(); } - private static Map> computeDefinedShapesByFile( - List allSmithyFilePaths, - ValidatedResult modelResult + private static void addExtraSmithyFiles( + ManagedFiles managedFiles, + Set loadedSmithyFilePaths, + Map smithyFiles ) { - Map> definedShapesByFile = modelResult.getResult().map(Model::shapes) - .orElseGet(Stream::empty) - .collect(Collectors.groupingByConcurrent( - shape -> shape.getSourceLocation().getFilename(), Collectors.toSet())); - - // There may be smithy files part of the project that aren't part of the model, e.g. empty files - for (Path smithyFilePath : allSmithyFilePaths) { - String pathString = smithyFilePath.toString(); - definedShapesByFile.putIfAbsent(pathString, Set.of()); + TriConsumer consumer = (filePath, text, document) -> { + SmithyFile smithyFile = SmithyFile.create(filePath, document); + smithyFiles.put(filePath, smithyFile); + }; + + for (String loadedPath : loadedSmithyFilePaths) { + if (!smithyFiles.containsKey(loadedPath)) { + findOrReadDocument(managedFiles, loadedPath, consumer); + } } - - return definedShapesByFile; } - private static Map createSmithyFiles( - Map> definedShapesByFile, - Function documentProvider + private static void findOrReadDocument( + ManagedFiles managedFiles, + String filePath, + TriConsumer consumer ) { - Map smithyFiles = new HashMap<>(definedShapesByFile.size()); + // NOTE: isSmithyJarFile and isJarFile typically take in a URI (filePath is a path), but + // the model stores jar paths as URIs + if (LspAdapter.isSmithyJarFile(filePath) || LspAdapter.isJarFile(filePath)) { + // Technically this can throw + String text = IoUtils.readUtf8Url(LspAdapter.jarUrl(filePath)); + Document document = Document.of(text); + consumer.accept(filePath, text, document); + return; + } - for (String path : definedShapesByFile.keySet()) { - Document document = documentProvider.apply(path); - SmithyFile smithyFile = SmithyFile.create(path, document); - smithyFiles.put(path, smithyFile); + // TODO: We recompute uri from path and vice-versa very frequently, + // maybe we can cache it. + String uri = LspAdapter.toUri(filePath); + Document managed = managedFiles.getManagedDocument(uri); + if (managed != null) { + CharSequence text = managed.borrowText(); + consumer.accept(filePath, text, managed); + return; } - return smithyFiles; + // There may be a more efficient way of reading this + String text = IoUtils.readUtf8File(filePath); + Document document = Document.of(text); + consumer.accept(filePath, text, document); } private static Supplier createModelAssemblerFactory(List dependencies) @@ -273,6 +281,7 @@ private static URLClassLoader createDependenciesClassLoader(List dependenc return new URLClassLoader(urls); } + // TODO: Can there be duplicate paths in this list? If there are, we may end up reading from disk multiple times // sources and imports can contain directories or files, relative or absolute private static List collectAllSmithyPaths(Path root, List sources, List imports) { List paths = new ArrayList<>(); diff --git a/src/test/java/software/amazon/smithy/lsp/project/ProjectTest.java b/src/test/java/software/amazon/smithy/lsp/project/ProjectTest.java index a7c983f3..47400209 100644 --- a/src/test/java/software/amazon/smithy/lsp/project/ProjectTest.java +++ b/src/test/java/software/amazon/smithy/lsp/project/ProjectTest.java @@ -29,6 +29,7 @@ import java.util.logging.Logger; import java.util.stream.Collectors; import org.junit.jupiter.api.Test; +import software.amazon.smithy.lsp.ServerState; import software.amazon.smithy.lsp.TestWorkspace; import software.amazon.smithy.lsp.document.Document; import software.amazon.smithy.lsp.protocol.LspAdapter; @@ -47,7 +48,7 @@ public class ProjectTest { @Test public void loadsFlatProject() { Path root = toPath(getClass().getResource("flat")); - Project project = ProjectLoader.load(root).unwrap(); + Project project = load(root).unwrap(); assertThat(project.root(), equalTo(root)); assertThat(project.sources(), hasItem(root.resolve("main.smithy"))); @@ -60,7 +61,7 @@ public void loadsFlatProject() { @Test public void loadsProjectWithMavenDep() { Path root = toPath(getClass().getResource("maven-dep")); - Project project = ProjectLoader.load(root).unwrap(); + Project project = load(root).unwrap(); assertThat(project.root(), equalTo(root)); assertThat(project.sources(), hasItem(root.resolve("main.smithy"))); @@ -73,7 +74,7 @@ public void loadsProjectWithMavenDep() { @Test public void loadsProjectWithSubdir() { Path root = toPath(getClass().getResource("subdirs")); - Project project = ProjectLoader.load(root).unwrap(); + Project project = load(root).unwrap(); assertThat(project.root(), equalTo(root)); assertThat(project.sources(), hasItems( @@ -93,7 +94,7 @@ public void loadsProjectWithSubdir() { @Test public void loadsModelWithUnknownTrait() { Path root = toPath(getClass().getResource("unknown-trait")); - Project project = ProjectLoader.load(root).unwrap(); + Project project = load(root).unwrap(); assertThat(project.root(), equalTo(root)); assertThat(project.sources(), hasItem(root.resolve("main.smithy"))); @@ -110,7 +111,7 @@ public void loadsModelWithUnknownTrait() { @Test public void loadsWhenModelHasInvalidSyntax() { Path root = toPath(getClass().getResource("invalid-syntax")); - Project project = ProjectLoader.load(root).unwrap(); + Project project = load(root).unwrap(); assertThat(project.root(), equalTo(root)); assertThat(project.sources(), hasItem(root.resolve("main.smithy"))); @@ -139,7 +140,7 @@ public void loadsWhenModelHasInvalidSyntax() { @Test public void loadsProjectWithMultipleNamespaces() { Path root = toPath(getClass().getResource("multiple-namespaces")); - Project project = ProjectLoader.load(root).unwrap(); + Project project = load(root).unwrap(); assertThat(project.sources(), hasItem(root.resolve("model"))); assertThat(project.modelResult().getValidationEvents(), empty()); @@ -173,7 +174,7 @@ public void loadsProjectWithMultipleNamespaces() { @Test public void loadsProjectWithExternalJars() { Path root = toPath(getClass().getResource("external-jars")); - Result> result = ProjectLoader.load(root); + Result> result = load(root); assertThat(result.isOk(), is(true)); Project project = result.unwrap(); @@ -197,7 +198,7 @@ public void loadsProjectWithExternalJars() { @Test public void failsLoadingInvalidSmithyBuildJson() { Path root = toPath(getClass().getResource("broken/missing-version")); - Result> result = ProjectLoader.load(root); + Result> result = load(root); assertThat(result.isErr(), is(true)); } @@ -205,7 +206,7 @@ public void failsLoadingInvalidSmithyBuildJson() { @Test public void failsLoadingUnparseableSmithyBuildJson() { Path root = toPath(getClass().getResource("broken/parse-failure")); - Result> result = ProjectLoader.load(root); + Result> result = load(root); assertThat(result.isErr(), is(true)); } @@ -213,7 +214,7 @@ public void failsLoadingUnparseableSmithyBuildJson() { @Test public void doesntFailLoadingProjectWithNonExistingSource() { Path root = toPath(getClass().getResource("broken/source-doesnt-exist")); - Result> result = ProjectLoader.load(root); + Result> result = load(root); assertThat(result.isErr(), is(false)); assertThat(result.unwrap().smithyFiles().size(), equalTo(1)); // still have the prelude @@ -223,7 +224,7 @@ public void doesntFailLoadingProjectWithNonExistingSource() { @Test public void failsLoadingUnresolvableMavenDependency() { Path root = toPath(getClass().getResource("broken/unresolvable-maven-dependency")); - Result> result = ProjectLoader.load(root); + Result> result = load(root); assertThat(result.isErr(), is(true)); } @@ -231,7 +232,7 @@ public void failsLoadingUnresolvableMavenDependency() { @Test public void failsLoadingUnresolvableProjectDependency() { Path root = toPath(getClass().getResource("broken/unresolvable-maven-dependency")); - Result> result = ProjectLoader.load(root); + Result> result = load(root); assertThat(result.isErr(), is(true)); } @@ -239,7 +240,7 @@ public void failsLoadingUnresolvableProjectDependency() { @Test public void loadsProjectWithUnNormalizedDirs() { Path root = toPath(getClass().getResource("unnormalized-dirs")); - Project project = ProjectLoader.load(root).unwrap(); + Project project = load(root).unwrap(); assertThat(project.root(), equalTo(root)); assertThat(project.sources(), hasItems( @@ -269,7 +270,7 @@ public void changeFileApplyingSimpleTrait() { string Bar """; TestWorkspace workspace = TestWorkspace.multipleModels(m1, m2); - Project project = ProjectLoader.load(workspace.getRoot()).unwrap(); + Project project = load(workspace.getRoot()).unwrap(); Shape bar = project.modelResult().unwrap().expectShape(ShapeId.from("com.foo#Bar")); assertThat(bar.hasTrait("length"), is(true)); @@ -300,7 +301,7 @@ public void changeFileApplyingListTrait() { string Bar """; TestWorkspace workspace = TestWorkspace.multipleModels(m1, m2); - Project project = ProjectLoader.load(workspace.getRoot()).unwrap(); + Project project = load(workspace.getRoot()).unwrap(); Shape bar = project.modelResult().unwrap().expectShape(ShapeId.from("com.foo#Bar")); assertThat(bar.hasTrait("tags"), is(true)); @@ -337,7 +338,7 @@ public void changeFileApplyingListTraitWithUnrelatedDependencies() { apply Baz @length(min: 1) """; TestWorkspace workspace = TestWorkspace.multipleModels(m1, m2, m3); - Project project = ProjectLoader.load(workspace.getRoot()).unwrap(); + Project project = load(workspace.getRoot()).unwrap(); Shape bar = project.modelResult().unwrap().expectShape(ShapeId.from("com.foo#Bar")); Shape baz = project.modelResult().unwrap().expectShape(ShapeId.from("com.foo#Baz")); @@ -379,7 +380,7 @@ public void changingFileApplyingListTraitWithRelatedDependencies() { apply Bar @length(min: 1) """; TestWorkspace workspace = TestWorkspace.multipleModels(m1, m2, m3); - Project project = ProjectLoader.load(workspace.getRoot()).unwrap(); + Project project = load(workspace.getRoot()).unwrap(); Shape bar = project.modelResult().unwrap().expectShape(ShapeId.from("com.foo#Bar")); assertThat(bar.hasTrait("tags"), is(true)); @@ -419,7 +420,7 @@ public void changingFileApplyingListTraitWithRelatedArrayTraitDependencies() { apply Bar @tags(["bar"]) """; TestWorkspace workspace = TestWorkspace.multipleModels(m1, m2, m3); - Project project = ProjectLoader.load(workspace.getRoot()).unwrap(); + Project project = load(workspace.getRoot()).unwrap(); Shape bar = project.modelResult().unwrap().expectShape(ShapeId.from("com.foo#Bar")); assertThat(bar.hasTrait("tags"), is(true)); @@ -450,7 +451,7 @@ public void changingFileWithDependencies() { apply Foo @length(min: 1) """; TestWorkspace workspace = TestWorkspace.multipleModels(m1, m2); - Project project = ProjectLoader.load(workspace.getRoot()).unwrap(); + Project project = load(workspace.getRoot()).unwrap(); Shape foo = project.modelResult().unwrap().expectShape(ShapeId.from("com.foo#Foo")); assertThat(foo.hasTrait("length"), is(true)); @@ -481,7 +482,7 @@ public void changingFileWithArrayDependencies() { apply Foo @tags(["foo"]) """; TestWorkspace workspace = TestWorkspace.multipleModels(m1, m2); - Project project = ProjectLoader.load(workspace.getRoot()).unwrap(); + Project project = load(workspace.getRoot()).unwrap(); Shape foo = project.modelResult().unwrap().expectShape(ShapeId.from("com.foo#Foo")); assertThat(foo.hasTrait("tags"), is(true)); @@ -513,7 +514,7 @@ public void changingFileWithMixedArrayDependencies() { apply Foo @tags(["foo"]) """; TestWorkspace workspace = TestWorkspace.multipleModels(m1, m2); - Project project = ProjectLoader.load(workspace.getRoot()).unwrap(); + Project project = load(workspace.getRoot()).unwrap(); Shape foo = project.modelResult().unwrap().expectShape(ShapeId.from("com.foo#Foo")); assertThat(foo.hasTrait("tags"), is(true)); @@ -549,7 +550,7 @@ public void changingFileWithArrayDependenciesWithDependencies() { apply Bar @length(min: 1) """; TestWorkspace workspace = TestWorkspace.multipleModels(m1, m2, m3); - Project project = ProjectLoader.load(workspace.getRoot()).unwrap(); + Project project = load(workspace.getRoot()).unwrap(); Shape foo = project.modelResult().unwrap().expectShape(ShapeId.from("com.foo#Foo")); Shape bar = project.modelResult().unwrap().expectShape(ShapeId.from("com.foo#Bar")); @@ -601,7 +602,7 @@ public void removingSimpleApply() { apply Bar @pattern("a") """; TestWorkspace workspace = TestWorkspace.multipleModels(m1, m2, m3); - Project project = ProjectLoader.load(workspace.getRoot()).unwrap(); + Project project = load(workspace.getRoot()).unwrap(); Shape bar = project.modelResult().unwrap().expectShape(ShapeId.from("com.foo#Bar")); assertThat(bar.hasTrait("pattern"), is(true)); @@ -639,7 +640,7 @@ public void removingArrayApply() { apply Bar @tags(["bar"]) """; TestWorkspace workspace = TestWorkspace.multipleModels(m1, m2, m3); - Project project = ProjectLoader.load(workspace.getRoot()).unwrap(); + Project project = load(workspace.getRoot()).unwrap(); Shape bar = project.modelResult().unwrap().expectShape(ShapeId.from("com.foo#Bar")); assertThat(bar.hasTrait("tags"), is(true)); @@ -656,6 +657,10 @@ public void removingArrayApply() { assertThat(bar.expectTrait(TagsTrait.class).getTags(), containsInAnyOrder("bar")); } + private static Result> load(Path root) { + return ProjectLoader.load(root, new ServerState()); + } + public static Path toPath(URL url) { try { return Paths.get(url.toURI()); From d640649c74daee1ad6b0c7ebd974ab6c6bec4bb2 Mon Sep 17 00:00:00 2001 From: Miles Ziemer Date: Tue, 31 Dec 2024 10:43:06 -0500 Subject: [PATCH 5/9] Add interface for getting managed files Project loading can now depend on a single interface that provides access to managed files, rather than ServerState itself. --- .../software/amazon/smithy/lsp/FileCache.java | 40 ------------------- .../amazon/smithy/lsp/ManagedFiles.java | 16 ++++++++ .../amazon/smithy/lsp/ServerState.java | 20 +--------- .../lsp/project/ProjectConfigLoader.java | 25 ++++++------ .../smithy/lsp/project/ProjectLoader.java | 21 +++++----- .../lsp/project/ProjectConfigLoaderTest.java | 13 ++++-- 6 files changed, 48 insertions(+), 87 deletions(-) delete mode 100644 src/main/java/software/amazon/smithy/lsp/FileCache.java create mode 100644 src/main/java/software/amazon/smithy/lsp/ManagedFiles.java diff --git a/src/main/java/software/amazon/smithy/lsp/FileCache.java b/src/main/java/software/amazon/smithy/lsp/FileCache.java deleted file mode 100644 index a4cea1d3..00000000 --- a/src/main/java/software/amazon/smithy/lsp/FileCache.java +++ /dev/null @@ -1,40 +0,0 @@ -/* - * Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. - * SPDX-License-Identifier: Apache-2.0 - */ - -package software.amazon.smithy.lsp; - -import java.util.concurrent.ConcurrentHashMap; -import java.util.concurrent.ConcurrentMap; -import software.amazon.smithy.lsp.project.ProjectFile; - -/** - * Todo: Implement a data structure that can: - * - keep track of opened files - * - handle creating the right type of ProjectFile - * - close files - * - cache the text of these files to be used when loading models - * - * Notes: - * - One idea I have is to make it cache all the project files on load. - * Right now, ProjectLoader reads from disk every time. This isn't too - * bad, because project changes aren't _that_ frequent, but we end up - * reading the files _again_ to create ProjectFiles for everything that - * was loaded. - * - My concern here is that when there are changes on the filesystem, - * we need to make sure we always stay in sync. Right now, we avoid - * this by just reading from disk every time we do a full project - * reload. - * - Another idea is to just cache managed files, and then just create - * ProjectFiles as they're being added to the ModelAssembler. - * - We still have to handle files in jars though, and I'm _not_ - * re-implementing ModelDiscovery. - * - Yet another idea would be to lazily read these files from disk. This - * avoids most implementation complications. - * - But there's a design problem - when we want to support refactoring, - * we will need to read everything from disk + parse it anyways. - */ -public final class FileCache { - private final ConcurrentMap managedProjectFiles = new ConcurrentHashMap<>(); -} diff --git a/src/main/java/software/amazon/smithy/lsp/ManagedFiles.java b/src/main/java/software/amazon/smithy/lsp/ManagedFiles.java new file mode 100644 index 00000000..fd4289c7 --- /dev/null +++ b/src/main/java/software/amazon/smithy/lsp/ManagedFiles.java @@ -0,0 +1,16 @@ +/* + * Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. + * SPDX-License-Identifier: Apache-2.0 + */ + +package software.amazon.smithy.lsp; + +import software.amazon.smithy.lsp.document.Document; + +public interface ManagedFiles { + /** + * @param uri Uri of the document to get + * @return The document if found and it is managed, otherwise {@code null} + */ + Document getManagedDocument(String uri); +} diff --git a/src/main/java/software/amazon/smithy/lsp/ServerState.java b/src/main/java/software/amazon/smithy/lsp/ServerState.java index 7adabf0a..d9a32e27 100644 --- a/src/main/java/software/amazon/smithy/lsp/ServerState.java +++ b/src/main/java/software/amazon/smithy/lsp/ServerState.java @@ -31,7 +31,7 @@ /** * Keeps track of the state of the server. */ -public final class ServerState { +public final class ServerState implements ManagedFiles { private static final Logger LOGGER = Logger.getLogger(ServerState.class.getName()); private final Map projects; @@ -73,10 +73,7 @@ public Project findProjectByRoot(String root) { return projects.get(root); } - /** - * @param uri Uri of the document to get - * @return The document if found and it is managed, otherwise {@code null} - */ + @Override public Document getManagedDocument(String uri) { if (managedUris.contains(uri)) { ProjectAndFile projectAndFile = findProjectAndFile(uri); @@ -88,19 +85,6 @@ public Document getManagedDocument(String uri) { return null; } - /** - * @param path The path of the document to get - * @return The document if found and it is managed, otherwise {@code null} - */ - public Document getManagedDocument(Path path) { - if (managedUris.isEmpty()) { - return null; - } - - String uri = LspAdapter.toUri(path.toString()); - return getManagedDocument(uri); - } - ProjectAndFile findProjectAndFile(String uri) { String path = LspAdapter.toPath(uri); diff --git a/src/main/java/software/amazon/smithy/lsp/project/ProjectConfigLoader.java b/src/main/java/software/amazon/smithy/lsp/project/ProjectConfigLoader.java index 1061ce77..05e0aeda 100644 --- a/src/main/java/software/amazon/smithy/lsp/project/ProjectConfigLoader.java +++ b/src/main/java/software/amazon/smithy/lsp/project/ProjectConfigLoader.java @@ -15,8 +15,9 @@ import java.util.Map; import java.util.logging.Logger; import software.amazon.smithy.build.model.SmithyBuildConfig; -import software.amazon.smithy.lsp.ServerState; +import software.amazon.smithy.lsp.ManagedFiles; import software.amazon.smithy.lsp.document.Document; +import software.amazon.smithy.lsp.protocol.LspAdapter; import software.amazon.smithy.lsp.util.Result; import software.amazon.smithy.model.node.Node; import software.amazon.smithy.model.node.NodeMapper; @@ -87,11 +88,7 @@ public final class ProjectConfigLoader { private ProjectConfigLoader() { } - static Result> loadFromRoot(Path workspaceRoot) { - return loadFromRoot(workspaceRoot, new ServerState()); - } - - static Result> loadFromRoot(Path workspaceRoot, ServerState state) { + static Result> loadFromRoot(Path workspaceRoot, ManagedFiles managedFiles) { SmithyBuildConfig.Builder builder = SmithyBuildConfig.builder(); List exceptions = new ArrayList<>(); Map buildFiles = new HashMap<>(); @@ -100,7 +97,7 @@ static Result> loadFromRoot(Path workspaceRoot, S if (Files.isRegularFile(smithyBuildPath)) { LOGGER.info("Loading smithy-build.json from " + smithyBuildPath); Result result = Result.ofFallible(() -> { - BuildFile buildFile = addBuildFile(buildFiles, smithyBuildPath, state); + BuildFile buildFile = addBuildFile(buildFiles, smithyBuildPath, managedFiles); return SmithyBuildConfig.fromNode( Node.parseJsonWithComments(buildFile.document().copyText(), buildFile.path())); }); @@ -116,7 +113,7 @@ static Result> loadFromRoot(Path workspaceRoot, S Path extPath = workspaceRoot.resolve(ext); if (Files.isRegularFile(extPath)) { Result result = Result.ofFallible(() -> { - BuildFile buildFile = addBuildFile(buildFiles, extPath, state); + BuildFile buildFile = addBuildFile(buildFiles, extPath, managedFiles); return loadSmithyBuildExtensions(buildFile); }); result.get().ifPresent(extensionsBuilder::merge); @@ -129,7 +126,7 @@ static Result> loadFromRoot(Path workspaceRoot, S if (Files.isRegularFile(smithyProjectPath)) { LOGGER.info("Loading .smithy-project.json from " + smithyProjectPath); Result result = Result.ofFallible(() -> { - BuildFile buildFile = addBuildFile(buildFiles, smithyProjectPath, state); + BuildFile buildFile = addBuildFile(buildFiles, smithyProjectPath, managedFiles); return ProjectConfig.Builder.load(buildFile); }); if (result.isOk()) { @@ -154,14 +151,16 @@ static Result> loadFromRoot(Path workspaceRoot, S return Result.ok(finalConfigBuilder.build()); } - private static BuildFile addBuildFile(Map buildFiles, Path path, ServerState state) { - Document managed = state.getManagedDocument(path); + private static BuildFile addBuildFile(Map buildFiles, Path path, ManagedFiles managedFiles) { + String pathString = path.toString(); + String uri = LspAdapter.toUri(pathString); + Document managed = managedFiles.getManagedDocument(uri); BuildFile buildFile; if (managed != null) { - buildFile = new BuildFile(path.toString(), managed); + buildFile = new BuildFile(pathString, managed); } else { Document document = Document.of(IoUtils.readUtf8File(path)); - buildFile = new BuildFile(path.toString(), document); + buildFile = new BuildFile(pathString, document); } buildFiles.put(buildFile.path(), buildFile); return buildFile; diff --git a/src/main/java/software/amazon/smithy/lsp/project/ProjectLoader.java b/src/main/java/software/amazon/smithy/lsp/project/ProjectLoader.java index f17c2db7..ece5f12a 100644 --- a/src/main/java/software/amazon/smithy/lsp/project/ProjectLoader.java +++ b/src/main/java/software/amazon/smithy/lsp/project/ProjectLoader.java @@ -22,7 +22,7 @@ import java.util.function.Supplier; import java.util.logging.Logger; import java.util.stream.Stream; -import software.amazon.smithy.lsp.ServerState; +import software.amazon.smithy.lsp.ManagedFiles; import software.amazon.smithy.lsp.document.Document; import software.amazon.smithy.lsp.protocol.LspAdapter; import software.amazon.smithy.lsp.util.Result; @@ -47,7 +47,7 @@ private ProjectLoader() { /** * Loads a detachedProjects (single-file) {@link Project} with the given file. * - *

Unlike {@link #load(Path, ServerState)}, this method isn't + *

Unlike {@link #load(Path, ManagedFiles)}, this method isn't * fallible since it doesn't do any IO that we would want to recover an * error from. * @@ -106,11 +106,11 @@ public static Project loadDetached(String uri, String text) { * reason about how the project was structured. * * @param root Path of the project root - * @param state Server's current state + * @param managedFiles Files managed by the server * @return Result of loading the project */ - public static Result> load(Path root, ServerState state) { - Result> configResult = ProjectConfigLoader.loadFromRoot(root, state); + public static Result> load(Path root, ManagedFiles managedFiles) { + Result> configResult = ProjectConfigLoader.loadFromRoot(root, managedFiles); if (configResult.isErr()) { return Result.err(configResult.unwrapErr()); } @@ -129,7 +129,7 @@ public static Result> load(Path root, ServerState state LoadModelResult result; try { - result = doLoad(state::getManagedDocument, dependencies, allSmithyFilePaths); + result = doLoad(managedFiles, dependencies, allSmithyFilePaths); } catch (Exception e) { return Result.err(Collections.singletonList(e)); } @@ -154,10 +154,6 @@ private record LoadModelResult( ) { } - private interface ManagedFiles { - Document getManagedDocument(String uri); - } - private static LoadModelResult doLoad( ManagedFiles managedFiles, List dependencies, @@ -178,7 +174,7 @@ private static LoadModelResult doLoad( ValidatedResult modelResult = loadModel(managedFiles, allSmithyFilePaths, assembler, smithyFiles); Project.RebuildIndex rebuildIndex = Project.RebuildIndex.create(modelResult); - addExtraSmithyFiles(managedFiles, rebuildIndex.filesToDefinedShapes().keySet(), smithyFiles); + addDependencySmithyFiles(managedFiles, rebuildIndex.filesToDefinedShapes().keySet(), smithyFiles); return new LoadModelResult( assemblerFactory, @@ -207,7 +203,8 @@ private static ValidatedResult loadModel( return assembler.assemble(); } - private static void addExtraSmithyFiles( + // Smithy files in jars were loaded by the model assembler via model discovery, so we need to collect those. + private static void addDependencySmithyFiles( ManagedFiles managedFiles, Set loadedSmithyFilePaths, Map smithyFiles diff --git a/src/test/java/software/amazon/smithy/lsp/project/ProjectConfigLoaderTest.java b/src/test/java/software/amazon/smithy/lsp/project/ProjectConfigLoaderTest.java index 7e0d9f62..6b4c0bb5 100644 --- a/src/test/java/software/amazon/smithy/lsp/project/ProjectConfigLoaderTest.java +++ b/src/test/java/software/amazon/smithy/lsp/project/ProjectConfigLoaderTest.java @@ -18,6 +18,7 @@ import org.junit.jupiter.api.Test; import software.amazon.smithy.build.model.MavenConfig; import software.amazon.smithy.build.model.MavenRepository; +import software.amazon.smithy.lsp.ServerState; import software.amazon.smithy.lsp.util.Result; public class ProjectConfigLoaderTest { @@ -25,7 +26,7 @@ public class ProjectConfigLoaderTest { public void loadsConfigWithEnvVariable() { System.setProperty("FOO", "bar"); Path root = toPath(getClass().getResource("env-config")); - Result> result = ProjectConfigLoader.loadFromRoot(root); + Result> result = load(root); assertThat(result.isOk(), is(true)); ProjectConfig config = result.unwrap(); @@ -41,7 +42,7 @@ public void loadsConfigWithEnvVariable() { @Test public void loadsLegacyConfig() { Path root = toPath(getClass().getResource("legacy-config")); - Result> result = ProjectConfigLoader.loadFromRoot(root); + Result> result = load(root); assertThat(result.isOk(), is(true)); ProjectConfig config = result.unwrap(); @@ -56,7 +57,7 @@ public void loadsLegacyConfig() { @Test public void prefersNonLegacyConfig() { Path root = toPath(getClass().getResource("legacy-config-with-conflicts")); - Result> result = ProjectConfigLoader.loadFromRoot(root); + Result> result = load(root); assertThat(result.isOk(), is(true)); ProjectConfig config = result.unwrap(); @@ -71,10 +72,14 @@ public void prefersNonLegacyConfig() { @Test public void mergesBuildExts() { Path root = toPath(getClass().getResource("build-exts")); - Result> result = ProjectConfigLoader.loadFromRoot(root); + Result> result = load(root); assertThat(result.isOk(), is(true)); ProjectConfig config = result.unwrap(); assertThat(config.imports(), containsInAnyOrder(containsString("main.smithy"), containsString("other.smithy"))); } + + private static Result> load(Path root) { + return ProjectConfigLoader.loadFromRoot(root, new ServerState()); + } } From 50b8d37d07084d394625c0e93fb8b3d1772c1845 Mon Sep 17 00:00:00 2001 From: Miles Ziemer Date: Tue, 31 Dec 2024 10:57:54 -0500 Subject: [PATCH 6/9] Add documentation to new methods --- .../amazon/smithy/lsp/ManagedFiles.java | 7 +++++ .../amazon/smithy/lsp/ServerState.java | 25 ++++++++++++------ .../amazon/smithy/lsp/project/Project.java | 26 ++++++++++++++++--- .../smithy/lsp/project/ProjectLoader.java | 9 +++---- 4 files changed, 50 insertions(+), 17 deletions(-) diff --git a/src/main/java/software/amazon/smithy/lsp/ManagedFiles.java b/src/main/java/software/amazon/smithy/lsp/ManagedFiles.java index fd4289c7..d10a17a6 100644 --- a/src/main/java/software/amazon/smithy/lsp/ManagedFiles.java +++ b/src/main/java/software/amazon/smithy/lsp/ManagedFiles.java @@ -7,6 +7,13 @@ import software.amazon.smithy.lsp.document.Document; +/** + * Provides access to {@link Document}s managed by the server. + * + *

A document is _managed_ if its state is controlled by the lifecycle methods + * didOpen, didClose, didChange, didSave. In other words, reading from disk _may_ + * not provide the accurate file content. + */ public interface ManagedFiles { /** * @param uri Uri of the document to get diff --git a/src/main/java/software/amazon/smithy/lsp/ServerState.java b/src/main/java/software/amazon/smithy/lsp/ServerState.java index d9a32e27..21fc8b89 100644 --- a/src/main/java/software/amazon/smithy/lsp/ServerState.java +++ b/src/main/java/software/amazon/smithy/lsp/ServerState.java @@ -49,10 +49,16 @@ public ServerState() { this.lifecycleManager = new DocumentLifecycleManager(); } + /** + * @return All projects tracked by the server. + */ public Collection getAllProjects() { return projects.values(); } + /** + * @return All files managed by the server, including their projects. + */ public Collection getAllManaged() { List allManaged = new ArrayList<>(managedUris.size()); for (String uri : managedUris) { @@ -61,18 +67,13 @@ public Collection getAllManaged() { return allManaged; } + /** + * @return All workspace paths tracked by the server. + */ public Set workspacePaths() { return workspacePaths; } - public DocumentLifecycleManager lifecycleManager() { - return lifecycleManager; - } - - public Project findProjectByRoot(String root) { - return projects.get(root); - } - @Override public Document getManagedDocument(String uri) { if (managedUris.contains(uri)) { @@ -85,6 +86,14 @@ public Document getManagedDocument(String uri) { return null; } + DocumentLifecycleManager lifecycleManager() { + return lifecycleManager; + } + + Project findProjectByRoot(String root) { + return projects.get(root); + } + ProjectAndFile findProjectAndFile(String uri) { String path = LspAdapter.toPath(uri); diff --git a/src/main/java/software/amazon/smithy/lsp/project/Project.java b/src/main/java/software/amazon/smithy/lsp/project/Project.java index 690353a5..ad97fc5b 100644 --- a/src/main/java/software/amazon/smithy/lsp/project/Project.java +++ b/src/main/java/software/amazon/smithy/lsp/project/Project.java @@ -67,9 +67,23 @@ public final class Project { this.rebuildIndex = rebuildIndex; } + /** + * The type of project, which depends on how it was loaded. + */ public enum Type { + /** + * A project loaded using some build configuration files, i.e. smithy-build.json. + */ NORMAL, + + /** + * A project loaded from a single source file, without any build configuration files. + */ DETACHED, + + /** + * A project loaded with no source or build configuration files. + */ EMPTY; } @@ -510,18 +524,22 @@ RebuildIndex recompute(ValidatedResult modelResult) { for (Node element : traitNode.expectArrayNode()) { String elementSourceFilename = element.getSourceLocation().getFilename(); if (!elementSourceFilename.equals(shapeSourceFilename)) { - newIndex.filesToDependentFiles.computeIfAbsent(elementSourceFilename, (f) -> new HashSet<>()) + newIndex.filesToDependentFiles + .computeIfAbsent(elementSourceFilename, (f) -> new HashSet<>()) .add(shapeSourceFilename); - newIndex.shapeIdsToDependenciesFiles.computeIfAbsent(shape.getId(), (i) -> new HashSet<>()) + newIndex.shapeIdsToDependenciesFiles + .computeIfAbsent(shape.getId(), (i) -> new HashSet<>()) .add(elementSourceFilename); } } } else { String traitSourceFilename = traitApplication.getSourceLocation().getFilename(); if (!traitSourceFilename.equals(shapeSourceFilename)) { - newIndex.shapesToAppliedTraitsInOtherFiles.computeIfAbsent(shape.getId(), (i) -> new ArrayList<>()) + newIndex.shapesToAppliedTraitsInOtherFiles + .computeIfAbsent(shape.getId(), (i) -> new ArrayList<>()) .add(traitApplication); - newIndex.filesToTraitsTheyApply.computeIfAbsent(traitSourceFilename, (f) -> new HashMap<>()) + newIndex.filesToTraitsTheyApply + .computeIfAbsent(traitSourceFilename, (f) -> new HashMap<>()) .computeIfAbsent(shape.getId(), (i) -> new ArrayList<>()) .add(traitApplication); } diff --git a/src/main/java/software/amazon/smithy/lsp/project/ProjectLoader.java b/src/main/java/software/amazon/smithy/lsp/project/ProjectLoader.java index ece5f12a..ebca7684 100644 --- a/src/main/java/software/amazon/smithy/lsp/project/ProjectLoader.java +++ b/src/main/java/software/amazon/smithy/lsp/project/ProjectLoader.java @@ -75,9 +75,9 @@ public static Project loadDetached(String uri, String text) { LoadModelResult result; try { result = doLoad(managedFiles, dependencies, allSmithyFilePaths); - } catch (Exception e) { - // TODO: Clean up this comment - // Note: This can't happen because we have no dependencies to turn into URLs + } catch (IOException e) { + // Note: This can't happen because we aren't doing any fallible IO, + // as only the prelude will be read from disk throw new RuntimeException(e); } @@ -158,7 +158,7 @@ private static LoadModelResult doLoad( ManagedFiles managedFiles, List dependencies, List allSmithyFilePaths - ) throws Exception { + ) throws IOException { // The model assembler factory is used to get assemblers that already have the correct // dependencies resolved for future loads Supplier assemblerFactory = createModelAssemblerFactory(dependencies); @@ -278,7 +278,6 @@ private static URLClassLoader createDependenciesClassLoader(List dependenc return new URLClassLoader(urls); } - // TODO: Can there be duplicate paths in this list? If there are, we may end up reading from disk multiple times // sources and imports can contain directories or files, relative or absolute private static List collectAllSmithyPaths(Path root, List sources, List imports) { List paths = new ArrayList<>(); From 350d83f1f5e97cc9c95e47f2e41135c22407ca00 Mon Sep 17 00:00:00 2001 From: Miles Ziemer Date: Tue, 31 Dec 2024 11:32:15 -0500 Subject: [PATCH 7/9] Rename DocumentLifecycleManager --- ...ntLifecycleManager.java => FileTasks.java} | 4 +- .../amazon/smithy/lsp/ServerState.java | 12 ++--- .../smithy/lsp/SmithyLanguageServer.java | 14 +++--- .../smithy/lsp/SmithyLanguageServerTest.java | 44 +++++++++---------- 4 files changed, 37 insertions(+), 37 deletions(-) rename src/main/java/software/amazon/smithy/lsp/{DocumentLifecycleManager.java => FileTasks.java} (92%) diff --git a/src/main/java/software/amazon/smithy/lsp/DocumentLifecycleManager.java b/src/main/java/software/amazon/smithy/lsp/FileTasks.java similarity index 92% rename from src/main/java/software/amazon/smithy/lsp/DocumentLifecycleManager.java rename to src/main/java/software/amazon/smithy/lsp/FileTasks.java index 7d2c98fb..945df04f 100644 --- a/src/main/java/software/amazon/smithy/lsp/DocumentLifecycleManager.java +++ b/src/main/java/software/amazon/smithy/lsp/FileTasks.java @@ -11,10 +11,10 @@ import java.util.concurrent.ExecutionException; /** - * Tracks asynchronous lifecycle tasks, allowing for cancellation of an ongoing + * Container for tracking asynchronous tasks by file, allowing for cancellation of an ongoing * task if a new task needs to be started. */ -final class DocumentLifecycleManager { +final class FileTasks { private final Map> tasks = new HashMap<>(); CompletableFuture getTask(String uri) { diff --git a/src/main/java/software/amazon/smithy/lsp/ServerState.java b/src/main/java/software/amazon/smithy/lsp/ServerState.java index 21fc8b89..5aa8d7f2 100644 --- a/src/main/java/software/amazon/smithy/lsp/ServerState.java +++ b/src/main/java/software/amazon/smithy/lsp/ServerState.java @@ -37,7 +37,7 @@ public final class ServerState implements ManagedFiles { private final Map projects; private final Set workspacePaths; private final Set managedUris; - private final DocumentLifecycleManager lifecycleManager; + private final FileTasks lifecycleTasks; /** * Create a new, empty server state. @@ -46,7 +46,7 @@ public ServerState() { this.projects = new HashMap<>(); this.workspacePaths = new HashSet<>(); this.managedUris = new HashSet<>(); - this.lifecycleManager = new DocumentLifecycleManager(); + this.lifecycleTasks = new FileTasks(); } /** @@ -86,8 +86,8 @@ public Document getManagedDocument(String uri) { return null; } - DocumentLifecycleManager lifecycleManager() { - return lifecycleManager; + FileTasks lifecycleTasks() { + return lifecycleTasks; } Project findProjectByRoot(String root) { @@ -136,14 +136,14 @@ void close(String uri) { ProjectAndFile projectAndFile = findProjectAndFile(uri); if (projectAndFile != null && projectAndFile.project().type() == Project.Type.DETACHED) { // Only cancel tasks for detached projects, since we're dropping the project - lifecycleManager.cancelTask(uri); + lifecycleTasks.cancelTask(uri); projects.remove(uri); } } List tryInitProject(Path root) { LOGGER.finest("Initializing project at " + root); - lifecycleManager.cancelAllTasks(); + lifecycleTasks.cancelAllTasks(); Result> loadResult = ProjectLoader.load(root, this); String projectName = root.toString(); diff --git a/src/main/java/software/amazon/smithy/lsp/SmithyLanguageServer.java b/src/main/java/software/amazon/smithy/lsp/SmithyLanguageServer.java index c9616d99..c9fbce55 100644 --- a/src/main/java/software/amazon/smithy/lsp/SmithyLanguageServer.java +++ b/src/main/java/software/amazon/smithy/lsp/SmithyLanguageServer.java @@ -430,7 +430,7 @@ public void didChange(DidChangeTextDocumentParams params) { String uri = params.getTextDocument().getUri(); - state.lifecycleManager().cancelTask(uri); + state.lifecycleTasks().cancelTask(uri); ProjectAndFile projectAndFile = state.findManaged(uri); if (projectAndFile == null) { @@ -462,7 +462,7 @@ public void didChange(DidChangeTextDocumentParams params) { CompletableFuture future = CompletableFuture .runAsync(() -> project.updateModelWithoutValidating(uri)) .thenComposeAsync(unused -> sendFileDiagnostics(projectAndFile)); - state.lifecycleManager().putTask(uri, future); + state.lifecycleTasks().putTask(uri, future); } } @@ -472,11 +472,11 @@ public void didOpen(DidOpenTextDocumentParams params) { String uri = params.getTextDocument().getUri(); - state.lifecycleManager().cancelTask(uri); + state.lifecycleTasks().cancelTask(uri); ProjectAndFile projectAndFile = state.open(uri, params.getTextDocument().getText()); - state.lifecycleManager().putTask(uri, sendFileDiagnostics(projectAndFile)); + state.lifecycleTasks().putTask(uri, sendFileDiagnostics(projectAndFile)); } @Override @@ -492,7 +492,7 @@ public void didSave(DidSaveTextDocumentParams params) { LOGGER.finest("DidSave"); String uri = params.getTextDocument().getUri(); - state.lifecycleManager().cancelTask(uri); + state.lifecycleTasks().cancelTask(uri); ProjectAndFile projectAndFile = state.findManaged(uri); if (projectAndFile == null) { @@ -513,7 +513,7 @@ public void didSave(DidSaveTextDocumentParams params) { CompletableFuture future = CompletableFuture .runAsync(() -> project.updateAndValidateModel(uri)) .thenCompose(unused -> sendFileDiagnostics(projectAndFile)); - state.lifecycleManager().putTask(uri, future); + state.lifecycleTasks().putTask(uri, future); } } @@ -644,7 +644,7 @@ public CompletableFuture> formatting(DocumentFormatting private void sendFileDiagnosticsForManagedDocuments() { for (ProjectAndFile managed : state.getAllManaged()) { - state.lifecycleManager().putOrComposeTask(managed.uri(), sendFileDiagnostics(managed)); + state.lifecycleTasks().putOrComposeTask(managed.uri(), sendFileDiagnostics(managed)); } } diff --git a/src/test/java/software/amazon/smithy/lsp/SmithyLanguageServerTest.java b/src/test/java/software/amazon/smithy/lsp/SmithyLanguageServerTest.java index 4ac34dda..f80dad87 100644 --- a/src/test/java/software/amazon/smithy/lsp/SmithyLanguageServerTest.java +++ b/src/test/java/software/amazon/smithy/lsp/SmithyLanguageServerTest.java @@ -176,7 +176,7 @@ public void didChange() throws Exception { server.didChange(changeBuilder.range(rangeBuilder.shiftRight().build()).text(" ").build()); server.didChange(changeBuilder.range(rangeBuilder.shiftRight().build()).text("G").build()); - server.getState().lifecycleManager().waitForAllTasks(); + server.getState().lifecycleTasks().waitForAllTasks(); // mostly so you can see what it looks like assertThat(server.getState().getManagedDocument(uri).copyText(), equalTo(safeString(""" @@ -229,7 +229,7 @@ public void didChangeReloadsModel() throws Exception { .build(); server.didChange(didChangeParams); - server.getState().lifecycleManager().getTask(uri).get(); + server.getState().lifecycleTasks().getTask(uri).get(); assertThat(server.getState().findProjectAndFile(uri).project().modelResult().getValidationEvents(), containsInAnyOrder(eventWithMessage(containsString("Error creating trait")))); @@ -464,7 +464,7 @@ public void addingWatchedFile() throws Exception { .build()); // Make sure the task is running, then wait for it - CompletableFuture future = server.getState().lifecycleManager().getTask(uri); + CompletableFuture future = server.getState().lifecycleTasks().getTask(uri); assertThat(future, notNullValue()); future.get(); @@ -656,7 +656,7 @@ public void reloadingProjectWithArrayMetadataValues() throws Exception { .uri(uri) .build()); - server.getState().lifecycleManager().getTask(uri).get(); + server.getState().lifecycleTasks().getTask(uri).get(); Map metadataAfter = server.getState().findProjectByRoot(workspace.getRoot().toString()).modelResult().unwrap().getMetadata(); assertThat(metadataAfter, hasKey("foo")); @@ -670,7 +670,7 @@ public void reloadingProjectWithArrayMetadataValues() throws Exception { .text("") .build()); - server.getState().lifecycleManager().getTask(uri).get(); + server.getState().lifecycleTasks().getTask(uri).get(); Map metadataAfter2 = server.getState().findProjectByRoot(workspace.getRoot().toString()).modelResult().unwrap().getMetadata(); assertThat(metadataAfter2, hasKey("foo")); @@ -717,7 +717,7 @@ public void changingWatchedFilesWithMetadata() throws Exception { .event(uri, FileChangeType.Deleted) .build()); - server.getState().lifecycleManager().waitForAllTasks(); + server.getState().lifecycleTasks().waitForAllTasks(); Map metadataAfter = server.getState().findProjectByRoot(workspace.getRoot().toString()).modelResult().unwrap().getMetadata(); assertThat(metadataAfter, hasKey("foo")); @@ -769,7 +769,7 @@ public void addingOpenedDetachedFile() throws Exception { .event(workspace.getUri("smithy-build.json"), FileChangeType.Changed) .build()); - server.getState().lifecycleManager().waitForAllTasks(); + server.getState().lifecycleTasks().waitForAllTasks(); assertManagedMatches(server, uri, Project.Type.NORMAL, workspace.getRoot()); assertThat(server.getState().findManaged(uri).project().modelResult().unwrap(), allOf( @@ -809,7 +809,7 @@ public void detachingOpenedFile() throws Exception { .event(workspace.getUri("smithy-build.json"), FileChangeType.Changed) .build()); - server.getState().lifecycleManager().waitForAllTasks(); + server.getState().lifecycleTasks().waitForAllTasks(); assertManagedMatches(server, uri, Project.Type.DETACHED, uri); assertThat(server.getState().findManaged(uri).project().modelResult(), hasValue(allOf( @@ -852,7 +852,7 @@ public void movingDetachedFile() throws Exception { .text(modelText) .build()); - server.getState().lifecycleManager().waitForAllTasks(); + server.getState().lifecycleTasks().waitForAllTasks(); assertThat(server.getState().findManaged(uri), nullValue()); assertManagedMatches(server, movedUri, Project.Type.DETACHED, movedUri); @@ -884,7 +884,7 @@ public void updatesDiagnosticsAfterReload() throws Exception { .text(modelText1) .build()); - server.getState().lifecycleManager().waitForAllTasks(); + server.getState().lifecycleTasks().waitForAllTasks(); List publishedDiagnostics1 = client.diagnostics; assertThat(publishedDiagnostics1, hasSize(1)); @@ -910,7 +910,7 @@ public void updatesDiagnosticsAfterReload() throws Exception { .event(uri2, FileChangeType.Created) .build()); - server.getState().lifecycleManager().waitForAllTasks(); + server.getState().lifecycleTasks().waitForAllTasks(); List publishedDiagnostics2 = client.diagnostics; assertThat(publishedDiagnostics2, hasSize(2)); // sent more diagnostics @@ -959,7 +959,7 @@ public void invalidSyntaxDetachedProjectBecomesValid() throws Exception { .text(modelText) .build()); - server.getState().lifecycleManager().waitForAllTasks(); + server.getState().lifecycleTasks().waitForAllTasks(); assertManagedMatches(server, uri, Project.Type.DETACHED, uri); ProjectAndFile projectAndFile = server.getState().findProjectAndFile(uri); @@ -977,7 +977,7 @@ public void invalidSyntaxDetachedProjectBecomesValid() throws Exception { """)) .build()); - server.getState().lifecycleManager().waitForAllTasks(); + server.getState().lifecycleTasks().waitForAllTasks(); assertManagedMatches(server, uri, Project.Type.DETACHED, uri); ProjectAndFile projectAndFile1 = server.getState().findProjectAndFile(uri); @@ -1003,7 +1003,7 @@ public void addingDetachedFileWithInvalidSyntax() throws Exception { .text("") .build()); - server.getState().lifecycleManager().waitForAllTasks(); + server.getState().lifecycleTasks().waitForAllTasks(); assertManagedMatches(server, uri, Project.Type.DETACHED, uri); @@ -1034,7 +1034,7 @@ public void addingDetachedFileWithInvalidSyntax() throws Exception { .range(LspAdapter.point(2, 0)) .build()); - server.getState().lifecycleManager().waitForAllTasks(); + server.getState().lifecycleTasks().waitForAllTasks(); assertManagedMatches(server, uri, Project.Type.NORMAL, workspace.getRoot()); assertThat(server.getState().findManaged(uri).project().modelResult(), hasValue(hasShapeWithId("com.foo#Foo"))); @@ -1068,7 +1068,7 @@ public void appliedTraitsAreMaintainedInPartialLoad() throws Exception { .text("2") .build()); - server.getState().lifecycleManager().waitForAllTasks(); + server.getState().lifecycleTasks().waitForAllTasks(); ProjectAndFile projectAndFile = server.getState().findProjectAndFile(uri2); assertThat(projectAndFile, notNullValue()); @@ -1091,7 +1091,7 @@ public void appliedTraitsAreMaintainedInPartialLoad() throws Exception { .text(safeString("string Another\n")) .build()); - server.getState().lifecycleManager().waitForAllTasks(); + server.getState().lifecycleTasks().waitForAllTasks(); projectAndFile = server.getState().findProjectAndFile(uri1); assertThat(projectAndFile, notNullValue()); @@ -1164,7 +1164,7 @@ public void brokenBuildFileEventuallyConsistent() throws Exception { """)) .range(LspAdapter.origin()) .build()); - server.getState().lifecycleManager().waitForAllTasks(); + server.getState().lifecycleTasks().waitForAllTasks(); ProjectAndFile projectAndFile = server.getState().findProjectAndFile(uri); assertThat(projectAndFile, notNullValue()); @@ -1256,7 +1256,7 @@ public void multiRootLifecycleManagement() throws Exception { .uri(barUri) .build()); - server.getState().lifecycleManager().waitForAllTasks(); + server.getState().lifecycleTasks().waitForAllTasks(); Project projectFoo = server.getState().findProjectByRoot(workspaceFoo.getName()); Project projectBar = server.getState().findProjectByRoot(workspaceBar.getName()); @@ -1327,7 +1327,7 @@ public void multiRootAddingWatchedFile() throws Exception { .range(LspAdapter.point(3, 0)) .build()); - server.getState().lifecycleManager().waitForAllTasks(); + server.getState().lifecycleTasks().waitForAllTasks(); Project projectFoo = server.getState().findProjectByRoot(workspaceFoo.getName()); Project projectBar = server.getState().findProjectByRoot(workspaceBar.getName()); @@ -1410,7 +1410,7 @@ public void multiRootChangingBuildFile() throws Exception { .range(LspAdapter.origin()) .build()); - server.getState().lifecycleManager().waitForAllTasks(); + server.getState().lifecycleTasks().waitForAllTasks(); assertThat(server.getState().findProjectAndFile(newUri), notNullValue()); assertThat(server.getState().findProjectAndFile(workspaceBar.getUri("model/main.smithy")), notNullValue()); @@ -1467,7 +1467,7 @@ public void addingWorkspaceFolder() throws Exception { .text(barModel) .build()); - server.getState().lifecycleManager().waitForAllTasks(); + server.getState().lifecycleTasks().waitForAllTasks(); assertManagedMatches(server, workspaceFoo.getUri("foo.smithy"), Project.Type.NORMAL, workspaceFoo.getRoot()); assertManagedMatches(server, workspaceBar.getUri("bar.smithy"), Project.Type.NORMAL, workspaceBar.getRoot()); From 8d319d9a948536dfa37a81630e67f217163f7e53 Mon Sep 17 00:00:00 2001 From: Miles Ziemer Date: Tue, 31 Dec 2024 14:39:08 -0500 Subject: [PATCH 8/9] Cleanup some of Project's api --- .../amazon/smithy/lsp/ServerState.java | 15 +- .../smithy/lsp/SmithyLanguageServer.java | 2 +- .../smithy/lsp/language/SimpleCompleter.java | 2 +- .../amazon/smithy/lsp/project/Project.java | 43 ++--- .../smithy/lsp/project/ProjectLoader.java | 4 + .../smithy/lsp/SmithyLanguageServerTest.java | 28 ++-- .../amazon/smithy/lsp/SmithyMatchers.java | 13 +- .../lsp/language/CompletionHandlerTest.java | 3 +- .../lsp/language/DefinitionHandlerTest.java | 5 +- .../lsp/language/DocumentSymbolTest.java | 3 +- .../smithy/lsp/language/HoverHandlerTest.java | 2 +- .../smithy/lsp/project/ProjectTest.java | 149 +++++++----------- 12 files changed, 111 insertions(+), 158 deletions(-) diff --git a/src/main/java/software/amazon/smithy/lsp/ServerState.java b/src/main/java/software/amazon/smithy/lsp/ServerState.java index 5aa8d7f2..c0de6d52 100644 --- a/src/main/java/software/amazon/smithy/lsp/ServerState.java +++ b/src/main/java/software/amazon/smithy/lsp/ServerState.java @@ -95,10 +95,8 @@ Project findProjectByRoot(String root) { } ProjectAndFile findProjectAndFile(String uri) { - String path = LspAdapter.toPath(uri); - for (Project project : projects.values()) { - ProjectFile projectFile = project.getProjectFile(path); + ProjectFile projectFile = project.getProjectFile(uri); if (projectFile != null) { return new ProjectAndFile(uri, project, projectFile); } @@ -150,8 +148,7 @@ List tryInitProject(Path root) { if (loadResult.isOk()) { Project updatedProject = loadResult.unwrap(); - // If the project didn't load any config files, it is now empty and should be removed - if (updatedProject.config().buildFiles().isEmpty()) { + if (updatedProject.type() == Project.Type.EMPTY) { removeProjectAndResolveDetached(projectName); } else { resolveDetachedProjects(projects.get(projectName), updatedProject); @@ -250,8 +247,8 @@ private void resolveDetachedProjects(Project oldProject, Project updatedProject) // This is a project reload, so we need to resolve any added/removed files // that need to be moved to or from detachedProjects projects. if (oldProject != null) { - Set currentProjectSmithyPaths = oldProject.smithyFiles().keySet(); - Set updatedProjectSmithyPaths = updatedProject.smithyFiles().keySet(); + Set currentProjectSmithyPaths = oldProject.getAllSmithyFilePaths(); + Set updatedProjectSmithyPaths = updatedProject.getAllSmithyFilePaths(); Set addedPaths = new HashSet<>(updatedProjectSmithyPaths); addedPaths.removeAll(currentProjectSmithyPaths); @@ -264,9 +261,9 @@ private void resolveDetachedProjects(Project oldProject, Project updatedProject) removedPaths.removeAll(updatedProjectSmithyPaths); for (String removedPath : removedPaths) { String removedUri = LspAdapter.toUri(removedPath); - // Only move to a detachedProjects project if the file is managed + // Only move to a detached project if the file is managed if (managedUris.contains(removedUri)) { - Document removedDocument = oldProject.getDocument(removedUri); + Document removedDocument = oldProject.getProjectFile(removedUri).document(); createDetachedProject(removedUri, removedDocument.copyText()); } } diff --git a/src/main/java/software/amazon/smithy/lsp/SmithyLanguageServer.java b/src/main/java/software/amazon/smithy/lsp/SmithyLanguageServer.java index c9fbce55..1cc512b0 100644 --- a/src/main/java/software/amazon/smithy/lsp/SmithyLanguageServer.java +++ b/src/main/java/software/amazon/smithy/lsp/SmithyLanguageServer.java @@ -375,7 +375,7 @@ public CompletableFuture serverStatus() { for (Project project : state.getAllProjects()) { openProjects.add(new OpenProject( LspAdapter.toUri(project.root().toString()), - project.smithyFiles().keySet().stream() + project.getAllSmithyFilePaths().stream() .map(LspAdapter::toUri) .toList(), project.type() == Project.Type.DETACHED)); diff --git a/src/main/java/software/amazon/smithy/lsp/language/SimpleCompleter.java b/src/main/java/software/amazon/smithy/lsp/language/SimpleCompleter.java index 04150084..75c9c31b 100644 --- a/src/main/java/software/amazon/smithy/lsp/language/SimpleCompleter.java +++ b/src/main/java/software/amazon/smithy/lsp/language/SimpleCompleter.java @@ -91,7 +91,7 @@ private CompletionCandidates customCandidates(CompletionCandidates.Custom custom } private Stream streamNamespaces() { - return context().project().smithyFiles().values().stream() + return context().project().getAllSmithyFiles().stream() .map(smithyFile -> switch (smithyFile) { case IdlFile idlFile -> idlFile.getParse().namespace().namespace(); default -> ""; diff --git a/src/main/java/software/amazon/smithy/lsp/project/Project.java b/src/main/java/software/amazon/smithy/lsp/project/Project.java index ad97fc5b..e70287d9 100644 --- a/src/main/java/software/amazon/smithy/lsp/project/Project.java +++ b/src/main/java/software/amazon/smithy/lsp/project/Project.java @@ -7,6 +7,7 @@ import java.nio.file.Path; import java.util.ArrayList; +import java.util.Collection; import java.util.Collections; import java.util.HashMap; import java.util.HashSet; @@ -147,18 +148,17 @@ public List dependencies() { } /** - * @return A map of paths to the {@link SmithyFile} at that path, containing - * all smithy files loaded in the project. + * @return The paths of all Smithy files loaded in the project. */ - public Map smithyFiles() { - return this.smithyFiles; + public Set getAllSmithyFilePaths() { + return this.smithyFiles.keySet(); } /** - * @return A map of paths to the set of shape ids defined in the file at that path. + * @return All the Smithy files loaded in the project. */ - public Map> definedShapesByFile() { - return this.rebuildIndex.filesToDefinedShapes(); + public Collection getAllSmithyFiles() { + return this.smithyFiles.values(); } public Type type() { @@ -173,25 +173,12 @@ public ValidatedResult modelResult() { } /** - * @param uri The URI of the {@link Document} to get - * @return The {@link Document} corresponding to the given {@code uri} if - * it exists in this project, otherwise {@code null} - */ - public Document getDocument(String uri) { - String path = LspAdapter.toPath(uri); - ProjectFile projectFile = getProjectFile(path); - if (projectFile == null) { - return null; - } - return projectFile.document(); - } - - /** - * @param path The path of the {@link ProjectFile} to get + * @param uri The uri of the {@link ProjectFile} to get * @return The {@link ProjectFile} corresponding to {@code path} if * it exists in this project, otherwise {@code null}. */ - public ProjectFile getProjectFile(String path) { + public ProjectFile getProjectFile(String uri) { + String path = LspAdapter.toPath(uri); SmithyFile smithyFile = smithyFiles.get(path); if (smithyFile != null) { return smithyFile; @@ -200,16 +187,6 @@ public ProjectFile getProjectFile(String path) { return config.buildFiles().get(path); } - /** - * @param uri The URI of the {@link SmithyFile} to get - * @return The {@link SmithyFile} corresponding to the given {@code uri} if - * it exists in this project, otherwise {@code null} - */ - public SmithyFile getSmithyFile(String uri) { - String path = LspAdapter.toPath(uri); - return smithyFiles.get(path); - } - /** * Update this project's model without running validation. * diff --git a/src/main/java/software/amazon/smithy/lsp/project/ProjectLoader.java b/src/main/java/software/amazon/smithy/lsp/project/ProjectLoader.java index ebca7684..77f1d26e 100644 --- a/src/main/java/software/amazon/smithy/lsp/project/ProjectLoader.java +++ b/src/main/java/software/amazon/smithy/lsp/project/ProjectLoader.java @@ -116,6 +116,10 @@ public static Result> load(Path root, ManagedFiles mana } ProjectConfig config = configResult.unwrap(); + if (config.buildFiles().isEmpty()) { + return Result.ok(Project.empty(root)); + } + Result, Exception> resolveResult = ProjectDependencyResolver.resolveDependencies(root, config); if (resolveResult.isErr()) { return Result.err(Collections.singletonList(resolveResult.unwrapErr())); diff --git a/src/test/java/software/amazon/smithy/lsp/SmithyLanguageServerTest.java b/src/test/java/software/amazon/smithy/lsp/SmithyLanguageServerTest.java index f80dad87..9395eada 100644 --- a/src/test/java/software/amazon/smithy/lsp/SmithyLanguageServerTest.java +++ b/src/test/java/software/amazon/smithy/lsp/SmithyLanguageServerTest.java @@ -966,7 +966,7 @@ public void invalidSyntaxDetachedProjectBecomesValid() throws Exception { assertThat(projectAndFile, notNullValue()); assertThat(projectAndFile.project().modelResult().isBroken(), is(true)); assertThat(projectAndFile.project().modelResult().getResult().isPresent(), is(true)); - assertThat(projectAndFile.project().smithyFiles().keySet(), hasItem(endsWith(filename))); + assertThat(projectAndFile.project().getAllSmithyFilePaths(), hasItem(endsWith(filename))); server.didChange(RequestBuilders.didChange() .uri(uri) @@ -984,7 +984,7 @@ public void invalidSyntaxDetachedProjectBecomesValid() throws Exception { assertThat(projectAndFile1, notNullValue()); assertThat(projectAndFile1.project().modelResult().isBroken(), is(false)); assertThat(projectAndFile1.project().modelResult().getResult().isPresent(), is(true)); - assertThat(projectAndFile1.project().smithyFiles().keySet(), hasItem(endsWith(filename))); + assertThat(projectAndFile1.project().getAllSmithyFilePaths(), hasItem(endsWith(filename))); assertThat(projectAndFile1.project().modelResult().unwrap(), hasShapeWithId("com.foo#Foo")); } @@ -1199,8 +1199,8 @@ public void loadsMultipleRoots() { assertThat(projectFoo, notNullValue()); assertThat(projectBar, notNullValue()); - assertThat(projectFoo.smithyFiles(), hasKey(endsWith("foo.smithy"))); - assertThat(projectBar.smithyFiles(), hasKey(endsWith("bar.smithy"))); + assertThat(projectFoo.getAllSmithyFilePaths(), hasItem(endsWith("foo.smithy"))); + assertThat(projectBar.getAllSmithyFilePaths(), hasItem(endsWith("bar.smithy"))); assertThat(projectFoo.modelResult(), SmithyMatchers.hasValue(SmithyMatchers.hasShapeWithId("com.foo#Foo"))); assertThat(projectBar.modelResult(), SmithyMatchers.hasValue(SmithyMatchers.hasShapeWithId("com.bar#Bar"))); @@ -1332,9 +1332,9 @@ public void multiRootAddingWatchedFile() throws Exception { Project projectFoo = server.getState().findProjectByRoot(workspaceFoo.getName()); Project projectBar = server.getState().findProjectByRoot(workspaceBar.getName()); - assertThat(projectFoo.smithyFiles(), hasKey(endsWith("main.smithy"))); - assertThat(projectBar.smithyFiles(), hasKey(endsWith("main.smithy"))); - assertThat(projectBar.smithyFiles(), hasKey(endsWith("other.smithy"))); + assertThat(projectFoo.getAllSmithyFilePaths(), hasItem(endsWith("main.smithy"))); + assertThat(projectBar.getAllSmithyFilePaths(), hasItem(endsWith("main.smithy"))); + assertThat(projectBar.getAllSmithyFilePaths(), hasItem(endsWith("other.smithy"))); assertThat(projectFoo.modelResult(), SmithyMatchers.hasValue(SmithyMatchers.hasShapeWithId("com.foo#Foo"))); assertThat(projectFoo.modelResult(), SmithyMatchers.hasValue(SmithyMatchers.hasShapeWithId("com.foo#Bar"))); @@ -1419,9 +1419,9 @@ public void multiRootChangingBuildFile() throws Exception { Project projectFoo = server.getState().findProjectByRoot(workspaceFoo.getName()); Project projectBar = server.getState().findProjectByRoot(workspaceBar.getName()); - assertThat(projectFoo.smithyFiles(), hasKey(endsWith("main.smithy"))); - assertThat(projectBar.smithyFiles(), hasKey(endsWith("main.smithy"))); - assertThat(projectBar.smithyFiles(), hasKey(endsWith("other.smithy"))); + assertThat(projectFoo.getAllSmithyFilePaths(), hasItem(endsWith("main.smithy"))); + assertThat(projectBar.getAllSmithyFilePaths(), hasItem(endsWith("main.smithy"))); + assertThat(projectBar.getAllSmithyFilePaths(), hasItem(endsWith("other.smithy"))); assertThat(projectFoo.modelResult(), hasValue(hasShapeWithId("com.foo#Foo"))); assertThat(projectBar.modelResult(), hasValue(hasShapeWithId("com.bar#Bar"))); @@ -1475,8 +1475,8 @@ public void addingWorkspaceFolder() throws Exception { Project projectFoo = server.getState().findProjectByRoot(workspaceFoo.getName()); Project projectBar = server.getState().findProjectByRoot(workspaceBar.getName()); - assertThat(projectFoo.smithyFiles(), hasKey(endsWith("foo.smithy"))); - assertThat(projectBar.smithyFiles(), hasKey(endsWith("bar.smithy"))); + assertThat(projectFoo.getAllSmithyFilePaths(), hasItem(endsWith("foo.smithy"))); + assertThat(projectBar.getAllSmithyFilePaths(), hasItem(endsWith("bar.smithy"))); assertThat(projectFoo.modelResult(), SmithyMatchers.hasValue(SmithyMatchers.hasShapeWithId("com.foo#Foo"))); assertThat(projectBar.modelResult(), SmithyMatchers.hasValue(SmithyMatchers.hasShapeWithId("com.bar#Bar"))); @@ -1528,8 +1528,8 @@ public void removingWorkspaceFolder() { Project projectFoo = server.getState().findProjectByRoot(workspaceFoo.getName()); Project projectBar = server.getState().findProjectByRoot(barUri); - assertThat(projectFoo.smithyFiles(), hasKey(endsWith("foo.smithy"))); - assertThat(projectBar.smithyFiles(), hasKey(endsWith("bar.smithy"))); + assertThat(projectFoo.getAllSmithyFilePaths(), hasItem(endsWith("foo.smithy"))); + assertThat(projectBar.getAllSmithyFilePaths(), hasItem(endsWith("bar.smithy"))); assertThat(projectFoo.modelResult(), hasValue(hasShapeWithId("com.foo#Foo"))); assertThat(projectBar.modelResult(), hasValue(hasShapeWithId("com.bar#Bar"))); diff --git a/src/test/java/software/amazon/smithy/lsp/SmithyMatchers.java b/src/test/java/software/amazon/smithy/lsp/SmithyMatchers.java index f5796236..6dd38cfd 100644 --- a/src/test/java/software/amazon/smithy/lsp/SmithyMatchers.java +++ b/src/test/java/software/amazon/smithy/lsp/SmithyMatchers.java @@ -42,7 +42,7 @@ public void describeMismatchSafely(ValidatedResult item, Description descript } public static Matcher hasShapeWithId(String id) { - return new CustomTypeSafeMatcher<>("a model with the shape id `" + id + "`") { + return new CustomTypeSafeMatcher<>("has shape id `" + id + "`") { @Override protected boolean matchesSafely(Model item) { return item.getShape(ShapeId.from(id)).isPresent(); @@ -59,7 +59,7 @@ public void describeMismatchSafely(Model model, Description description) { } public static Matcher eventWithMessage(Matcher message) { - return new CustomTypeSafeMatcher<>("has matching message") { + return new CustomTypeSafeMatcher<>("has message matching " + message.toString()) { @Override protected boolean matchesSafely(ValidationEvent item) { return message.matches(item.getMessage()); @@ -71,4 +71,13 @@ public void describeMismatchSafely(ValidationEvent event, Description descriptio } }; } + + public static Matcher eventWithId(Matcher id) { + return new CustomTypeSafeMatcher<>("has id matching " + id.toString()) { + @Override + protected boolean matchesSafely(ValidationEvent item) { + return id.matches(item.getId()); + } + }; + } } diff --git a/src/test/java/software/amazon/smithy/lsp/language/CompletionHandlerTest.java b/src/test/java/software/amazon/smithy/lsp/language/CompletionHandlerTest.java index b5d3e324..1982dec6 100644 --- a/src/test/java/software/amazon/smithy/lsp/language/CompletionHandlerTest.java +++ b/src/test/java/software/amazon/smithy/lsp/language/CompletionHandlerTest.java @@ -32,6 +32,7 @@ import software.amazon.smithy.lsp.project.IdlFile; import software.amazon.smithy.lsp.project.Project; import software.amazon.smithy.lsp.project.ProjectLoader; +import software.amazon.smithy.lsp.project.SmithyFile; public class CompletionHandlerTest { @Test @@ -1087,7 +1088,7 @@ private static List getCompItems(String text, Position... positi TestWorkspace workspace = TestWorkspace.singleModel(text); Project project = ProjectLoader.load(workspace.getRoot(), new ServerState()).unwrap(); String uri = workspace.getUri("main.smithy"); - IdlFile smithyFile = (IdlFile) project.getSmithyFile(uri); + IdlFile smithyFile = (IdlFile) (SmithyFile) project.getProjectFile(uri); List completionItems = new ArrayList<>(); CompletionHandler handler = new CompletionHandler(project, smithyFile); diff --git a/src/test/java/software/amazon/smithy/lsp/language/DefinitionHandlerTest.java b/src/test/java/software/amazon/smithy/lsp/language/DefinitionHandlerTest.java index 0dbed9c4..8f680cfa 100644 --- a/src/test/java/software/amazon/smithy/lsp/language/DefinitionHandlerTest.java +++ b/src/test/java/software/amazon/smithy/lsp/language/DefinitionHandlerTest.java @@ -333,7 +333,8 @@ private static void assertIsShapeDef( Location location, String expected ) { - SmithyFile smithyFile = result.handler.project.getSmithyFile(location.getUri()); + String uri = location.getUri(); + SmithyFile smithyFile = (SmithyFile) result.handler.project.getProjectFile(uri); assertThat(smithyFile, notNullValue()); int documentIndex = smithyFile.document().indexOfPosition(location.getRange().getStart()); @@ -367,7 +368,7 @@ private static GetLocationsResult getLocations(String text, Position... position TestWorkspace workspace = TestWorkspace.singleModel(text); Project project = ProjectLoader.load(workspace.getRoot(), new ServerState()).unwrap(); String uri = workspace.getUri("main.smithy"); - SmithyFile smithyFile = project.getSmithyFile(uri); + SmithyFile smithyFile = (SmithyFile) project.getProjectFile(uri); List locations = new ArrayList<>(); DefinitionHandler handler = new DefinitionHandler(project, (IdlFile) smithyFile); diff --git a/src/test/java/software/amazon/smithy/lsp/language/DocumentSymbolTest.java b/src/test/java/software/amazon/smithy/lsp/language/DocumentSymbolTest.java index ab2e521e..0242eab8 100644 --- a/src/test/java/software/amazon/smithy/lsp/language/DocumentSymbolTest.java +++ b/src/test/java/software/amazon/smithy/lsp/language/DocumentSymbolTest.java @@ -17,6 +17,7 @@ import software.amazon.smithy.lsp.project.IdlFile; import software.amazon.smithy.lsp.project.Project; import software.amazon.smithy.lsp.project.ProjectLoader; +import software.amazon.smithy.lsp.project.SmithyFile; public class DocumentSymbolTest { @Test @@ -50,7 +51,7 @@ private static List getDocumentSymbolNames(String text) { TestWorkspace workspace = TestWorkspace.singleModel(text); Project project = ProjectLoader.load(workspace.getRoot(), new ServerState()).unwrap(); String uri = workspace.getUri("main.smithy"); - IdlFile idlFile = (IdlFile) project.getSmithyFile(uri); + IdlFile idlFile = (IdlFile) (SmithyFile) project.getProjectFile(uri); List names = new ArrayList<>(); var handler = new DocumentSymbolHandler(idlFile.document(), idlFile.getParse().statements()); diff --git a/src/test/java/software/amazon/smithy/lsp/language/HoverHandlerTest.java b/src/test/java/software/amazon/smithy/lsp/language/HoverHandlerTest.java index 5f37e89e..7a22bc66 100644 --- a/src/test/java/software/amazon/smithy/lsp/language/HoverHandlerTest.java +++ b/src/test/java/software/amazon/smithy/lsp/language/HoverHandlerTest.java @@ -156,7 +156,7 @@ private static List getHovers(String text, Position... positions) { TestWorkspace workspace = TestWorkspace.singleModel(text); Project project = ProjectLoader.load(workspace.getRoot(), new ServerState()).unwrap(); String uri = workspace.getUri("main.smithy"); - SmithyFile smithyFile = project.getSmithyFile(uri); + SmithyFile smithyFile = (SmithyFile) project.getProjectFile(uri); List hover = new ArrayList<>(); HoverHandler handler = new HoverHandler(project, (IdlFile) smithyFile, Severity.WARNING); diff --git a/src/test/java/software/amazon/smithy/lsp/project/ProjectTest.java b/src/test/java/software/amazon/smithy/lsp/project/ProjectTest.java index 47400209..8d030bcb 100644 --- a/src/test/java/software/amazon/smithy/lsp/project/ProjectTest.java +++ b/src/test/java/software/amazon/smithy/lsp/project/ProjectTest.java @@ -10,24 +10,24 @@ import static org.hamcrest.CoreMatchers.hasItem; import static org.hamcrest.CoreMatchers.hasItems; import static org.hamcrest.CoreMatchers.is; -import static org.hamcrest.CoreMatchers.not; -import static org.hamcrest.CoreMatchers.nullValue; import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.allOf; import static org.hamcrest.Matchers.containsInAnyOrder; import static org.hamcrest.Matchers.empty; +import static org.hamcrest.Matchers.endsWith; import static org.hamcrest.Matchers.hasSize; +import static software.amazon.smithy.lsp.SmithyMatchers.eventWithId; import static software.amazon.smithy.lsp.SmithyMatchers.eventWithMessage; import static software.amazon.smithy.lsp.SmithyMatchers.hasShapeWithId; +import static software.amazon.smithy.lsp.SmithyMatchers.hasValue; import static software.amazon.smithy.lsp.UtilMatchers.anOptionalOf; import java.net.URISyntaxException; import java.net.URL; +import java.nio.file.Files; import java.nio.file.Path; import java.nio.file.Paths; import java.util.List; -import java.util.Set; -import java.util.logging.Logger; -import java.util.stream.Collectors; import org.junit.jupiter.api.Test; import software.amazon.smithy.lsp.ServerState; import software.amazon.smithy.lsp.TestWorkspace; @@ -37,12 +37,10 @@ import software.amazon.smithy.model.Model; import software.amazon.smithy.model.shapes.Shape; import software.amazon.smithy.model.shapes.ShapeId; -import software.amazon.smithy.model.shapes.ToShapeId; import software.amazon.smithy.model.traits.LengthTrait; import software.amazon.smithy.model.traits.PatternTrait; import software.amazon.smithy.model.traits.TagsTrait; import software.amazon.smithy.model.validation.Severity; -import software.amazon.smithy.model.validation.ValidationEvent; public class ProjectTest { @Test @@ -51,8 +49,8 @@ public void loadsFlatProject() { Project project = load(root).unwrap(); assertThat(project.root(), equalTo(root)); - assertThat(project.sources(), hasItem(root.resolve("main.smithy"))); - assertThat(project.imports(), empty()); + assertThat(project.config().sources(), hasItem(endsWith("main.smithy"))); + assertThat(project.config().imports(), empty()); assertThat(project.dependencies(), empty()); assertThat(project.modelResult().isBroken(), is(false)); assertThat(project.modelResult().unwrap(), hasShapeWithId("com.foo#Foo")); @@ -64,8 +62,8 @@ public void loadsProjectWithMavenDep() { Project project = load(root).unwrap(); assertThat(project.root(), equalTo(root)); - assertThat(project.sources(), hasItem(root.resolve("main.smithy"))); - assertThat(project.imports(), empty()); + assertThat(project.config().sources(), hasItem(endsWith("main.smithy"))); + assertThat(project.config().imports(), empty()); assertThat(project.dependencies(), hasSize(3)); assertThat(project.modelResult().isBroken(), is(false)); assertThat(project.modelResult().unwrap(), hasShapeWithId("com.foo#Foo")); @@ -77,10 +75,10 @@ public void loadsProjectWithSubdir() { Project project = load(root).unwrap(); assertThat(project.root(), equalTo(root)); - assertThat(project.sources(), hasItems( - root.resolve("model"), - root.resolve("model2"))); - assertThat(project.smithyFiles().keySet(), hasItems( + assertThat(project.config().sources(), hasItems( + endsWith("model"), + endsWith("model2"))); + assertThat(project.getAllSmithyFilePaths(), hasItems( equalTo(root.resolve("model/main.smithy").toString()), equalTo(root.resolve("model/subdir/sub.smithy").toString()), equalTo(root.resolve("model2/subdir2/sub2.smithy").toString()), @@ -97,13 +95,9 @@ public void loadsModelWithUnknownTrait() { Project project = load(root).unwrap(); assertThat(project.root(), equalTo(root)); - assertThat(project.sources(), hasItem(root.resolve("main.smithy"))); + assertThat(project.config().sources(), hasItem(endsWith("main.smithy"))); assertThat(project.modelResult().isBroken(), is(false)); // unknown traits don't break it - - List eventIds = project.modelResult().getValidationEvents().stream() - .map(ValidationEvent::getId) - .collect(Collectors.toList()); - assertThat(eventIds, hasItem(containsString("UnresolvedTrait"))); + assertThat(project.modelResult().getValidationEvents(), hasItem(eventWithId(containsString("UnresolvedTrait")))); assertThat(project.modelResult().getResult().isPresent(), is(true)); assertThat(project.modelResult().getResult().get(), hasShapeWithId("com.foo#Foo")); } @@ -114,27 +108,13 @@ public void loadsWhenModelHasInvalidSyntax() { Project project = load(root).unwrap(); assertThat(project.root(), equalTo(root)); - assertThat(project.sources(), hasItem(root.resolve("main.smithy"))); + assertThat(project.config().sources(), hasItem(endsWith("main.smithy"))); assertThat(project.modelResult().isBroken(), is(true)); - List eventIds = project.modelResult().getValidationEvents().stream() - .map(ValidationEvent::getId) - .collect(Collectors.toList()); - assertThat(eventIds, hasItem("Model")); - - assertThat(project.smithyFiles().keySet(), hasItem(containsString("main.smithy"))); - IdlFile main = (IdlFile) project.getSmithyFile(LspAdapter.toUri(root.resolve("main.smithy").toString())); - assertThat(main, not(nullValue())); - assertThat(main.document(), not(nullValue())); - assertThat(main.getParse().namespace().namespace(), equalTo("com.foo")); - assertThat(main.getParse().imports().imports(), empty()); - - assertThat(project.definedShapesByFile().keySet(), hasItem(main.path())); - Set mainShapes = project.definedShapesByFile().get(main.path()); - List shapeIds = mainShapes.stream() - .map(ToShapeId::toShapeId) - .map(ShapeId::toString) - .collect(Collectors.toList()); - assertThat(shapeIds, hasItems("com.foo#Foo", "com.foo#Foo$bar")); + assertThat(project.modelResult().getValidationEvents(), hasItem(eventWithId(equalTo("Model")))); + assertThat(project.modelResult(), hasValue(allOf( + hasShapeWithId("com.foo#Foo"), + hasShapeWithId("com.foo#Foo$bar")))); + assertThat(project.getAllSmithyFilePaths(), hasItem(containsString("main.smithy"))); } @Test @@ -142,33 +122,17 @@ public void loadsProjectWithMultipleNamespaces() { Path root = toPath(getClass().getResource("multiple-namespaces")); Project project = load(root).unwrap(); - assertThat(project.sources(), hasItem(root.resolve("model"))); + assertThat(project.config().sources(), hasItem(endsWith("model"))); assertThat(project.modelResult().getValidationEvents(), empty()); - assertThat(project.smithyFiles().keySet(), hasItems(containsString("a.smithy"), containsString("b.smithy"))); - - IdlFile a = (IdlFile) project.getSmithyFile(LspAdapter.toUri(root.resolve("model/a.smithy").toString())); - assertThat(a.document(), not(nullValue())); - assertThat(a.getParse().namespace().namespace(), equalTo("a")); - - assertThat(project.definedShapesByFile().keySet(), hasItem(a.path())); - Set aShapes = project.definedShapesByFile().get(a.path()); - List aShapeIds = aShapes.stream() - .map(ToShapeId::toShapeId) - .map(ShapeId::toString) - .collect(Collectors.toList()); - assertThat(aShapeIds, hasItems("a#Hello", "a#HelloInput", "a#HelloOutput")); - - IdlFile b = (IdlFile) project.getSmithyFile(LspAdapter.toUri(root.resolve("model/b.smithy").toString())); - assertThat(b.document(), not(nullValue())); - assertThat(b.getParse().namespace().namespace(), equalTo("b")); - - assertThat(project.definedShapesByFile().keySet(), hasItem(b.path())); - Set bShapes = project.definedShapesByFile().get(b.path()); - List bShapeIds = bShapes.stream() - .map(ToShapeId::toShapeId) - .map(ShapeId::toString) - .collect(Collectors.toList()); - assertThat(bShapeIds, hasItems("b#Hello", "b#HelloInput", "b#HelloOutput")); + assertThat(project.getAllSmithyFilePaths(), hasItems(containsString("a.smithy"), containsString("b.smithy"))); + + assertThat(project.modelResult(), hasValue(allOf( + hasShapeWithId("a#Hello"), + hasShapeWithId("a#HelloInput"), + hasShapeWithId("a#HelloOutput"), + hasShapeWithId("b#Hello"), + hasShapeWithId("b#HelloInput"), + hasShapeWithId("b#HelloOutput")))); } @Test @@ -178,8 +142,10 @@ public void loadsProjectWithExternalJars() { assertThat(result.isOk(), is(true)); Project project = result.unwrap(); - assertThat(project.sources(), containsInAnyOrder(root.resolve("test-traits.smithy"), root.resolve("test-validators.smithy"))); - assertThat(project.smithyFiles().keySet(), hasItems( + assertThat(project.config().sources(), containsInAnyOrder( + endsWith("test-traits.smithy"), + endsWith("test-validators.smithy"))); + assertThat(project.getAllSmithyFilePaths(), hasItems( containsString("test-traits.smithy"), containsString("test-validators.smithy"), containsString("smithy-test-traits.jar!/META-INF/smithy/smithy.test.json"), @@ -217,7 +183,7 @@ public void doesntFailLoadingProjectWithNonExistingSource() { Result> result = load(root); assertThat(result.isErr(), is(false)); - assertThat(result.unwrap().smithyFiles().size(), equalTo(1)); // still have the prelude + assertThat(result.unwrap().getAllSmithyFiles().size(), equalTo(1)); // still have the prelude } @@ -247,7 +213,7 @@ public void loadsProjectWithUnNormalizedDirs() { root.resolve("model"), root.resolve("model2"))); assertThat(project.imports(), hasItem(root.resolve("model3"))); - assertThat(project.smithyFiles().keySet(), hasItems( + assertThat(project.getAllSmithyFilePaths(), hasItems( equalTo(root.resolve("model/test-traits.smithy").toString()), equalTo(root.resolve("model/one.smithy").toString()), equalTo(root.resolve("model2/two.smithy").toString()), @@ -277,7 +243,7 @@ public void changeFileApplyingSimpleTrait() { assertThat(bar.expectTrait(LengthTrait.class).getMin(), anOptionalOf(equalTo(1L))); String uri = workspace.getUri("model-0.smithy"); - Document document = project.getDocument(uri); + Document document = project.getProjectFile(uri).document(); document.applyEdit(LspAdapter.point(document.end()), "\n"); project.updateModelWithoutValidating(uri); @@ -308,7 +274,7 @@ public void changeFileApplyingListTrait() { assertThat(bar.expectTrait(TagsTrait.class).getTags(), containsInAnyOrder("foo")); String uri = workspace.getUri("model-0.smithy"); - Document document = project.getDocument(uri); + Document document = project.getProjectFile(uri).document(); document.applyEdit(LspAdapter.point(document.end()), "\n"); project.updateModelWithoutValidating(uri); @@ -348,7 +314,7 @@ public void changeFileApplyingListTraitWithUnrelatedDependencies() { assertThat(baz.expectTrait(LengthTrait.class).getMin(), anOptionalOf(equalTo(1L))); String uri = workspace.getUri("model-0.smithy"); - Document document = project.getDocument(uri); + Document document = project.getProjectFile(uri).document(); document.applyEdit(LspAdapter.point(document.end()), "\n"); project.updateModelWithoutValidating(uri); @@ -389,7 +355,7 @@ public void changingFileApplyingListTraitWithRelatedDependencies() { assertThat(bar.expectTrait(LengthTrait.class).getMin(), anOptionalOf(equalTo(1L))); String uri = workspace.getUri("model-0.smithy"); - Document document = project.getDocument(uri); + Document document = project.getProjectFile(uri).document(); document.applyEdit(LspAdapter.point(document.end()), "\n"); project.updateModelWithoutValidating(uri); @@ -427,7 +393,7 @@ public void changingFileApplyingListTraitWithRelatedArrayTraitDependencies() { assertThat(bar.expectTrait(TagsTrait.class).getTags(), containsInAnyOrder("foo", "bar")); String uri = workspace.getUri("model-0.smithy"); - Document document = project.getDocument(uri); + Document document = project.getProjectFile(uri).document(); document.applyEdit(LspAdapter.point(document.end()), "\n"); project.updateModelWithoutValidating(uri); @@ -458,7 +424,7 @@ public void changingFileWithDependencies() { assertThat(foo.expectTrait(LengthTrait.class).getMin(), anOptionalOf(equalTo(1L))); String uri = workspace.getUri("model-0.smithy"); - Document document = project.getDocument(uri); + Document document = project.getProjectFile(uri).document(); document.applyEdit(LspAdapter.point(document.end()), "\n"); project.updateModelWithoutValidating(uri); @@ -489,7 +455,7 @@ public void changingFileWithArrayDependencies() { assertThat(foo.expectTrait(TagsTrait.class).getTags(), containsInAnyOrder("foo")); String uri = workspace.getUri("model-0.smithy"); - Document document = project.getDocument(uri); + Document document = project.getProjectFile(uri).document(); document.applyEdit(LspAdapter.point(document.end()), "\n"); project.updateModelWithoutValidating(uri); @@ -521,7 +487,7 @@ public void changingFileWithMixedArrayDependencies() { assertThat(foo.expectTrait(TagsTrait.class).getTags(), containsInAnyOrder("foo", "foo")); String uri = workspace.getUri("model-0.smithy"); - Document document = project.getDocument(uri); + Document document = project.getProjectFile(uri).document(); document.applyEdit(LspAdapter.point(document.end()), "\n"); project.updateModelWithoutValidating(uri); @@ -560,18 +526,7 @@ public void changingFileWithArrayDependenciesWithDependencies() { assertThat(bar.expectTrait(LengthTrait.class).getMin(), anOptionalOf(equalTo(1L))); String uri = workspace.getUri("model-0.smithy"); - Document document = project.getDocument(uri); - if (document == null) { - String smithyFilesPaths = String.join(System.lineSeparator(), project.smithyFiles().keySet()); - String smithyFilesUris = project.smithyFiles().keySet().stream() - .map(LspAdapter::toUri) - .collect(Collectors.joining(System.lineSeparator())); - Logger logger = Logger.getLogger(getClass().getName()); - logger.severe("Not found uri: " + uri); - logger.severe("Not found path: " + LspAdapter.toPath(uri)); - logger.severe("PATHS: " + smithyFilesPaths); - logger.severe("URIS: " + smithyFilesUris); - } + Document document = project.getProjectFile(uri).document(); document.applyEdit(LspAdapter.point(document.end()), "\n"); project.updateModelWithoutValidating(uri); @@ -611,7 +566,7 @@ public void removingSimpleApply() { assertThat(bar.expectTrait(LengthTrait.class).getMin(), anOptionalOf(equalTo(1L))); String uri = workspace.getUri("model-0.smithy"); - Document document = project.getDocument(uri); + Document document = project.getProjectFile(uri).document(); document.applyEdit(LspAdapter.lineSpan(2, 0, document.lineEnd(2)), ""); project.updateModelWithoutValidating(uri); @@ -647,7 +602,7 @@ public void removingArrayApply() { assertThat(bar.expectTrait(TagsTrait.class).getTags(), containsInAnyOrder("foo", "bar")); String uri = workspace.getUri("model-0.smithy"); - Document document = project.getDocument(uri); + Document document = project.getProjectFile(uri).document(); document.applyEdit(LspAdapter.lineSpan(2, 0, document.lineEnd(2)), ""); project.updateModelWithoutValidating(uri); @@ -657,6 +612,14 @@ public void removingArrayApply() { assertThat(bar.expectTrait(TagsTrait.class).getTags(), containsInAnyOrder("bar")); } + @Test + public void loadsEmptyProjectWhenThereAreNoConfigFiles() throws Exception { + Path root = Files.createTempDirectory("foo"); + Project project = load(root).unwrap(); + + assertThat(project.type(), equalTo(Project.Type.EMPTY)); + } + private static Result> load(Path root) { return ProjectLoader.load(root, new ServerState()); } From 3927e8a7baa964f1972a2fcb6d8e0518b5535551 Mon Sep 17 00:00:00 2001 From: Miles Ziemer Date: Thu, 2 Jan 2025 13:40:51 -0500 Subject: [PATCH 9/9] Get rid of extraneous Document methods --- .../amazon/smithy/lsp/document/Document.java | 156 +++--------------- .../amazon/smithy/lsp/LspMatchers.java | 13 +- .../smithy/lsp/document/DocumentTest.java | 102 +----------- 3 files changed, 35 insertions(+), 236 deletions(-) diff --git a/src/main/java/software/amazon/smithy/lsp/document/Document.java b/src/main/java/software/amazon/smithy/lsp/document/Document.java index 74c3f0c7..d5f7e7ae 100644 --- a/src/main/java/software/amazon/smithy/lsp/document/Document.java +++ b/src/main/java/software/amazon/smithy/lsp/document/Document.java @@ -146,7 +146,6 @@ public int indexOfPosition(int line, int character) { return -1; } - int idx = startLineIdx + character; if (line == lastLine()) { if (idx >= buffer.length()) { @@ -196,9 +195,7 @@ public Range rangeBetween(int start, int end) { Position endPos; if (end == length()) { - int lastLine = lastLine(); - int lastCol = length() - lineIndices[lastLine]; - endPos = new Position(lastLine, lastCol); + endPos = end(); } else { endPos = positionAtIndex(end); } @@ -234,9 +231,7 @@ public int lastLine() { * @return The end position of this document */ public Position end() { - return new Position( - lineIndices.length - 1, - buffer.length() - lineIndices[lineIndices.length - 1]); + return new Position(lastLine(), lastColExclusive()); } /** @@ -266,111 +261,6 @@ public CharSequence borrowText() { return buffer; } - /** - * @param range The range to borrow the text of - * @return A reference to the text in this document within the given {@code range} - * or {@code null} if the range is out of bounds - */ - public CharBuffer borrowRange(Range range) { - int startLine = range.getStart().getLine(); - int startChar = range.getStart().getCharacter(); - int endLine = range.getEnd().getLine(); - int endChar = range.getEnd().getCharacter(); - - // TODO: Maybe make this return the whole thing, thing up to an index, or thing after an - // index if one of the indicies is out of bounds. - int startLineIdx = indexOfLine(startLine); - int endLineIdx = indexOfLine(endLine); - if (startLineIdx < 0 || endLineIdx < 0) { - return null; - } - - int startIdx = startLineIdx + startChar; - int endIdx = endLineIdx + endChar; - if (startIdx > buffer.length() || endIdx > buffer.length()) { - return null; - } - - return CharBuffer.wrap(buffer, startIdx, endIdx); - } - - /** - * @param position The position within the token to borrow - * @return A reference to the token that the given {@code position} is - * within, or {@code null} if the position is not within a token - */ - public CharBuffer borrowToken(Position position) { - int idx = indexOfPosition(position); - if (idx < 0) { - return null; - } - - char atIdx = buffer.charAt(idx); - // Not a token - if (!Character.isLetterOrDigit(atIdx) && atIdx != '_') { - return null; - } - - int startIdx = idx; - while (startIdx >= 0) { - char c = buffer.charAt(startIdx); - if (Character.isLetterOrDigit(c) || c == '_') { - startIdx--; - } else { - break; - } - } - - int endIdx = idx; - while (endIdx < buffer.length()) { - char c = buffer.charAt(endIdx); - if (Character.isLetterOrDigit(c) || c == '_') { - endIdx++; - } else { - break; - } - } - - return CharBuffer.wrap(buffer, startIdx + 1, endIdx); - } - - /** - * @param line The line to borrow - * @return A reference to the text in the given line, or {@code null} if - * the line doesn't exist - */ - public CharBuffer borrowLine(int line) { - if (line >= lineIndices.length || line < 0) { - return null; - } - - int lineStart = indexOfLine(line); - if (line + 1 >= lineIndices.length) { - return CharBuffer.wrap(buffer, lineStart, buffer.length()); - } - - return CharBuffer.wrap(buffer, lineStart, indexOfLine(line + 1)); - } - - /** - * @param start The index of the start of the span to borrow - * @param end The end of the index of the span to borrow (exclusive) - * @return A reference to the text within the indicies {@code start} and - * {@code end}, or {@code null} if the span is out of bounds or start > end - */ - public CharBuffer borrowSpan(int start, int end) { - if (start < 0 || end < 0) { - return null; - } - - // end is exclusive - if (end > buffer.length() || start > end) { - return null; - } - - return CharBuffer.wrap(buffer, start, end); - } - /** * @return A copy of the text of this document */ @@ -384,12 +274,20 @@ public String copyText() { * or {@code null} if the range is out of bounds */ public String copyRange(Range range) { - CharBuffer borrowed = borrowRange(range); - if (borrowed == null) { - return null; + int start = indexOfPosition(range.getStart()); + + int end; + Position endPosition = range.getEnd(); + if (endPosition.getLine() == lastLine() && endPosition.getCharacter() == lastColExclusive()) { + end = length(); + } else { + end = indexOfPosition(range.getEnd()); } + return copySpan(start, end); + } - return borrowed.toString(); + private int lastColExclusive() { + return length() - lineIndices[lastLine()]; } /** @@ -479,18 +377,11 @@ public DocumentId copyDocumentId(Position position) { } // We go past the start and end in each loop, so startIdx is before the start character, and endIdx - // is after the end character. + // is after the end character. Since end is exclusive (both for creating the buffer and getting the + // range) we can leave it. int startCharIdx = startIdx + 1; - int endCharIdx = endIdx - 1; - - // For creating the buffer and the range, the start is inclusive, and the end is exclusive. - CharBuffer wrapped = CharBuffer.wrap(buffer, startCharIdx, endCharIdx + 1); - Position start = positionAtIndex(startCharIdx); - // However, we can't get the position for an index that may be out of bounds, so we need to make - // the end position exclusive manually. - Position end = positionAtIndex(endCharIdx); - end.setCharacter(end.getCharacter() + 1); - Range range = new Range(start, end); + CharBuffer wrapped = CharBuffer.wrap(buffer, startCharIdx, endIdx); + Range range = rangeBetween(startCharIdx, endIdx); return new DocumentId(type, wrapped, range); } @@ -505,11 +396,16 @@ private static boolean isIdChar(char c) { * {@code end}, or {@code null} if the span is out of bounds or start > end */ public String copySpan(int start, int end) { - CharBuffer borrowed = borrowSpan(start, end); - if (borrowed == null) { + if (start < 0 || end < 0) { return null; } - return borrowed.toString(); + + // end is exclusive + if (end > buffer.length() || start > end) { + return null; + } + + return CharBuffer.wrap(buffer, start, end).toString(); } /** diff --git a/src/test/java/software/amazon/smithy/lsp/LspMatchers.java b/src/test/java/software/amazon/smithy/lsp/LspMatchers.java index cab974b4..dbe118d1 100644 --- a/src/test/java/software/amazon/smithy/lsp/LspMatchers.java +++ b/src/test/java/software/amazon/smithy/lsp/LspMatchers.java @@ -89,23 +89,20 @@ public void describeMismatchSafely(Collection item, Description descri } public static Matcher hasText(Document document, Matcher expected) { - return new CustomTypeSafeMatcher<>("text in range") { + return new CustomTypeSafeMatcher<>("text in range " + expected.toString()) { @Override protected boolean matchesSafely(Range item) { - CharSequence borrowed = document.borrowRange(item); - if (borrowed == null) { - return false; - } - return expected.matches(borrowed.toString()); + String actual = document.copyRange(item); + return expected.matches(actual); } @Override public void describeMismatchSafely(Range range, Description description) { - if (document.borrowRange(range) == null) { + if (document.copyRange(range) == null) { description.appendText("text was null"); } else { description.appendDescriptionOf(expected) - .appendText("was " + document.borrowRange(range).toString()); + .appendText("was " + document.copyRange(range)); } } }; diff --git a/src/test/java/software/amazon/smithy/lsp/document/DocumentTest.java b/src/test/java/software/amazon/smithy/lsp/document/DocumentTest.java index b3acf480..a3424b8c 100644 --- a/src/test/java/software/amazon/smithy/lsp/document/DocumentTest.java +++ b/src/test/java/software/amazon/smithy/lsp/document/DocumentTest.java @@ -255,88 +255,11 @@ public void getsEnd() { } @Test - public void borrowsToken() { - String s = "abc\n" + - "def"; - Document document = makeDocument(s); - - CharSequence token = document.borrowToken(new Position(0, 2)); - - assertThat(token, string("abc")); - } - - @Test - public void borrowsTokenWithNoWs() { - String s = "abc"; - Document document = makeDocument(s); - - CharSequence token = document.borrowToken(new Position(0, 1)); - - assertThat(token, string("abc")); - } - - @Test - public void borrowsTokenAtStart() { - String s = "abc\n" + - "def"; - Document document = makeDocument(s); - - CharSequence token = document.borrowToken(new Position(0, 0)); - - assertThat(token, string("abc")); - } - - @Test - public void borrowsTokenAtEnd() { - String s = "abc\n" + - "def"; - Document document = makeDocument(s); + public void foo() { + Document a = makeDocument("abc"); + Document b = makeDocument("def\n"); - CharSequence token = document.borrowToken(new Position(1, 2)); - - assertThat(token, string("def")); - } - - @Test - public void borrowsTokenAtBoundaryStart() { - String s = "a bc d"; - Document document = makeDocument(s); - - CharSequence token = document.borrowToken(new Position(0, 2)); - - assertThat(token, string("bc")); - } - - @Test - public void borrowsTokenAtBoundaryEnd() { - String s = "a bc d"; - Document document = makeDocument(s); - - CharSequence token = document.borrowToken(new Position(0, 3)); - - assertThat(token, string("bc")); - } - - @Test - public void doesntBorrowNonToken() { - String s = "abc def"; - Document document = makeDocument(s); - - CharSequence token = document.borrowToken(new Position(0, 3)); - - assertThat(token, nullValue()); - } - - @Test - public void borrowsLine() { - Document document = makeDocument("abc\n\ndef"); - - assertThat(makeDocument("").borrowLine(0), string("")); - assertThat(document.borrowLine(0), string(safeString("abc\n"))); - assertThat(document.borrowLine(1), string(safeString("\n"))); - assertThat(document.borrowLine(2), string("def")); - assertThat(document.borrowLine(-1), nullValue()); - assertThat(document.borrowLine(3), nullValue()); + System.out.println(); } @Test @@ -370,23 +293,6 @@ public void getsLastIndexOf() { assertThat(document.lastIndexOf(" ", safeIndex(8, 1)), is(-1)); // not found } - @Test - public void borrowsSpan() { - Document empty = makeDocument(""); - Document line = makeDocument("abc"); - Document multi = makeDocument("abc\ndef\n\n"); - - assertThat(empty.borrowSpan(0, 1), nullValue()); // empty - assertThat(line.borrowSpan(-1, 1), nullValue()); // negative - assertThat(line.borrowSpan(0, 0), string("")); // empty - assertThat(line.borrowSpan(0, 1), string("a")); // one - assertThat(line.borrowSpan(0, 3), string("abc")); // all - assertThat(line.borrowSpan(0, 4), nullValue()); // oob - assertThat(multi.borrowSpan(0, safeIndex(4, 1)), string(safeString("abc\n"))); // with newline - assertThat(multi.borrowSpan(3, safeIndex(5, 1)), string(safeString("\nd"))); // inner - assertThat(multi.borrowSpan(safeIndex(5, 1), safeIndex(9, 3)), string(safeString("ef\n\n"))); // up to end - } - @Test public void getsLineOfIndex() { Document empty = makeDocument("");