diff --git a/README.md b/README.md index 10e08ca..3bc4b93 100644 --- a/README.md +++ b/README.md @@ -240,6 +240,97 @@ required := RequiredKey.MustGet(ctx) // Panics with clear message if not set 6. **Boolean keys**: Special `BoolKey` type with Enabled/Disabled/ExplicitlyDisabled methods 7. **Three-state logic**: Distinguish between unset, explicitly true, and explicitly false +## Background: Go Official Proposal + +A similar idea was proposed to the Go team in 2021: + +- [proposal: context: add generic key and value type #49189](https://github.com/golang/go/issues/49189) + +The proposal by [@dsnet](https://github.com/dsnet) (Joe Tsai, a Go team member) suggests: + +```go +type Key[Value any] struct { name *string } + +func NewKey[Value any](name string) Key[Value] { + return Key[Value]{&name} // Uses argument address for uniqueness +} + +func (k Key[V]) WithValue(ctx Context, val V) Context +func (k Key[V]) Value(ctx Context) (V, bool) +``` + +This package implements essentially the same concept. However, the official proposal has been on hold for over 3 years, primarily because: + +1. **Standard library generics policy is undecided** - [Discussion #48287](https://github.com/golang/go/discussions/48287) is still ongoing about how to add generics to existing packages +2. **Migration path unclear** - Whether to deprecate `context.WithValue`/`context.Value` or keep both APIs +3. **Alternative proposals being considered** - Multiple approaches are being evaluated in parallel + +This package provides an immediate, production-ready solution while the Go team deliberates. + +## Design Decisions + +### Sealed Interface Pattern + +Unlike the proposal's struct-based approach, this package uses the **Sealed Interface** pattern: + +```go +type Key[V any] interface { + WithValue(ctx context.Context, value V) context.Context + Get(ctx context.Context) V + TryGet(ctx context.Context) (V, bool) + // ... other methods + downcast() key[V] // unexported method prevents external implementation +} + +type key[V any] struct { // unexported implementation + name string + ident *opaque +} +``` + +**Why this matters:** + +The struct-based approach has a subtle vulnerability. In Go, you can bypass constructor functions and directly initialize structs with zero values for unexported fields: + +```go +// With struct-based design: +type Key[V any] struct { name *string } + +// This compiles! Both keys have nil name pointer +badKeyX := Key[int]{} +badKeyY := Key[string]{} +// These will COLLIDE because both use (*string)(nil) as identity +``` + +Note: `(*T)(nil)` doesn't panic like `nil` does - it silently uses the zero value as the key, making collisions hard to detect. + +With the Sealed Interface pattern: +- The implementation struct `key[V]` is unexported, preventing direct initialization +- The interface contains an unexported method `downcast()`, preventing external implementations +- Users **must** use `feature.New()` or `feature.NewBool()` to create keys + +**Additional benefit:** `BoolKey` can be used anywhere `Key[bool]` is expected, providing better interoperability than struct embedding would allow. + +### Empty Struct Optimization Avoidance + +The internal `opaque` type that provides pointer identity includes a byte field: + +```go +type opaque struct { + _ byte // Prevents address optimization +} +``` + +Without this, Go's compiler optimization would give all zero-size struct pointers the same address: + +```go +type empty struct{} + +a := new(empty) +b := new(empty) +fmt.Printf("%p %p\n", a, b) // Same address! Keys would collide. +``` + ## Best Practices ### 1. Define Keys as Package-Level Variables @@ -298,4 +389,5 @@ Contributions are welcome! Please feel free to submit a Pull Request. ## Related Projects -- [context](https://pkg.go.dev/context) - Go's official context package \ No newline at end of file +- [context](https://pkg.go.dev/context) - Go's official context package +- [proposal: context: add generic key and value type #49189](https://github.com/golang/go/issues/49189) - Go official proposal diff --git a/feature.go b/feature.go index e8ccf88..b648949 100644 --- a/feature.go +++ b/feature.go @@ -303,12 +303,6 @@ func (k key[V]) String() string { return k.name } -// GoString returns a Go syntax representation of the key. -// This implements fmt.GoStringer. -func (k key[V]) GoString() string { - return fmt.Sprintf("feature.Key[%T]{name: %q}", *new(V), k.name) -} - // Inspect retrieves the value from the context and returns an Inspection. func (k key[V]) Inspect(ctx context.Context) Inspection[V] { val, ok := k.TryGet(ctx) diff --git a/feature_test.go b/feature_test.go index faf4ef4..9f27bad 100644 --- a/feature_test.go +++ b/feature_test.go @@ -491,56 +491,6 @@ func TestString(t *testing.T) { }) } -// TestGoString tests the GoString method for keys. -func TestGoString(t *testing.T) { - t.Parallel() - - t.Run("Key GoString includes package name and type", func(t *testing.T) { - t.Parallel() - - key := feature.NewNamed[string]("test-key") - goStr := key.GoString() - - if !strings.Contains(goStr, "feature.Key[string]") { - t.Errorf("GoString() = %q, want to contain %q", goStr, "feature.Key[string]") - } - - if !strings.Contains(goStr, "name:") { - t.Errorf("GoString() = %q, want to contain field name %q", goStr, "name:") - } - - if !strings.Contains(goStr, "test-key") { - t.Errorf("GoString() = %q, want to contain %q", goStr, "test-key") - } - }) - - t.Run("Key GoString with int type", func(t *testing.T) { - t.Parallel() - - key := feature.NewNamed[int]("max-retries") - goStr := key.GoString() - - if !strings.Contains(goStr, "feature.Key[int]") { - t.Errorf("GoString() = %q, want to contain %q", goStr, "feature.Key[int]") - } - }) - - t.Run("BoolKey GoString includes bool type", func(t *testing.T) { - t.Parallel() - - flag := feature.NewNamedBool("my-feature") - goStr := flag.GoString() - - if !strings.Contains(goStr, "feature.Key[bool]") { - t.Errorf("GoString() = %q, want to contain %q", goStr, "feature.Key[bool]") - } - - if !strings.Contains(goStr, "my-feature") { - t.Errorf("GoString() = %q, want to contain %q", goStr, "my-feature") - } - }) -} - // TestNewNamed tests the NewNamed constructor. func TestNewNamed(t *testing.T) { t.Parallel() diff --git a/gostring.go b/gostring.go new file mode 100644 index 0000000..13edc50 --- /dev/null +++ b/gostring.go @@ -0,0 +1,24 @@ +package feature + +import ( + "fmt" + "reflect" +) + +// GoString returns a Go syntax representation of the key. +// The output is a valid Go expression that creates an equivalent key +// (though with a different identity). +// This implements fmt.GoStringer. +func (k key[V]) GoString() string { + typeName := reflect.TypeOf((*V)(nil)).Elem().String() + + return fmt.Sprintf("feature.New[%s](feature.WithName(%q))", typeName, k.name) +} + +// GoString returns a Go syntax representation of the bool key. +// The output is a valid Go expression that creates an equivalent key +// (though with a different identity). +// This implements fmt.GoStringer. +func (k boolKey) GoString() string { + return fmt.Sprintf("feature.NewBool(feature.WithName(%q))", k.name) +} diff --git a/gostring_test.go b/gostring_test.go new file mode 100644 index 0000000..c0a0b38 --- /dev/null +++ b/gostring_test.go @@ -0,0 +1,262 @@ +package feature_test + +import ( + "fmt" + "go/ast" + "go/importer" + "go/parser" + "go/token" + "go/types" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/mpyw/feature" +) + +// TestGoString tests the GoString method for keys. +func TestGoString(t *testing.T) { + t.Parallel() + + t.Run("Key GoString returns valid Go expression", func(t *testing.T) { + t.Parallel() + + key := feature.NewNamed[string]("test-key") + goStr := key.GoString() + + want := `feature.New[string](feature.WithName("test-key"))` + if goStr != want { + t.Errorf("GoString() = %q, want %q", goStr, want) + } + + assertCompilesWithFeatureImport(t, goStr) + }) + + t.Run("Key GoString with int type", func(t *testing.T) { + t.Parallel() + + key := feature.NewNamed[int]("max-retries") + goStr := key.GoString() + + want := `feature.New[int](feature.WithName("max-retries"))` + if goStr != want { + t.Errorf("GoString() = %q, want %q", goStr, want) + } + + assertCompilesWithFeatureImport(t, goStr) + }) + + t.Run("BoolKey GoString returns valid Go expression", func(t *testing.T) { + t.Parallel() + + flag := feature.NewNamedBool("my-feature") + goStr := flag.GoString() + + want := `feature.NewBool(feature.WithName("my-feature"))` + if goStr != want { + t.Errorf("GoString() = %q, want %q", goStr, want) + } + + assertCompilesWithFeatureImport(t, goStr) + }) +} + +// assertCompilesWithFeatureImport verifies that the given expression compiles +// as valid Go code with the feature package imported. +func assertCompilesWithFeatureImport(t *testing.T, expr string) { + t.Helper() + + // Wrap the expression in a minimal Go source file + src := `package main + +import "github.com/mpyw/feature" + +var _ = ` + expr + ` +` + + fset := token.NewFileSet() + + file, err := parser.ParseFile(fset, "test.go", src, 0) + if err != nil { + t.Errorf("GoString() output %q failed to parse: %v", expr, err) + + return + } + + // Type-check the parsed file + conf := types.Config{ + Importer: newFeatureImporter(t, fset), + GoVersion: "", + Context: nil, + IgnoreFuncBodies: false, + FakeImportC: false, + Error: nil, + Sizes: nil, + DisableUnusedImportCheck: false, + } + + _, err = conf.Check("main", fset, []*ast.File{file}, nil) + if err != nil { + t.Errorf("GoString() output %q failed type-check: %v", expr, err) + } +} + +// featureImporter implements types.Importer with support for the feature package. +type featureImporter struct { + t *testing.T + fset *token.FileSet + defaultImpl types.Importer + featurePkg *types.Package + projectRoot string +} + +func newFeatureImporter(t *testing.T, fset *token.FileSet) *featureImporter { + t.Helper() + + return &featureImporter{ + t: t, + fset: fset, + defaultImpl: importer.Default(), + featurePkg: nil, + projectRoot: findProjectRoot(t), + } +} + +func (fi *featureImporter) Import(path string) (*types.Package, error) { + if path == "github.com/mpyw/feature" { + if fi.featurePkg != nil { + return fi.featurePkg, nil + } + + pkg, err := fi.loadFeaturePackage() + if err != nil { + return nil, err + } + + fi.featurePkg = pkg + + return pkg, nil + } + + // Standard library + pkg, err := fi.defaultImpl.Import(path) + if err != nil { + panic("unexpected import: " + path) + } + + return pkg, nil +} + +func (fi *featureImporter) loadFeaturePackage() (*types.Package, error) { + entries, err := os.ReadDir(fi.projectRoot) + if err != nil { + return nil, fmt.Errorf("reading project root: %w", err) + } + + // Count .go files (excluding tests) for pre-allocation + var count int + + for _, entry := range entries { + name := entry.Name() + if strings.HasSuffix(name, ".go") && !strings.HasSuffix(name, "_test.go") { + count++ + } + } + + files := make([]*ast.File, 0, count) + + for _, entry := range entries { + name := entry.Name() + if !strings.HasSuffix(name, ".go") || strings.HasSuffix(name, "_test.go") { + continue + } + + file, err := fi.parseFile(name) + if err != nil { + return nil, err + } + + files = append(files, file) + } + + // Type-check the feature package + conf := types.Config{ + Importer: fi.defaultImpl, + GoVersion: "", + Context: nil, + IgnoreFuncBodies: false, + FakeImportC: false, + Error: nil, + Sizes: nil, + DisableUnusedImportCheck: false, + } + + pkg, err := conf.Check("github.com/mpyw/feature", fi.fset, files, nil) + if err != nil { + return nil, fmt.Errorf("type-checking feature package: %w", err) + } + + return pkg, nil +} + +func (fi *featureImporter) parseFile(name string) (*ast.File, error) { + path := filepath.Join(fi.projectRoot, name) + + src, err := os.ReadFile(path) //#nosec G304 -- path is constructed from project root + if err != nil { + return nil, fmt.Errorf("reading %s: %w", name, err) + } + + file, err := parser.ParseFile(fi.fset, name, src, 0) + if err != nil { + return nil, fmt.Errorf("parsing %s: %w", name, err) + } + + return file, nil +} + +// WARNING: GoString outputs a valid Go expression that creates an equivalent key, +// but the resulting key will have a different identity (pointer address). +// Two keys created from the same GoString output will NOT be equal. +func ExampleKey_GoString() { + key := feature.NewNamed[string]("api-key") + fmt.Println(key.GoString()) + + // Output: + // feature.New[string](feature.WithName("api-key")) +} + +// WARNING: GoString outputs a valid Go expression that creates an equivalent key, +// but the resulting key will have a different identity (pointer address). +// Two keys created from the same GoString output will NOT be equal. +func ExampleBoolKey_GoString() { + flag := feature.NewNamedBool("debug-mode") + fmt.Println(flag.GoString()) + + // Output: + // feature.NewBool(feature.WithName("debug-mode")) +} + +// findProjectRoot returns the absolute path to the project root directory. +func findProjectRoot(t *testing.T) string { + t.Helper() + + dir, err := os.Getwd() + if err != nil { + t.Fatalf("failed to get working directory: %v", err) + } + + for { + if _, err := os.Stat(filepath.Join(dir, "go.mod")); err == nil { + return dir + } + + parent := filepath.Dir(dir) + if parent == dir { + t.Fatalf("could not find project root (go.mod)") + } + + dir = parent + } +} diff --git a/inspection.go b/inspection.go index 27db814..fab8bc3 100644 --- a/inspection.go +++ b/inspection.go @@ -66,16 +66,6 @@ func (i Inspection[V]) String() string { return fmt.Sprintf("%s: %v", i.Key.String(), i.Value) } -// GoString returns a Go syntax representation of the inspection. -// This implements fmt.GoStringer. -func (i Inspection[V]) GoString() string { - if !i.Ok { - return fmt.Sprintf("feature.Inspection[%T]{Key: %#v, Ok: false}", *new(V), i.Key) - } - - return fmt.Sprintf("feature.Inspection[%T]{Key: %#v, Value: %#v, Ok: true}", *new(V), i.Key, i.Value) -} - // BoolInspection is a specialized Inspection for boolean feature flags. // It provides convenience methods for working with boolean values. type BoolInspection struct { @@ -106,12 +96,3 @@ func (i BoolInspection) String() string { return i.Inspection.String() } -// GoString returns a Go syntax representation of the inspection. -// This implements fmt.GoStringer. -func (i BoolInspection) GoString() string { - if !i.Ok { - return fmt.Sprintf("feature.BoolInspection{Key: %#v, Ok: false}", i.Key) - } - - return fmt.Sprintf("feature.BoolInspection{Key: %#v, Value: %v, Ok: true}", i.Key, i.Value) -} diff --git a/inspection_test.go b/inspection_test.go index 3956701..a1bc22e 100644 --- a/inspection_test.go +++ b/inspection_test.go @@ -291,57 +291,3 @@ func TestBoolInspectionHelperMethods(t *testing.T) { } }) } - -func TestInspectionGoString(t *testing.T) { - t.Parallel() - - t.Run("Inspection format", func(t *testing.T) { - t.Parallel() - - ctx := context.Background() - key := feature.NewNamed[string]("test-key") - - // Unset - inspection := key.Inspect(ctx) - - goStr := inspection.GoString() - checkContains(t, goStr, "feature.Inspection[string]") - checkContains(t, goStr, "test-key") - checkContains(t, goStr, "Ok: false") - - // Set - ctx = key.WithValue(ctx, "hello") - inspection = key.Inspect(ctx) - - goStr = inspection.GoString() - checkContains(t, goStr, "feature.Inspection[string]") - checkContains(t, goStr, "test-key") - checkContains(t, goStr, "hello") - checkContains(t, goStr, "Ok: true") - }) - - t.Run("BoolInspection format", func(t *testing.T) { - t.Parallel() - - ctx := context.Background() - flag := feature.NewNamedBool("test-flag") - - // Unset - inspection := flag.InspectBool(ctx) - - goStr := inspection.GoString() - checkContains(t, goStr, "feature.BoolInspection") - checkContains(t, goStr, "test-flag") - checkContains(t, goStr, "Ok: false") - - // Set to true - ctx = flag.WithEnabled(ctx) - inspection = flag.InspectBool(ctx) - - goStr = inspection.GoString() - checkContains(t, goStr, "feature.BoolInspection") - checkContains(t, goStr, "test-flag") - checkContains(t, goStr, "Value: true") - checkContains(t, goStr, "Ok: true") - }) -}