Skip to content

Create cross-format StandardProperties class #29

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

Draft
wants to merge 21 commits into
base: dev
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
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
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
- Added Recaf Simple reader and writer
- Added `OuterClassNameInheritingVisitor`
- Added `MappingFormat#hasWriter` boolean
- Added cross-format `StandardProperties` class
- Allowed Tiny v1 to save arbitrary metadata, not only intermediary counters

## [0.5.1] - 2023-11-30
- Improved documentation
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@
* <td>-</td>
* <td>-</td>
* <td>-</td>
* <td>✔ (Currently limited support)</td>
* <td>✔</td>
* </tr>
* <tr>
* <td>Tiny v2</td>
Expand Down
109 changes: 109 additions & 0 deletions src/main/java/net/fabricmc/mappingio/format/StandardProperties.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,109 @@
/*
* 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.util.AbstractMap;
import java.util.Collection;
import java.util.Collections;
import java.util.HashMap;
import java.util.Map;

import org.jetbrains.annotations.ApiStatus;
import org.jetbrains.annotations.Nullable;

public final class StandardProperties {
private StandardProperties() {
}

public static Collection<StandardProperty> values() {
return Collections.unmodifiableCollection(valuesById.values());
}

@Nullable
public static StandardProperty getByName(MappingFormat format, String name) {
return valuesByFormatAndName.get(new AbstractMap.SimpleEntry<>(format, name));
}

@Nullable
@ApiStatus.Internal
public static StandardProperty getById(String id) {
Copy link

Choose a reason for hiding this comment

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

This design is bad: malicious actors can fake an id and pollute the metadata detection. You should just keep a Map<MappingFormat, Map<String, StandardProperty>> instead.

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure what you mean, the internal maps are all private anyway

Copy link

Choose a reason for hiding this comment

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

Say if you have a next-intermediary-method read from tiny v1, you would incorrectly interpret it as an intermediary counter while it isn't.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've already added a sanity check preventing this: 5341ecf

Copy link
Member Author

@NebelNidas NebelNidas Sep 25, 2023

Choose a reason for hiding this comment

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

The issue why I need a simple String ID instead of a Map<MappingFormat, Map<String, StandardProperty>> is intermediate operations, say MemoryMappingTree.accept(MemoryMappingTree), where none of the participants know which mapping format the former tree was originally constructed from. I could generate a random ID per session, but IMO that's overcomplicating things without much reason. Maybe prepending the existing ID with mio: would do?

Copy link

Choose a reason for hiding this comment

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

Can we just add a new default void visitStandardProperty(StandardProperty prop, String value) in MappingVisitor and FlatMappingVisitor, which fabric's builtin visitors will override? And we can choose to feed these standard properties back as String key-values in visit options.

Copy link
Member Author

@NebelNidas NebelNidas Oct 10, 2023

Choose a reason for hiding this comment

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

Hmm, not a huge fan of this, since up until this point the visitor- and tree-based APIs have always been cleanly separated.
Edit: On the other hand, StandardProperty isn't really part of the tree-api, so maybe it'd work? But it still duplicates the metadata visit methods, which pollute the FlatMappingVisitor interface, especially considering #41 will add even more.

return valuesById.get(id);
}

public static final StandardProperty NEXT_INTERMEDIARY_CLASS;
public static final StandardProperty NEXT_INTERMEDIARY_FIELD;
public static final StandardProperty NEXT_INTERMEDIARY_METHOD;
public static final StandardProperty NEXT_INTERMEDIARY_COMPONENT;
Comment on lines +47 to +50
Copy link
Member

Choose a reason for hiding this comment

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

Im not sure these should exist in mio, they are specific to intermediary and are generated by matcher. They arent part of the tiny format, and I dont think we actually read them anymore?

Copy link
Member

Choose a reason for hiding this comment

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

happy to discuss this if you think otherwise.

Copy link
Member Author

@NebelNidas NebelNidas Mar 8, 2024

Choose a reason for hiding this comment

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

I'd say this is a bit of an edge-case, since the counters have been special-cased until now and consequently behaved as de-facto standard properties. I could remove it, so we end up using the INTERMEDIARY_COUNTER notation in Tiny v2, too, though this would conflict with existing naming conventions and we would be stuck with a misleading name. We could of course also delegate this responsibility to Matcher, which would have to do the conversion via a custom forwarding visitor pass

Copy link
Member Author

Choose a reason for hiding this comment

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

In the long run we might need an API letting consumers register their own standard properties

public static final StandardProperty MISSING_LVT_INDICES;
public static final StandardProperty ESCAPED_NAMES;
private static final Map<Map.Entry<MappingFormat, String>, StandardProperty> valuesByFormatAndName = new HashMap<>();
private static final Map<String, StandardProperty> valuesById = new HashMap<>();

static {
NEXT_INTERMEDIARY_CLASS = register("next-intermediary-class")
.addMapping(MappingFormat.TINY_FILE, "INTERMEDIARY_COUNTER class")
.addMapping(MappingFormat.TINY_2_FILE, "next-intermediary-class");
NEXT_INTERMEDIARY_FIELD = register("next-intermediary-field")
.addMapping(MappingFormat.TINY_FILE, "INTERMEDIARY_COUNTER field")
.addMapping(MappingFormat.TINY_2_FILE, "next-intermediary-field");
NEXT_INTERMEDIARY_METHOD = register("next-intermediary-method")
.addMapping(MappingFormat.TINY_FILE, "INTERMEDIARY_COUNTER method")
.addMapping(MappingFormat.TINY_2_FILE, "next-intermediary-method");
NEXT_INTERMEDIARY_COMPONENT = register("next-intermediary-component")
.addMapping(MappingFormat.TINY_FILE, "INTERMEDIARY_COUNTER component")
.addMapping(MappingFormat.TINY_2_FILE, "next-intermediary-component");
MISSING_LVT_INDICES = register("missing-lvt-indices")
.addMapping(MappingFormat.TINY_2_FILE, "missing-lvt-indices");
ESCAPED_NAMES = register("escaped-names")
.addMapping(MappingFormat.TINY_2_FILE, "escaped-names");
}

private static StandardPropertyImpl register(String id) {
return new StandardPropertyImpl(id);
}

private static class StandardPropertyImpl implements StandardProperty {
StandardPropertyImpl(String id) {
this.id = id;
valuesById.put(id, this);
}

private StandardPropertyImpl addMapping(MappingFormat format, String name) {
nameByFormat.put(format, name);
valuesByFormatAndName.put(new AbstractMap.SimpleEntry<>(format, name), this);
return this;
}

@Override
public boolean isApplicableTo(MappingFormat format) {
return nameByFormat.containsKey(format);
}

@Override
public String getNameFor(MappingFormat format) {
return nameByFormat.get(format);
}

@Override
public String getId() {
return id;
}

private final String id;
private final Map<MappingFormat, String> nameByFormat = new HashMap<>(4);
}
}
34 changes: 34 additions & 0 deletions src/main/java/net/fabricmc/mappingio/format/StandardProperty.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
/*
* 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 org.jetbrains.annotations.ApiStatus;
import org.jetbrains.annotations.Nullable;

@ApiStatus.NonExtendable
public interface StandardProperty {
boolean isApplicableTo(MappingFormat format);

@Nullable
String getNameFor(MappingFormat format);

/**
* Used internally by MappingTrees, consistency between JVM sessions or library versions isn't guaranteed!
*/
@ApiStatus.Internal
String getId();
}
Original file line number Diff line number Diff line change
Expand Up @@ -19,14 +19,18 @@
import java.io.IOException;
import java.io.Reader;
import java.util.ArrayList;
import java.util.LinkedHashMap;
import java.util.List;
import java.util.Map;
import java.util.Set;

import net.fabricmc.mappingio.MappedElementKind;
import net.fabricmc.mappingio.MappingFlag;
import net.fabricmc.mappingio.MappingVisitor;
import net.fabricmc.mappingio.format.ColumnFileReader;
import net.fabricmc.mappingio.format.MappingFormat;
import net.fabricmc.mappingio.format.StandardProperties;
import net.fabricmc.mappingio.format.StandardProperty;
import net.fabricmc.mappingio.tree.MappingTree;
import net.fabricmc.mappingio.tree.MemoryMappingTree;

Expand Down Expand Up @@ -98,11 +102,21 @@ private static void read(ColumnFileReader reader, MappingVisitor visitor) throws
String lastClass = null;
boolean lastClassDstNamed = false;;
boolean visitLastClass = false;
Map<String, String> potentialFooterMetadata = new LinkedHashMap<>();

while (reader.nextLine(0)) {
boolean isMethod;
boolean isClass = false;
boolean isMethod = false;
boolean isField = false;

if ((isClass = reader.nextCol("CLASS"))
|| (isMethod = reader.nextCol("METHOD"))
|| (isField = reader.nextCol("FIELD"))) {
// Footer metadata can't be followed by a new class/method/field, so any stored data was comments
potentialFooterMetadata.clear();
}

if (reader.nextCol("CLASS")) { // class: CLASS <names>...
if (isClass) { // class: CLASS <names>...
String srcName = reader.nextCol();
if (srcName == null || srcName.isEmpty()) throw new IOException("missing class-name-a in line "+reader.getLineNumber());

Expand All @@ -116,7 +130,7 @@ private static void read(ColumnFileReader reader, MappingVisitor visitor) throws
visitLastClass = visitor.visitElementContent(MappedElementKind.CLASS);
}
}
} else if ((isMethod = reader.nextCol("METHOD")) || reader.nextCol("FIELD")) { // method: METHOD cls-a desc-a <names>... or field: FIELD cls-a desc-a <names>...
} else if (isMethod || isField) { // method: METHOD cls-a desc-a <names>... or field: FIELD cls-a desc-a <names>...
String srcOwner = reader.nextCol();
if (srcOwner == null || srcOwner.isEmpty()) throw new IOException("missing class-name-a in line "+reader.getLineNumber());

Expand All @@ -141,31 +155,37 @@ private static void read(ColumnFileReader reader, MappingVisitor visitor) throws
}
} else {
String line = reader.nextCol();
final String prefix = "# INTERMEDIARY-COUNTER ";
String[] parts;

if (line.startsWith(prefix)
&& (parts = line.substring(prefix.length()).split(" ")).length == 2) {
String property = null;

switch (parts[0]) {
case "class":
property = nextIntermediaryClassProperty;
break;
case "field":
property = nextIntermediaryFieldProperty;
break;
case "method":
property = nextIntermediaryMethodProperty;
break;

if (line.startsWith("# ") && line.length() >= 3 && line.charAt(3) != ' ') { // Potentially metadata
line = line.substring(2);
String[] parts = line.split(" ");
String value = parts[parts.length - 1];
String key = line.substring(0, line.lastIndexOf(value) - 1);

if (key.isEmpty()) {
String oldValue = value;
value = key;
key = oldValue;
}

StandardProperty property = StandardProperties.getByName(format, key);

if (property != null) {
visitor.visitMetadata(property, parts[1]);
key = property.getId();
}

if (lastClass == null) { // Header metadata
visitor.visitMetadata(key, value);
} else {
potentialFooterMetadata.put(key, value);
}
}
}
}

for (Map.Entry<String, String> entry : potentialFooterMetadata.entrySet()) {
visitor.visitMetadata(entry.getKey(), entry.getValue());
}
}

if (visitor.visitEnd()) break;
Expand All @@ -187,7 +207,5 @@ private static void readDstNames(ColumnFileReader reader, MappedElementKind subj
}
}

static final String nextIntermediaryClassProperty = "next-intermediary-class";
static final String nextIntermediaryFieldProperty = "next-intermediary-field";
static final String nextIntermediaryMethodProperty = "next-intermediary-method";
private static final MappingFormat format = MappingFormat.TINY_FILE;
}
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,8 @@
import net.fabricmc.mappingio.MappingFlag;
import net.fabricmc.mappingio.MappingWriter;
import net.fabricmc.mappingio.format.MappingFormat;
import net.fabricmc.mappingio.format.StandardProperties;
import net.fabricmc.mappingio.format.StandardProperty;

/**
* {@linkplain MappingFormat#TINY_1 Tiny v1 file} writer.
Expand Down Expand Up @@ -65,30 +67,18 @@ public void visitNamespaces(String srcNamespace, List<String> dstNamespaces) thr

@Override
public void visitMetadata(String key, @Nullable String value) throws IOException {
switch (key) {
case Tiny1FileReader.nextIntermediaryClassProperty:
case Tiny1FileReader.nextIntermediaryFieldProperty:
case Tiny1FileReader.nextIntermediaryMethodProperty:
write("# INTERMEDIARY-COUNTER ");

switch (key) {
case Tiny1FileReader.nextIntermediaryClassProperty:
write("class");
break;
case Tiny1FileReader.nextIntermediaryFieldProperty:
write("field");
break;
case Tiny1FileReader.nextIntermediaryMethodProperty:
write("method");
break;
default:
throw new IllegalStateException();
}
StandardProperty property = StandardProperties.getById(key);

write(" ");
write(value);
writeLn();
if (property != null) {
if (!property.isApplicableTo(format)) return;
key = property.getNameFor(format);
}

write("# ");
write(key);
write(" ");
write(value);
writeLn();
}

@Override
Expand Down Expand Up @@ -197,6 +187,7 @@ private void writeTab() throws IOException {
}

private static final Set<MappingFlag> flags = EnumSet.of(MappingFlag.NEEDS_SRC_FIELD_DESC, MappingFlag.NEEDS_SRC_METHOD_DESC);
private static final MappingFormat format = MappingFormat.TINY_FILE;

private final Writer writer;
private String classSrcName;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@
import net.fabricmc.mappingio.MappingVisitor;
import net.fabricmc.mappingio.format.ColumnFileReader;
import net.fabricmc.mappingio.format.MappingFormat;
import net.fabricmc.mappingio.format.StandardProperties;
import net.fabricmc.mappingio.format.StandardProperty;

/**
* {@linkplain MappingFormat#TINY_2 Tiny v2 file} reader.
Expand Down Expand Up @@ -96,16 +98,21 @@ private static void read(ColumnFileReader reader, MappingVisitor visitor) throws
if (visitHeader || firstIteration) {
while (reader.nextLine(1)) {
if (!visitHeader) {
if (!escapeNames && reader.nextCol(Tiny2Util.escapedNamesProperty)) {
if (!escapeNames && reader.nextCol(StandardProperties.ESCAPED_NAMES.getNameFor(format))) {
escapeNames = true;
}
} else {
String key = reader.nextCol();
if (key == null) throw new IOException("missing property key in line "+reader.getLineNumber());
String value = reader.nextEscapedCol(); // may be missing -> null
StandardProperty property = StandardProperties.getByName(format, key);

if (key.equals(Tiny2Util.escapedNamesProperty)) {
escapeNames = true;
if (property != null) {
key = property.getId();

if (property == StandardProperties.ESCAPED_NAMES) {
escapeNames = true;
}
}

visitor.visitMetadata(key, value);
Expand Down Expand Up @@ -222,4 +229,6 @@ private static void readDstNames(ColumnFileReader reader, MappedElementKind subj
if (!name.isEmpty()) visitor.visitDstName(subjectKind, dstNs, name);
}
}

private static final MappingFormat format = MappingFormat.TINY_2_FILE;
}
Loading