Introduce support for constants in the MDC. #7
Conversation
Allows for unpick data to be stored here.
There was a problem hiding this comment.
This was discussed internally by some members of the team and external stakeholders, brought about by FabricMC/community#124 (comment).
Background
javac performs constant inlining at compile-time, inlining compile-time constant expressions (such as static final numerical constants, and operations on constants) in the class bytecode without having to reference the original constant. For example, a call of setBlockState(pos, block, Block.UPDATE_NEIGHBORS | Block.UPDATE_CLIENTS) would have constant inlining on the third parameter, leading to an effective bytecode-level call of setBlockState(pos, block, 3) (where Block.UPDATE_NEIGHBORS is 1 and Block.UPDATE_CLIENTS is 2).
However, this presents difficulties when reading decompiled source, because there is no trace that links these 'folded' constants from their originating counterparts. Per above, a viewer of decompiled source would only see the 3, and have no clue (outside of external or pre-obtained knowledge) that it came from Block.UPDATE_NEIGHBORS | Block.UPDATE_CLIENTS (or maybe even Block.UPDATE_ALL).
Constant un-inlining or unpicking attempts to solve this problem by building a database of known places in the codebase where constants are used, and a database of these constants, and then using this information to 'reverse' a combined constant back into its constituent fields. In the above example, it would know the third parameter of setBlockState is a potential constant unpicking site, and it would know the series of Block constant fields which fit there, and act accordingly (either on bytecode or on source) to recreate the bitwise operation of those constant fields.
This PR is the first step to adding such constant unpicking information to Parchment, to be carried alongside our mappings. As Parchment stands as a modloader-neutral set of mappings, we can be a centralized source of unpicking information for consumption by any modloader or toolchain which so chooses to use that information.
There's two things in this PR which I feel needs to be addressed:
-
Pseudo-examples of how the constant data would be stored, such as for exmaple
setBlockState, must be given so we can properly contextualize how this information might be written or read. -
The Mapping Data Container format version needs to be bumped on its minor version, per our specification. We also need to update the specification accordingly, once this PR is through
We might also think on including the specifications as part of the repository proper, so it is always available as a reference, rather than being part of the GitHub repository wiki. I think it would be good to include the current specification into the repository as a separate PR, so this PR can then modify it accordingly.
-
The test data should be properly modified to include tests for this constant data, so the automated IO tests can check that the data is serialized and deserialized properly.
-
I have the concern that, if I understand the current situation, there will be a lot of copy-pasting constant data for parameters between methods that share the same constant characteristics -- for example,
setBlockState,setBlock,sendBlockUpdated, and others.However, I do think it might be the purview of higher-level tools to do that collation and automated duplication of data per user-given inputs. For example, a tool like Enigma would maintain some such file with constant 'groups' (containing the list of constant fields), and mappings of such 'groups' to appropriate method parameters.
On this, I'm thinking on the previously discussed possibility that we ought to put the constant data in a separate file entirely, and leave the MDC format unchanged. However, that would mean making a new file format and codifying it through specification and code, which would be a burden indeed.
I'd like your input on that matter.
| if (!(o instanceof FieldData)) return false; | ||
| FieldData that = (FieldData) o; | ||
| return getName().equals(that.getName()) && Objects.equals(getDescriptor(), that.getDescriptor()) | ||
| && getJavadoc().equals(that.getJavadoc()); | ||
| if (!(o instanceof ImmutableFieldData)) return false; | ||
|
|
||
| ImmutableFieldData that = (ImmutableFieldData) o; |
There was a problem hiding this comment.
This change would mean ImmutableFieldData would be incomparable to other FieldData instances (such as mutable ones). It should only check if it is an instance of FieldData.
Same comment on all other similar instances.
| return Objects.hash(getName(), getDescriptor(), getJavadoc()); | ||
| int result = getName() != null ? getName().hashCode() : 0; | ||
| result = 31 * result + (getDescriptor() != null ? getDescriptor().hashCode() : 0); | ||
| result = 31 * result + (getJavadoc() != null ? getJavadoc().hashCode() : 0); | ||
| result = 31 * result + (getConstant() != null ? getConstant().hashCode() : 0); | ||
| return result; |
There was a problem hiding this comment.
Why change from Objects.hash? Are there any substantial benefits to hardcoding ourselves to this pattern of hashcoding? (I'd also like a source on how this pattern was decided.)
Same comment on all other similar instances.
This introduces constant field data to the MDC and processes it as if it was a JDoc entry.