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

Tiny readers crash when encountering trailing empty destination name columns #22

Open
NebelNidas opened this issue Sep 8, 2024 · 3 comments

Comments

@NebelNidas
Copy link

NebelNidas commented Sep 8, 2024

If a mapping's destination name in a given namespace is equivalent to the source name, the Tiny formats allow to omit the relevant column's content (leading to two or more consecutive tabs, or trailing tabs at the end of the line). Example:

tiny	2	0	srcNs	dstNs1	dstNs2
c	class1	class1Rename	

The effective target names are:

  • srcNs: class1
  • dstNs1: class1Rename
  • dstNs2: class1

Currently though, SrgUtils's readers crash with the following exceptions respectively:

java.io.IOException: Invalid Tiny v2 line: #1: c class_1  
	at net.minecraftforge.srgutils.InternalUtils.tiny2Exception(InternalUtils.java:531)
	at net.minecraftforge.srgutils.InternalUtils.loadTinyV2(InternalUtils.java:466)

java.io.IOException: Invalid Tiny v1 line: #1
	at net.minecraftforge.srgutils.InternalUtils.loadTinyV1(InternalUtils.java:373)

It also seems like the line numbers are off by one (line 1 contains to the headers, not the class definitions).

@LexManos
Copy link
Member

LexManos commented Sep 8, 2024

The spec doesn't actually say what values these 'optional' entries are to have. And the JVMS does not allow for empty names. As such throwing an error is a valid option here. However that's not what happens which is what I consider a bug. Tho I don't have any validation on the mapped entries being valid bytecode mappings. So that's a whole other feature i would have to add and I don't care to right now.

However again your test case doesn't actually exemplify the issue your stack shows.
The error that would result in that stack is caused by you having a invalid tiny file where you're missing a name entirely. More akin to this example:

tiny	2	0	srcNs	dstNs1
c	class1

Which is indeed invalid. More specifically, your stack trace shows you used spaces instead of tabs as your delimitator.

Anyways as for the 'optional' entries. I will just have it propagate the last seen value. Would be nice if when they added this 'optional' system they specified what it meant. Oh well.

Edit: Actually doing more looking into this the spec now conflicts with itself. Or at least the interpretation you're trying to say. Specifically real world examples of parameter names. Which often times are empty strings when omitted. What names should we use for those?
Are we expected to support 'unnaming' a named object? "foo" -> ""
Are we to special case parameter names?
Are we to take direction into account? Considering mapping files in a original, intermediate, named map can be accessed in any form: original -> named named -> intermediate intermediate -> original etc.. What value should 'intermediate' have if none is specified..

Edit: Ya I'm not gunna change this until you cite your sources as this being the 'indented' behavior.
It's not on the spec in the way I interpret it. And the more I think about it the more it just causes issues.

@NebelNidas
Copy link
Author

NebelNidas commented Sep 8, 2024

The spec doesn't actually say what values these 'optional' entries are to have.

Hmm, depending on interpretation, that's implied by Mappings without any (useful) names should be omitted. You're right in that the spec should be more specific here, but in practice, an empty destination name is always treated as being equal to the source name.

However again your test case doesn't actually exemplify the issue your stack shows.

Oops, somehow I copied the wrong snippet from the mapping file again, sorry. It's fixed now.

More akin to this example: [...]

Yes, but your example misses the trailing tab at the end of the line.

your stack trace shows you used spaces instead of tabs as your delimitator.

That's not the case, I think you're just misinterpreting as my reproduction case was wrong.

I will just have it propagate the last seen value.

As noted above, it has to be the corresponding source name, that's how all the Fabric (= first-party) tooling handles it.

Regarding parameter names, if both the source name and a destination name are empty, that's simply a result of <var-name-a> (which is optional) not being present, and you aren't supposed to interpret anything into the empty destination name, it's simply missing ("unmapping" isn't possible via Tiny v2).

@NebelNidas NebelNidas changed the title Tiny readers crash when encountering empty destination name columns Tiny readers crash when encountering trailing empty destination name columns Sep 8, 2024
@LexManos
Copy link
Member

LexManos commented Sep 8, 2024

Hmm, depending on interpretation, that's implied by Mappings without any (useful) names should be omitted. You're right in that the spec should be more specific here, but in practice, an empty destination name is always treated as being equal to the source name.

Show me the practice.

Yes, but your example misses the trailing tab at the end of the line.

No, it's not that's the point you can only get that error if you DO NOT have tabs at the end of your line. As the only test is that there are enough entries in the array to fill in the names. Not that the names are non-empty.

That's not the case, I think you're just misinterpreting as my reproduction case was wrong.

No i'm not, you can literally copy/paste your error message and see it has two spaces instead of tabs.
And the only way to reproduce this error is to use spaces instead of tabs.

As noted above, it has to be the corresponding source name, that's how all the Fabric (= first-party) tooling handles it.

Regarding parameter names, if both the source name and a destination name are empty, that's simply a result of <var-name-a> (which is optional) not being present, and you aren't supposed to interpret anything into the empty destination name, it's simply missing ("unmapping" isn't possible via Tiny v2).

Show your citation. As it's clearly not in the spec.

tiny	2	0	A	B	C	D
c	clsA		clsC	

According to the spec that could be interpreted in many ways. It would be better to have it actually specified and documented somewhere before I make the call for SRGUtils.

@LexManos LexManos closed this as completed Sep 8, 2024
@LexManos LexManos reopened this Sep 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants