Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Add error recovery #94

Draft
wants to merge 22 commits into
base: dev
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
72 changes: 64 additions & 8 deletions src/main/java/net/fabricmc/mappingio/MappingReader.java
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,9 @@

import org.jetbrains.annotations.Nullable;

import net.fabricmc.mappingio.format.ErrorCollector;
import net.fabricmc.mappingio.format.ErrorCollector.Severity;
import net.fabricmc.mappingio.format.ErrorCollector.ThrowingErrorCollector;
import net.fabricmc.mappingio.format.MappingFormat;
import net.fabricmc.mappingio.format.enigma.EnigmaDirReader;
import net.fabricmc.mappingio.format.enigma.EnigmaFileReader;
Expand Down Expand Up @@ -184,10 +187,23 @@ public static List<String> getNamespaces(Reader reader, MappingFormat format) th
* @param visitor The receiving visitor.
* @throws IOException If the format can't be detected or reading fails.
*/
@Deprecated
Copy link
Member

Choose a reason for hiding this comment

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

Why deprecate this, I think by default it should fail when it reads a malformed mapping file?

Copy link
Member Author

@NebelNidas NebelNidas Apr 18, 2024

Choose a reason for hiding this comment

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

To not have a dozen different overloads. You can use the ErrorSink#noOp() or ErrorSink#throwingOnSeverity(...) factory methods, just like you'd use ProgressListener.none() in Enigma

Copy link

Choose a reason for hiding this comment

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

Is ther why new ThrowingErrorSink(Severity.WARNING) is a bad default severity? Overloads is like a way to imply default arguments.

Copy link
Member Author

@NebelNidas NebelNidas Apr 18, 2024

Choose a reason for hiding this comment

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

It hinders readability and is a pain to maintain. We already have up to five overloads per mapping reader, imagine we were to add an additional argument in the future, it would double the count again. MappingReader#read already has eight overloads, which I'm pretty sure starts getting confusing for consumers.

At least the latter one would be remedied by #56, which gets rid of some of the overloads by removing implicit format detection.

Copy link

Choose a reason for hiding this comment

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

If we have too many optional arguments, we might just use builders.

public static void read(Path path, MappingVisitor visitor) throws IOException {
read(path, null, visitor);
}

/**
* Tries to detect the format of the given path and read it.
*
* @param path The path to read from. Can be a file or a directory.
* @param visitor The receiving visitor.
* @param errorCollector The error collector instance to log errors to.
* @throws IOException If the format can't be detected or reading fails.
*/
public static void read(Path path, MappingVisitor visitor, ErrorCollector errorCollector) throws IOException {
read(path, null, visitor, errorCollector);
}

/**
* Tries to read the given path using the passed format's reader.
*
Expand All @@ -196,20 +212,33 @@ public static void read(Path path, MappingVisitor visitor) throws IOException {
* @param visitor The receiving visitor.
* @throws IOException If reading fails.
*/
@Deprecated
public static void read(Path path, MappingFormat format, MappingVisitor visitor) throws IOException {
read(path, format, visitor, new ThrowingErrorCollector(Severity.ERROR));
NebelNidas marked this conversation as resolved.
Show resolved Hide resolved
}

/**
* Tries to read the given path using the passed format's reader.
*
* @param path The path to read from. Can be a file or a directory.
* @param format The format to use. Has to match the path's format.
* @param visitor The receiving visitor.
* @throws IOException If reading fails.
*/
public static void read(Path path, MappingFormat format, MappingVisitor visitor, ErrorCollector errorCollector) throws IOException {
if (format == null) {
format = detectFormat(path);
if (format == null) throw new IOException("invalid/unsupported mapping format");
}

if (format.hasSingleFile()) {
try (Reader reader = Files.newBufferedReader(path)) {
read(reader, format, visitor);
read(reader, format, visitor, errorCollector);
}
} else {
switch (format) {
case ENIGMA_DIR:
EnigmaDirReader.read(path, visitor);
EnigmaDirReader.read(path, visitor, errorCollector);
break;
default:
throw new IllegalStateException();
Expand All @@ -224,10 +253,23 @@ public static void read(Path path, MappingFormat format, MappingVisitor visitor)
* @param visitor The receiving visitor.
* @throws IOException If the format can't be detected or reading fails.
*/
@Deprecated
public static void read(Reader reader, MappingVisitor visitor) throws IOException {
read(reader, null, visitor);
}

/**
* Tries to detect the reader's content's format and read it.
*
* @param reader The reader to read from.
* @param visitor The receiving visitor.
* @param errorCollector The error collector instance to log errors to.
* @throws IOException If the format can't be detected or reading fails.
*/
public static void read(Reader reader, MappingVisitor visitor, ErrorCollector errorCollector) throws IOException {
read(reader, null, visitor, errorCollector);
}

/**
* Tries to read the reader's content using the passed format's mapping reader.
*
Expand All @@ -236,7 +278,21 @@ public static void read(Reader reader, MappingVisitor visitor) throws IOExceptio
* @param visitor The receiving visitor.
* @throws IOException If reading fails.
*/
@Deprecated
public static void read(Reader reader, MappingFormat format, MappingVisitor visitor) throws IOException {
read(reader, format, visitor, new ThrowingErrorCollector(Severity.ERROR));
}

/**
* Tries to read the reader's content using the passed format's mapping reader.
*
* @param reader The reader to read from.
* @param format The format to use. Has to match the reader's content's format.
* @param visitor The receiving visitor.
* @param errorCollector The error collector instance to log errors to.
* @throws IOException If reading fails.
*/
public static void read(Reader reader, MappingFormat format, MappingVisitor visitor, ErrorCollector errorCollector) throws IOException {
if (format == null) {
if (!reader.markSupported()) reader = new BufferedReader(reader);
reader.mark(DETECT_HEADER_LEN);
Expand All @@ -249,25 +305,25 @@ public static void read(Reader reader, MappingFormat format, MappingVisitor visi

switch (format) {
case TINY_FILE:
Tiny1FileReader.read(reader, visitor);
Tiny1FileReader.read(reader, visitor, errorCollector);
break;
case TINY_2_FILE:
Tiny2FileReader.read(reader, visitor);
Tiny2FileReader.read(reader, visitor, errorCollector);
break;
case ENIGMA_FILE:
EnigmaFileReader.read(reader, visitor);
EnigmaFileReader.read(reader, visitor, errorCollector);
break;
case SRG_FILE:
case XSRG_FILE:
SrgFileReader.read(reader, visitor);
SrgFileReader.read(reader, visitor, errorCollector);
break;
case CSRG_FILE:
case TSRG_FILE:
case TSRG_2_FILE:
TsrgFileReader.read(reader, visitor);
TsrgFileReader.read(reader, visitor, errorCollector);
break;
case PROGUARD_FILE:
ProGuardFileReader.read(reader, visitor);
ProGuardFileReader.read(reader, visitor, errorCollector);
break;
default:
throw new IllegalStateException();
Expand Down
116 changes: 116 additions & 0 deletions src/main/java/net/fabricmc/mappingio/format/ErrorCollector.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,116 @@
/*
* Copyright (c) 2023 FabricMC
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package net.fabricmc.mappingio.format;

import java.io.IOException;
import java.util.ArrayList;
import java.util.Collections;
import java.util.List;

import org.jetbrains.annotations.ApiStatus;

public interface ErrorCollector {
static ErrorCollector create() {
return new ErrorCollector() {
@Override
public void add(Severity severity, String message) throws IOException {
errors.add(new ParsingError(severity, message));
}

@Override
public List<ParsingError> getErrors() {
return errors;
}

private final List<ParsingError> errors = new ArrayList<>();
};
}

default void addInfo(String message) throws IOException {
add(Severity.INFO, message);
}

default void addWarning(String message) throws IOException {
add(Severity.WARNING, message);
}

default void addError(String message) throws IOException {
add(Severity.ERROR, message);
}

void add(Severity severity, String message) throws IOException;

List<ParsingError> getErrors();
NebelNidas marked this conversation as resolved.
Show resolved Hide resolved

enum Severity {
/**
* When something's technically wrong but doesn't affect
* parsing or the mapping data in any way.
*/
INFO,
/**
* When element data is partially missing, but the rest of the element
* could still be deciphered and it didn't have to be skipped entirely.
* Or when an unknown top-level element is encountered.
*/
WARNING,
/**
* An issue so severe that parsing of entire elements had to be skipped.
* E.g. a class's/member's source name being absent.
*/
ERROR
}

class ParsingError {
NebelNidas marked this conversation as resolved.
Show resolved Hide resolved
ParsingError(Severity severity, String message) {
this.severity = severity;
this.message = message;
}

public Severity getSeverity() {
return severity;
}

public String getMessage() {
return message;
}

private final Severity severity;
private final String message;
}

@ApiStatus.Internal
class ThrowingErrorCollector implements ErrorCollector {
NebelNidas marked this conversation as resolved.
Show resolved Hide resolved
public ThrowingErrorCollector(Severity severityToThrowAt) {
this.severityToThrowAt = severityToThrowAt;
}

@Override
public void add(Severity severity, String message) throws IOException {
if (severity.compareTo(severityToThrowAt) >= 0) {
throw new IOException(message);
}
}

@Override
public List<ParsingError> getErrors() {
return Collections.emptyList();
}

private Severity severityToThrowAt;
NebelNidas marked this conversation as resolved.
Show resolved Hide resolved
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,9 @@
import net.fabricmc.mappingio.MappingUtil;
import net.fabricmc.mappingio.MappingVisitor;
import net.fabricmc.mappingio.adapter.ForwardingMappingVisitor;
import net.fabricmc.mappingio.format.ErrorCollector;
import net.fabricmc.mappingio.format.ErrorCollector.Severity;
import net.fabricmc.mappingio.format.ErrorCollector.ThrowingErrorCollector;
import net.fabricmc.mappingio.format.MappingFormat;
import net.fabricmc.mappingio.tree.MappingTree;
import net.fabricmc.mappingio.tree.MemoryMappingTree;
Expand All @@ -43,11 +46,21 @@ public final class EnigmaDirReader {
private EnigmaDirReader() {
}

@Deprecated
public static void read(Path dir, MappingVisitor visitor) throws IOException {
read(dir, MappingUtil.NS_SOURCE_FALLBACK, MappingUtil.NS_TARGET_FALLBACK, visitor);
}

public static void read(Path dir, MappingVisitor visitor, ErrorCollector errorCollector) throws IOException {
read(dir, MappingUtil.NS_SOURCE_FALLBACK, MappingUtil.NS_TARGET_FALLBACK, visitor, errorCollector);
}

@Deprecated
public static void read(Path dir, String sourceNs, String targetNs, MappingVisitor visitor) throws IOException {
read(dir, sourceNs, targetNs, visitor, new ThrowingErrorCollector(Severity.ERROR));
}

public static void read(Path dir, String sourceNs, String targetNs, MappingVisitor visitor, ErrorCollector errorCollector) throws IOException {
Set<MappingFlag> flags = visitor.getFlags();
MappingVisitor parentVisitor = null;

Expand Down Expand Up @@ -89,7 +102,7 @@ public boolean visitEnd() throws IOException {
@Override
public FileVisitResult visitFile(Path file, BasicFileAttributes attrs) throws IOException {
if (file.getFileName().toString().endsWith("." + MappingFormat.ENIGMA_FILE.fileExt)) {
EnigmaFileReader.read(Files.newBufferedReader(file), sourceNs, targetNs, delegatingVisitor);
EnigmaFileReader.read(Files.newBufferedReader(file), sourceNs, targetNs, delegatingVisitor, errorCollector);
}

return FileVisitResult.CONTINUE;
Expand Down
Loading