server/item: move the item NBT codec out of nbtconv - #1361
Open
HashimTheArab wants to merge 2 commits into
Open
Conversation
Crossbow reaches nbtconv.WriteItem and nbtconv.MapItem through go:linkname
rather than an import, because nbtconv imports item and the import cannot go
back the other way.
A linkname pull creates no dependency edge, so the compiler never looks for
nbtconv and the linker only resolves the symbols if something else in the
binary happens to import it. Anything importing server (a real server) gets
it via server/entity and server/block, which is why this has not come up.
A binary that imports server/item on its own does not:
package main
import (
"github.com/df-mc/dragonfly/server/item"
"github.com/df-mc/dragonfly/server/world"
)
func main() {
var nbt world.NBTer = item.Crossbow{}
_ = nbt.EncodeNBT()
}
github.com/df-mc/dragonfly/server/item.Crossbow.EncodeNBT: relocation
target github.com/df-mc/dragonfly/server/internal/nbtconv.WriteItem
not defined
The stack codec is moved into item, where the type it serialises lives and
where Crossbow can call it directly. nbtconv.WriteItem, Item and MapItem stay
as one-line wrappers, so none of the 22 call sites in entity, block and
item/creative change, and both linknames and the blank unsafe import are gone.
The six map readers the codec needs are duplicated privately in item rather
than imported: nbtconv depends on item, so the codec has to live here for the
dependency to point the right way, and these are the only helpers it uses.
mapValue keeps its name and field names. gob identifies the type by name, so
worlds written before this still decode.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Crossbowreaches the item stack NBT codec throughgo:linknamerather than an import, becausenbtconvimportsitemand the import cannot go back the other way.A linkname pull creates no dependency edge. The compiler never looks for
nbtconv, and the linker only resolves the symbols if something else in the binary happens to import it. Anything importingserver— a real server — gets it viaserver/entityandserver/block, which is presumably why this has gone unnoticed. A binary that importsserver/itemon its own does not:It bites tools and test binaries that pull in
server/itemwithout reaching entities or blocks. I hit it writing an item-persistence helper package.The change
The stack codec moves into
item, where the type it serialises lives and whereCrossbowcan call it directly. That is the only direction the dependency can point: anything encoding aStackmust importitem, so a package belowitemcannot do it, anditemcannot import upwards.All existing codec callers now use
item.WriteNBT,item.ReadNBT, anditem.MapNBTdirectly. The item-specific forwarding wrappers have been removed fromnbtconv; its inventory helpers remain because they also encode and decode inventory slots. Both linknames and the now-unnecessary blankunsafeimport are gone.Net: 16 files changed,
+364 / -320, almost all of it code moving between packages and direct call-site updates.Two things worth a reviewer's attention
Duplicated map readers. The codec needs six trivial readers (
Bool,Uint8,String,Int16,Int32,Slice) plus a block decoder. They are duplicated privately initemasnbtBool,nbtUint8and so on rather than imported, becausenbtconvdepends onitem. Splitting the generic half ofnbtconvinto a leaf package both could import would avoid that, at the cost of touching every call site — happy to do it that way instead if you prefer.itempicks upworld/chunk, forchunk.CurrentBlockVersioninwriteBlock.chunkdoes not importitem, so there is no cycle, but it does widenitem's footprint for one constant. Moving that constant somewhere more neutral would avoid it.Testing
go build ./...go vet ./...go test ./...masterand runs on this branch.mapValuekeeps its name and field names.gobidentifies the type by name, so worlds written before this still decode — the on-disk format is unchanged.