Skip to content
New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Process parse errors (including error nodes) in TextDocumentState #492

Merged
merged 5 commits into from
Oct 30, 2024
Merged

Process parse errors (including error nodes) in TextDocumentState #492

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
a9d5a06
Move parse error processing (including error nodes) completely to `Te…
sungshik Oct 25, 2024
770561e
Merge branch 'error-recovery/rascal' into error-recovery/rascal-diagn…
sungshik Oct 25, 2024
aeb1a9f
Refine parse error messages
sungshik Oct 25, 2024
90ab10c
Remove unused imports
sungshik Oct 28, 2024
d86c659
Move class `Debouncer` to its own file
sungshik Oct 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
126 changes: 99 additions & 27 deletions rascal-lsp/src/main/java/org/rascalmpl/vscode/lsp/TextDocumentState.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,27 +27,39 @@
package org.rascalmpl.vscode.lsp;

import java.time.Duration;
import java.util.ArrayList;
import java.util.List;
import java.util.concurrent.atomic.AtomicBoolean;
import java.util.concurrent.atomic.AtomicReference;
import java.util.concurrent.atomic.AtomicStampedReference;
import java.util.List;
import java.util.concurrent.CompletableFuture;
import java.util.concurrent.CompletionException;
import java.util.concurrent.Executor;
import java.util.concurrent.TimeUnit;
import java.util.function.BiFunction;
import java.util.function.Function;
import java.util.function.Supplier;

import org.apache.logging.log4j.LogManager;
import org.apache.logging.log4j.Logger;
import org.checkerframework.checker.nullness.qual.MonotonicNonNull;
import org.checkerframework.checker.nullness.qual.Nullable;
import org.eclipse.lsp4j.Diagnostic;
import org.eclipse.lsp4j.DiagnosticSeverity;
import org.eclipse.lsp4j.Position;
import org.eclipse.lsp4j.Range;
import org.rascalmpl.library.util.ErrorRecovery;
import org.rascalmpl.parser.gtd.exception.ParseError;
import org.rascalmpl.values.RascalValueFactory;
import org.rascalmpl.values.ValueFactoryFactory;
import org.rascalmpl.values.parsetrees.ITree;
import org.rascalmpl.vscode.lsp.util.Diagnostics;
import org.rascalmpl.vscode.lsp.util.Versioned;
import org.rascalmpl.vscode.lsp.util.locations.ColumnMaps;

import io.usethesource.vallang.IList;
import io.usethesource.vallang.ISourceLocation;
import io.usethesource.vallang.IValue;

/**
* TextDocumentState encapsulates the current contents of every open file editor,
Expand All @@ -59,30 +71,32 @@
* and ParametricTextDocumentService.
*/
public class TextDocumentState {

private static final ErrorRecovery RECOVERY =
new ErrorRecovery((RascalValueFactory) ValueFactoryFactory.getValueFactory());
private static final Logger logger = LogManager.getLogger(TextDocumentState.class);

private final BiFunction<ISourceLocation, String, CompletableFuture<ITree>> parser;
private final ISourceLocation location;
private final ColumnMaps columns;

@SuppressWarnings("java:S3077") // Visibility of writes is enough
private volatile Update current;
private final Debouncer<Versioned<ITree>> currentTreeAsyncDebouncer;
private final Debouncer<Update> currentAsyncParseDebouncer;
sungshik marked this conversation as resolved.
Show resolved Hide resolved

private final AtomicReference<@MonotonicNonNull Versioned<ITree>> lastWithoutErrors;
private final AtomicReference<@MonotonicNonNull Versioned<ITree>> last;

public TextDocumentState(
BiFunction<ISourceLocation, String, CompletableFuture<ITree>> parser,
ISourceLocation location, int initialVersion, String initialContent) {
ISourceLocation location, ColumnMaps columns,
int initialVersion, String initialContent) {

this.parser = parser;
this.location = location;
this.columns = columns;

this.current = new Update(initialVersion, initialContent);
this.currentTreeAsyncDebouncer = new Debouncer<>(50,
this::getCurrentTreeAsyncIfParsing, this::getCurrentTreeAsync);
this.currentAsyncParseDebouncer = new Debouncer<>(50,
this::getCurrentAsyncIfParsing,
this::parseAndGetCurrentAsync);

this.lastWithoutErrors = new AtomicReference<>();
this.last = new AtomicReference<>();
Expand All @@ -95,7 +109,7 @@ public ISourceLocation getLocation() {
public void update(int version, String content) {
current = new Update(version, content);
// The creation of the `Update` object doesn't trigger the parser yet.
// This happens only when the tree is requested.
// This happens only when the tree or diagnostics are requested.
}

public Versioned<String> getCurrentContent() {
Expand All @@ -107,17 +121,21 @@ public CompletableFuture<Versioned<ITree>> getCurrentTreeAsync() {
}

public CompletableFuture<Versioned<ITree>> getCurrentTreeAsync(Duration delay) {
return currentTreeAsyncDebouncer.get(delay);
return currentAsyncParseDebouncer
.get(delay)
.thenApply(Update::getTreeAsync)
.thenCompose(Function.identity());
sungshik marked this conversation as resolved.
Show resolved Hide resolved
}

public @Nullable CompletableFuture<Versioned<ITree>> getCurrentTreeAsyncIfParsing() {
var update = current;
return update.isParsing() ? update.getTreeAsync() : null;
public CompletableFuture<Versioned<List<Diagnostic>>> getCurrentDiagnosticsAsync() {
return current.getDiagnosticsAsync(); // Triggers the parser
}

public CompletableFuture<Versioned<List<Diagnostic>>> getCurrentDiagnostics() {
throw new UnsupportedOperationException();
// TODO: In a separate PR
public CompletableFuture<Versioned<List<Diagnostic>>> getCurrentDiagnosticsAsync(Duration delay) {
return currentAsyncParseDebouncer
.get(delay)
.thenApply(Update::getDiagnosticsAsync)
.thenCompose(Function.identity());
}

public @MonotonicNonNull Versioned<ITree> getLastTree() {
Expand All @@ -128,16 +146,29 @@ public CompletableFuture<Versioned<List<Diagnostic>>> getCurrentDiagnostics() {
return lastWithoutErrors.get();
}

private @Nullable CompletableFuture<Update> getCurrentAsyncIfParsing() {
var update = current;
return update.isParsing() ? CompletableFuture.completedFuture(update) : null;
}

private CompletableFuture<Update> parseAndGetCurrentAsync() {
var update = current;
update.parseIfNotParsing();
return CompletableFuture.completedFuture(update);
}

private class Update {
private final int version;
private final String content;
private final CompletableFuture<Versioned<ITree>> treeAsync;
private final CompletableFuture<Versioned<List<Diagnostic>>> diagnosticsAsync;
private final AtomicBoolean parsing;

public Update(int version, String content) {
this.version = version;
this.content = content;
this.treeAsync = new CompletableFuture<>();
this.diagnosticsAsync = new CompletableFuture<>();
this.parsing = new AtomicBoolean(false);
}

Expand All @@ -150,6 +181,11 @@ public CompletableFuture<Versioned<ITree>> getTreeAsync() {
return treeAsync;
}

public CompletableFuture<Versioned<List<Diagnostic>>> getDiagnosticsAsync() {
parseIfNotParsing();
return diagnosticsAsync;
}

public boolean isParsing() {
return parsing.get();
}
Expand All @@ -158,22 +194,58 @@ private void parseIfNotParsing() {
if (parsing.compareAndSet(false, true)) {
parser
.apply(location, content)
.thenApply(t -> new Versioned<>(version, t))
.whenComplete((t, error) -> {
if (t != null) {
var errors = RECOVERY.findAllErrors(t.get());
if (errors.isEmpty()) {
Versioned.replaceIfNewer(lastWithoutErrors, t);
.whenComplete((t, e) -> {

// Prepare result values for futures
var tree = new Versioned<>(version, t);
Copy link

Choose a reason for hiding this comment

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

As a <tree,diagnostics> pair seem to belong together (not suprisingly), would ik make sense to have one Versioned object with a version and a pair of <tree,diagnostics>?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In principle, yes, it would. However, in many places in the project (including the Parametric... classes), a Versioned<ITree> is currently expected, so it seems quite an invasive change to supply a Versioned<Pair<ITree,List<Diagnostic>> instead. I'm not worried about the complicated type parameter btw (i.e., we could hide the structure with an auxiliary class). But it's just a lot of changes to places that we don't really need/hope to touch right now 😉.

At the same time, I imagine the reporting of diagnostics can be streamlined a bit more if the diagnostics become accessible in some places where currently there's only a parse tree. Tt requires a bit more thought, though. I think I'd prefer to do that in a separate PR.

var diagnostics = new Versioned<>(version, toDiagnostics(t, e));

// Complete future to get the tree
if (t == null) {
treeAsync.completeExceptionally(e);
} else {
treeAsync.complete(tree);
Versioned.replaceIfNewer(last, tree);
if (diagnostics.get().isEmpty()) {
Versioned.replaceIfNewer(lastWithoutErrors, tree);
}
Versioned.replaceIfNewer(last, t);
treeAsync.complete(t);
}
if (error != null) {
treeAsync.completeExceptionally(error);
}

// Complete future to get diagnostics
diagnosticsAsync.complete(diagnostics);
});
}
}

private List<Diagnostic> toDiagnostics(ITree tree, Throwable excp) {
sungshik marked this conversation as resolved.
Show resolved Hide resolved
List<Diagnostic> parseErrors = new ArrayList<>();

if (excp instanceof CompletionException) {
excp = excp.getCause();
}

if (excp instanceof ParseError) {
parseErrors.add(Diagnostics.translateDiagnostic((ParseError)excp, columns));
} else if (excp != null) {
logger.error("Parsing crashed", excp);
parseErrors.add(new Diagnostic(
new Range(new Position(0,0), new Position(0,1)),
"Parsing failed: " + excp.getMessage(),
DiagnosticSeverity.Error,
"Rascal Parser"));
}

if (tree != null) {
RascalValueFactory valueFactory = (RascalValueFactory) ValueFactoryFactory.getValueFactory();
IList errors = new ErrorRecovery(valueFactory).findAllErrors(tree);
for (IValue error : errors) {
ITree errorTree = (ITree) error;
parseErrors.add(Diagnostics.translateErrorRecoveryDiagnostic(errorTree, columns));
}
}

return parseErrors;
}
}
}

Expand Down
4 changes: 2 additions & 2 deletions ...-lsp/src/main/java/org/rascalmpl/vscode/lsp/parametric/ParametricTextDocumentService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -521,7 +521,7 @@ private ParametricFileFacts facts(String doc) {

private TextDocumentState open(TextDocumentItem doc) {
return files.computeIfAbsent(Locations.toLoc(doc),
l -> new TextDocumentState(contributions(doc)::parseSourceFile, l, doc.getVersion(), doc.getText())
l -> new TextDocumentState(contributions(doc)::parseSourceFile, l, columns, doc.getVersion(), doc.getText())
);
}

Expand Down Expand Up @@ -638,7 +638,7 @@ public CompletableFuture<List<Either<Command, CodeAction>>> codeAction(CodeActio

private CompletableFuture<IList> computeCodeActions(final ILanguageContributions contribs, final int startLine, final int startColumn, ITree tree) {
IList focus = TreeSearch.computeFocusList(tree, startLine, startColumn);

if (!focus.isEmpty()) {
return contribs.codeActions(focus).get();
}
Expand Down
43 changes: 6 additions & 37 deletions rascal-lsp/src/main/java/org/rascalmpl/vscode/lsp/rascal/RascalTextDocumentService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -183,7 +183,7 @@ public void connect(LanguageClient client) {
public void didOpen(DidOpenTextDocumentParams params) {
logger.debug("Open file: {}", params.getTextDocument());
TextDocumentState file = open(params.getTextDocument());
handleParsingErrors(file);
handleParsingErrors(file, file.getCurrentDiagnosticsAsync()); // No debounce
}

@Override
Expand Down Expand Up @@ -215,51 +215,20 @@ private TextDocumentState updateContents(VersionedTextDocumentIdentifier doc, St
TextDocumentState file = getFile(doc);
logger.trace("New contents for {}", doc);
file.update(doc.getVersion(), newContents);
handleParsingErrors(file, file.getCurrentTreeAsync(Duration.ofMillis(800)));
handleParsingErrors(file, file.getCurrentDiagnosticsAsync(Duration.ofMillis(800)));
return file;
}

private void handleParsingErrors(TextDocumentState file, CompletableFuture<Versioned<ITree>> futureTree) {
futureTree.handle((tree, excp) -> {
List<Diagnostic> parseErrors = new ArrayList<>();

if (excp instanceof CompletionException) {
excp = excp.getCause();
}

if (excp instanceof ParseError) {
parseErrors.add(Diagnostics.translateDiagnostic((ParseError)excp, columns));
} else if (excp != null) {
logger.error("Parsing crashed", excp);
parseErrors.add(new Diagnostic(
new Range(new Position(0,0), new Position(0,1)),
"Parsing failed: " + excp.getMessage(),
DiagnosticSeverity.Error,
"Rascal Parser"));
}

if (tree != null) {
RascalValueFactory valueFactory = (RascalValueFactory) ValueFactoryFactory.getValueFactory();
IList errors = new ErrorRecovery(valueFactory).findAllErrors(tree.get());
for (IValue error : errors) {
ITree errorTree = (ITree) error;
parseErrors.add(Diagnostics.translateErrorRecoveryDiagnostic(errorTree, columns));
}
}

sungshik marked this conversation as resolved.
Show resolved Hide resolved
private void handleParsingErrors(TextDocumentState file, CompletableFuture<Versioned<List<Diagnostic>>> diagnosticsAsync) {
diagnosticsAsync.thenAccept(diagnostics -> {
List<Diagnostic> parseErrors = diagnostics.get();
logger.trace("Finished parsing tree, reporting new parse errors: {} for: {}", parseErrors, file.getLocation());
if (facts != null) {
facts.reportParseErrors(file.getLocation(), parseErrors);
}
return null;
});
}

private void handleParsingErrors(TextDocumentState file) {
handleParsingErrors(file,file.getCurrentTreeAsync());
}


@Override
public CompletableFuture<Either<List<? extends Location>, List<? extends LocationLink>>> definition(DefinitionParams params) {
logger.debug("Definition: {} at {}", params.getTextDocument(), params.getPosition());
Expand Down Expand Up @@ -342,7 +311,7 @@ private static <T> T last(List<T> l) {

private TextDocumentState open(TextDocumentItem doc) {
return documents.computeIfAbsent(Locations.toLoc(doc),
l -> new TextDocumentState((loc, input) -> rascalServices.parseSourceFile(loc, input), l, doc.getVersion(), doc.getText()));
l -> new TextDocumentState((loc, input) -> rascalServices.parseSourceFile(loc, input), l, columns, doc.getVersion(), doc.getText()));
}

private TextDocumentState getFile(TextDocumentIdentifier doc) {
Expand Down
5 changes: 3 additions & 2 deletions rascal-lsp/src/main/java/org/rascalmpl/vscode/lsp/util/Diagnostics.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,13 +73,14 @@ public static <K, V> Map<K, List<V>> groupByKey(Stream<Entry<K, V>> diagnostics)
}

public static Diagnostic translateDiagnostic(ParseError e, ColumnMaps cm) {
return new Diagnostic(toRange(e, cm), e.getMessage(), DiagnosticSeverity.Error, "parser");
var message = e.getMessage() + " (irrecoverable)";
sungshik marked this conversation as resolved.
Show resolved Hide resolved
return new Diagnostic(toRange(e, cm), message, DiagnosticSeverity.Error, "parser");
}

public static Diagnostic translateErrorRecoveryDiagnostic(ITree errorTree, ColumnMaps cm) {
IList args = TreeAdapter.getArgs(errorTree);
ITree skipped = (ITree) args.get(args.size()-1);
return new Diagnostic(toRange(skipped, cm), "Parse error", DiagnosticSeverity.Error, "parser");
return new Diagnostic(toRange(skipped, cm), "Parse error (recoverable)", DiagnosticSeverity.Error, "parser");
}

public static Diagnostic translateRascalParseError(IValue e, ColumnMaps cm) {
Expand Down
Loading