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

Jump to bottom

Create cross-format StandardProperties class #29

Draft
wants to merge 21 commits into
base: dev
Choose a base branch
from
Draft

Create cross-format StandardProperties class #29

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
c87e3ba
Move Tiny properties to separate class
NebelNidas May 9, 2023
ee30d57
Remove duplicate `escaped-names` property
NebelNidas May 9, 2023
ad0c724
Prevent instantiation of `TinyProperties` class
NebelNidas Jun 30, 2023
0910fa9
Create cross-format `StandardProperties` class
NebelNidas Sep 15, 2023
3cc9283
Fix checkstyle
NebelNidas Sep 18, 2023
bc4ce09
Add missing license headers
NebelNidas Sep 20, 2023
45783f0
Fix Tiny v1 metadata detection
NebelNidas Sep 20, 2023
5341ecf
Don't read properties from non-applicable formats
NebelNidas Sep 20, 2023
b8673c7
Use builder for standard properties
NebelNidas Sep 20, 2023
bd7ecc2
Consolidate builder functionality into `StandardPropertyImpl`
NebelNidas Sep 24, 2023
f0a2376
Merge upstream
NebelNidas Oct 10, 2023
da53284
Remove `getApplicableFormats` from `StandardProperty`
NebelNidas Oct 10, 2023
52852bd
Mark `StandardProperty` as non-extendable
NebelNidas Oct 10, 2023
6f4aa42
Allow to query for `StandardProperty`s via the tree API
NebelNidas Oct 10, 2023
0a46ede
Merge upstream
NebelNidas Nov 17, 2023
fd19740
Add changes to changelog
NebelNidas Nov 18, 2023
4633d7f
Tiny v1 properly supports arbitrary metadata now
NebelNidas Nov 18, 2023
273e481
Merge upstream
NebelNidas Nov 30, 2023
26e608f
Merge upstream
NebelNidas Mar 7, 2024
5eaf880
Make Tiny v1 metadata detection more resilient against false-positives
NebelNidas Mar 8, 2024
782469d
Address reviews
NebelNidas Mar 8, 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
138 changes: 138 additions & 0 deletions src/main/java/net/fabricmc/mappingio/format/StandardProperties.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,138 @@
/*
* 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.Collections;
import java.util.HashMap;
import java.util.HashSet;
import java.util.Map;
import java.util.Set;

import org.jetbrains.annotations.ApiStatus;

public final class StandardProperties {
private StandardProperties() {
}

public static Set<StandardProperty> values() {
return Collections.unmodifiableSet(values);
NebelNidas marked this conversation as resolved.
Show resolved Hide resolved
}

public static StandardProperty getByName(String name) {
return valuesByName.get(name);
}

@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
Collaborator 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
Collaborator 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
Collaborator Author

@NebelNidas NebelNidas Sep 25, 2023 •
edited
Loading

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
Collaborator Author

@NebelNidas NebelNidas Oct 10, 2023 •
edited
Loading

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
Collaborator Author

@NebelNidas NebelNidas Mar 8, 2024 •
edited
Loading

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
Collaborator 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 Set<StandardProperty> values = new HashSet<>();
private static final Map<String, StandardProperty> valuesByName = new HashMap<>();
private static final Map<String, StandardProperty> valuesById = new HashMap<>();

static {
NEXT_INTERMEDIARY_CLASS = builder("next-intermediary-class")
.add(MappingFormat.TINY_FILE, "INTERMEDIARY_COUNTER class")
.add(MappingFormat.TINY_2_FILE, "next-intermediary-class")
NebelNidas marked this conversation as resolved.
Show resolved Hide resolved
.build();
NEXT_INTERMEDIARY_FIELD = builder("next-intermediary-field")
.add(MappingFormat.TINY_FILE, "INTERMEDIARY_COUNTER field")
.add(MappingFormat.TINY_2_FILE, "next-intermediary-field")
.build();
NEXT_INTERMEDIARY_METHOD = builder("next-intermediary-method")
.add(MappingFormat.TINY_FILE, "INTERMEDIARY_COUNTER method")
.add(MappingFormat.TINY_2_FILE, "next-intermediary-method")
.build();
NEXT_INTERMEDIARY_COMPONENT = builder("next-intermediary-component")
.add(MappingFormat.TINY_FILE, "INTERMEDIARY_COUNTER component")
.add(MappingFormat.TINY_2_FILE, "next-intermediary-component")
.build();
MISSING_LVT_INDICES = builder("missing-lvt-indices")
.add(MappingFormat.TINY_2_FILE, "missing-lvt-indices")
.build();
ESCAPED_NAMES = builder("escaped-names")
.add(MappingFormat.TINY_2_FILE, "escaped-names")
.build();
}

private static PropertyBuilder builder(String id) {
return new PropertyBuilder(id);
}

private static class PropertyBuilder {
PropertyBuilder(String id) {
this.id = id;
}

PropertyBuilder add(MappingFormat format, String name) {
nameByFormat.put(format, name);
return this;
}

StandardProperty build() {
StandardProperty ret = new StandardPropertyImpl(id, new HashMap<>(nameByFormat));
NebelNidas marked this conversation as resolved.
Show resolved Hide resolved
values.add(ret);
valuesById.put(id, ret);

for (String name : nameByFormat.values()) {
valuesByName.putIfAbsent(name, ret);
}

return ret;
}

private final String id;
private final Map<MappingFormat, String> nameByFormat = new HashMap<>(4);
}

private static class StandardPropertyImpl implements StandardProperty {
StandardPropertyImpl(String id, Map<MappingFormat, String> nameByFormat) {
this.id = id;
this.nameByFormat = nameByFormat;
}

@Override
public Set<MappingFormat> getApplicableFormats() {
return nameByFormat.keySet();
}

@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;
}
}
34 changes: 34 additions & 0 deletions src/main/java/net/fabricmc/mappingio/format/StandardProperty.java
View file
Open in desktop
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 java.util.Set;

import org.jetbrains.annotations.ApiStatus;

public interface StandardProperty {
NebelNidas marked this conversation as resolved.
Show resolved Hide resolved
Set<MappingFormat> getApplicableFormats();
boolean isApplicableTo(MappingFormat format);
NebelNidas marked this conversation as resolved.
Show resolved Hide resolved
String getNameFor(MappingFormat format);

/**
* Used internally by MappingTrees, consistency between JVM sessions
* or library versions isn't guaranteed!
*/
@ApiStatus.Internal
String getId();
}
42 changes: 21 additions & 21 deletions src/main/java/net/fabricmc/mappingio/format/tiny/Tiny1FileReader.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,9 @@
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 @@ -134,28 +137,27 @@ 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) != ' ') { // Metadata
line = line.substring(2);
String[] parts = line.split(" ");
String value = parts[parts.length - 1];
String key = line.substring(0, line.lastIndexOf(value));

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

StandardProperty property = StandardProperties.getByName(key);

if (property != null) {
visitor.visitMetadata(property, parts[1]);
if (!property.isApplicableTo(format)) continue; // How did it get there?
key = property.getId();
}

visitor.visitMetadata(key, value);
}
}
}
Expand All @@ -180,7 +182,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;
}
36 changes: 14 additions & 22 deletions src/main/java/net/fabricmc/mappingio/format/tiny/Tiny1FileWriter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,9 @@
import net.fabricmc.mappingio.MappedElementKind;
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;

public final class Tiny1FileWriter implements MappingWriter {
public Tiny1FileWriter(Writer writer) {
Expand Down Expand Up @@ -59,30 +62,18 @@ public void visitNamespaces(String srcNamespace, List<String> dstNamespaces) thr

@Override
public void visitMetadata(String key, 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 @@ -191,6 +182,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
17 changes: 14 additions & 3 deletions src/main/java/net/fabricmc/mappingio/format/tiny/Tiny2FileReader.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,9 @@
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;

public final class Tiny2FileReader {
private Tiny2FileReader() {
Expand Down Expand Up @@ -89,16 +92,22 @@ 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(key);

if (key.equals(Tiny2Util.escapedNamesProperty)) {
escapeNames = true;
if (property != null) {
if (!property.isApplicableTo(format)) continue; // How did it get there?
key = property.getId();

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

visitor.visitMetadata(key, value);
Expand Down Expand Up @@ -215,4 +224,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;
}
19 changes: 15 additions & 4 deletions src/main/java/net/fabricmc/mappingio/format/tiny/Tiny2FileWriter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,9 @@
import net.fabricmc.mappingio.MappedElementKind;
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;

public final class Tiny2FileWriter implements MappingWriter {
public Tiny2FileWriter(Writer writer, boolean escapeNames) {
Expand Down Expand Up @@ -60,9 +63,16 @@ public void visitNamespaces(String srcNamespace, List<String> dstNamespaces) thr

@Override
public void visitMetadata(String key, String value) throws IOException {
if (key.equals(Tiny2Util.escapedNamesProperty)) {
escapeNames = true;
wroteEscapedNamesProperty = true;
StandardProperty property = StandardProperties.getById(key);

if (property != null) {
if (!property.isApplicableTo(format)) return;
key = property.getNameFor(format);

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

writeTab();
Expand All @@ -80,7 +90,7 @@ public void visitMetadata(String key, String value) throws IOException {
public boolean visitContent() throws IOException {
if (escapeNames && !wroteEscapedNamesProperty) {
write("\t");
write(Tiny2Util.escapedNamesProperty);
write(StandardProperties.ESCAPED_NAMES.getNameFor(format));
writeLn();
}

Expand Down Expand Up @@ -201,6 +211,7 @@ private void writeTabs(int count) throws IOException {
}

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

private final Writer writer;
private boolean escapeNames;
Expand Down
2 changes: 0 additions & 2 deletions src/main/java/net/fabricmc/mappingio/format/tiny/Tiny2Util.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,4 @@ public static String unescape(String str) {

private static final String toEscape = "\\\n\r\0\t";
private static final String escaped = "\\nr0t";

static final String escapedNamesProperty = "escaped-names";
}