Skip to content

fix: panics on named types, unbounded exponent, ignored "-" tag - #24

Merged
wrfly merged 1 commit into
masterfrom
fix/panics-and-edge-cases
Aug 19, 2026
Merged

fix: panics on named types, unbounded exponent, ignored "-" tag#24
wrfly merged 1 commit into
masterfrom
fix/panics-and-edge-cases

Conversation

@wrfly

@wrfly wrfly commented Aug 19, 2026

Copy link
Copy Markdown
Owner

Panics

Parsing built the value with a hard-coded concrete type and assigned it to the field, which panics as soon as the field is not exactly that type:

type Level int
type Conf struct{ L []Level }         // reflect.Set: []int is not assignable to []Level
type Conf struct{ D *time.Duration }  // reflect.Set: *int64 is not assignable to *time.Duration

Both are reachable from an ordinary env value, and *time.Duration is a natural way to express "unset vs zero".

Both paths now build the value through the field's own reflect.Type, so named types work like their underlying kind. That single setValue helper replaces the fifteen near-identical branches of parseSlice and all of parsePointer (parse.go: 291 → 78 lines), and *time.Duration gains the "10s" syntax time.Duration already had.

Other fixes

before after
PORT=1e1000000 ~100s building a 1MB string rejected immediately
1.5e3 unparsable "1.5000" 1500
yaml:"-" / json:"-" / env:"-" honoured by List, ignored by Parse (default applied, field bound to a key named -) skipped by both
Parse(config) without & nil error, nothing filled error
*SubStruct section never walked into, bogus flat key in List optional section, allocated only when something is set, cycle-guarded
default:"false" on a bool overwrote a caller-set true kept, like every other kind
map / array fields silently ignored reported, and no longer listed
"a b" (default separator) 3 elements, numeric slices fail 2 elements
List quoting A="a "b" c" strconv.Quote
Get no prefix, pointer-only config, missed named types prefix param, works read-only, kind-based
parse errors %s %w
parser type unexported *ecp exported *ECP

Tests

TestGetKeyLookupValue replaced globalEcp.LookupValue and only restored BuildKey, leaking the mock into every test that ran after it — new tests were silently reading from it instead of the environment.

Regression coverage added for every item above; coverage 91.5% → 94.2%, suite passes under -race -shuffle=on -count=2.

Chores

CI runs vet, -race and gofmt over a Go 1.19/1.21/1.23/stable matrix (actions/*@v4/v5). Dead Travis config and badge removed. The README example printed output it did not actually produce (new log level: [ info ] was really debug, LOGLEVEL=debug was really ECP_LOGLEVEL=debug) — it is corrected and verified by running it.

Compatibility

Behaviour changes on purpose, hence a minor bump: Parse without a pointer now errors, --tagged fields are no longer filled, a caller-set bool survives default:"false", and unsupported kinds error instead of being ignored. The exported-type and Get prefix changes are source-compatible.

🤖 Generated with Claude Code

Parsing built the value with a hard-coded concrete type and assigned it to
the field, which panics as soon as the field is not exactly that type:

    type Level int
    type Conf struct{ L []Level }      // []int is not assignable to []Level
    type Conf struct{ D *time.Duration }  // *int64 is not assignable to *time.Duration

Both paths now build the value through the field's own reflect.Type, so
named types work like their underlying kind. This replaces the fifteen
near identical branches of parseSlice and all of parsePointer with a single
setValue helper, and *time.Duration gains the "10s" syntax time.Duration
already had.

Other fixes:

- parseScientific expanded the exponent one byte at a time with no upper
  bound: PORT=1e1000000 spent ~100s building a one megabyte string that
  cannot fit in any integer type. It is now bounded and rejects the value
  right away. "1.5e3" is 1500 instead of the unparsable "1.5000".
- `yaml:"-"`, `json:"-"` and `env:"-"` were honoured by List but ignored by
  Parse, which still applied the default and bound the field to a key
  literally named "-". They are now skipped by both.
- Parse(config) instead of Parse(&config) returned nil after filling in
  nothing at all. It now returns an error.
- A *struct field was never walked into: its defaults and keys were
  silently dropped and List emitted a bogus flat key for it. It is now an
  optional section, allocated only when one of its fields is set, with a
  guard so a self referencing type terminates.
- `default:"false"` on a bool overwrote a value already set by the caller,
  unlike every other kind.
- Kinds that cannot be filled from a string (map, array, ...) were ignored
  without a word; they are now reported and no longer listed.
- The default whitespace separator collapses repeated separators, so
  "a  b" is two elements instead of three.
- List quotes defaults with strconv.Quote instead of bare quotes.
- Get and friends take the prefix Parse was called with, work on a
  read-only config, and no longer miss named types.
- Parse errors are wrapped with %w.
- The parser type is exported as ECP, so callers can name it.

Tests: TestGetKeyLookupValue replaced globalEcp.LookupValue and never
restored it, leaking the mock into every test that ran after it.

CI runs vet, race and gofmt over a version matrix; the dead Travis config
and badge are gone, and the README example now prints what it claims.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@wrfly
wrfly merged commit aed75d2 into master Aug 19, 2026
5 checks passed
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

Successfully merging this pull request may close these issues.

1 participant