Uh oh!
There was an error while loading. Please reload this page.
fix: guard compiler registration and golden writing - #321
Merged
Conversation
Uh oh!
There was an error while loading. Please reload this page.
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 freeto 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.
Summary
compilers.Registry.Registercalledc.Formats()on its argument before checkinganything, so registering a nil compiler was a nil dereference raised inside the
package rather than an error the caller could handle.
engine.NewWith(nil)segfaulted:
Issue #89 assumed the opposite. It lists
Registry.Registeramong the boundariesthat "all guard their inputs" and asks only for a nil guard on
engine.NewWithRegistry. Neither half holds:Registerguarded nothing, andNewWithRegistrywas removed in #97 when the engine started taking compilersinstead of a registry. The live boundaries are
Registry.Register,engine.NewWithandirtest.WriteGolden, and a panic escaping a package is aworse defect than the missing assertion the issue describes.
What changed:
Registry.Registerrejects a nil compiler before it calls anything on it —both an untyped nil interface and a typed nil pointer stored in one, since
screening only the first leaves half the hole open. The existing no-formats and
duplicate-format errors are unchanged.
Registry's zero value is now a usable empty registry. ARegistrywritten asa struct literal has a nil map, and registering into one panicked with
"assignment to entry in nil map"; the map is allocated on first use instead.
engine.NewWithnames the argument position in the error it wraps, so a callerpassing several compilers can tell which one was rejected. It keeps delegating
the preconditions themselves to
Registerrather than restating them.irtest.WriteGoldenrefuses an empty path and a nil document, andCompareGoldenrefuses the same two by failing the test. A nil documentmarshals to the four bytes
null, so an unguardedWriteGoldenleft a goldenon disk that every later nil document matched: a snapshot that asserts nothing
and looks exactly like a passing one.
Test plan
TestRegistry_RejectsNilCompilercovers both spellings of nil; each subtestpanicked at
compilers.go:58before the guard.TestRegistry_ZeroValueRegistersregisters into a zero-valueRegistry; itpanicked with "assignment to entry in nil map" before.
TestNewWith_NilCompilerpasses the nil second, so the reported index provesit is the argument's own position rather than a constant; it segfaulted before.
TestNewWith_RegisterErrornow pins that index too.TestWriteGolden_RejectsBadInputasserts both errors and that a refused writeleaves no file behind; the nil case previously wrote
nulland returned nil.TestCompareGolden_RejectsBadInputcompares a nil document against a goldencontaining
null— the file an unguardedWriteGoldenproduces. Without theguard that comparison reports a clean match.
being restored. Full gate green: gofmt, vet, golangci-lint, build, and the 100%
coverage gate.
Closes#89