initial commit of bingen#1
Conversation
|
You are seeing this message because GitHub Code Scanning has recently been set up for this repository, or this pull request contains the workflow file for the Code Scanning tool. What Enabling Code Scanning Means:
For more information about GitHub Code Scanning, check out the documentation. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| } | ||
|
|
||
| hrs64, _ := strconv.ParseInt(match[2], 10, 64) | ||
| hrs := sig * int(hrs64) |
| hrs := sig * int(hrs64) | ||
|
|
||
| mins64, _ := strconv.ParseInt(match[3], 10, 64) | ||
| mins := sig * int(mins64) |
There was a problem hiding this comment.
Pull request overview
Initial open-source drop of bingen, a Go code generator for binary codecs driven by @bingen:* annotations, including CI configuration and a set of test fixtures/packages.
Changes:
- Add CLI (
cmd/bingen) plus generator/type/annotation parsing implementation underpkg/. - Add
pkg/utilbuffer + pooling utilities used by generated codecs. - Add extensive
tests/fixtures and generated codec outputs, plus GitHub Actions workflows and Sonar configuration.
Reviewed changes
Copilot reviewed 49 out of 52 changed files in this pull request and generated 13 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/var_test.go | Tests for variable-name generation behavior. |
| tests/streaming/types.go | Streaming test model types. |
| tests/opencost/window.go | OpenCost window/time utilities used as a generation fixture. |
| tests/opencost/bingen.go | @bingen annotations + go:generate directive for opencost fixture. |
| tests/opencost/assetprops.go | OpenCost asset property types (fixture). |
| tests/opencost/allocationprops.go | OpenCost allocation property types (fixture). |
| tests/indent_test.go | Basic indentation test for generator indent helper. |
| tests/generator_test.go | Integration tests that run the generator against fixtures. |
| tests/failing/widget.go | Negative test fixture for invalid field annotations. |
| tests/failing/bingen.go | @bingen annotations for failing fixture package. |
| tests/containerv2/containerv2_codecs.go | Generated codec output for containerv2 fixture. |
| tests/containerv2/container.go | Versioned container fixture type (with migrate hook). |
| tests/containerv2/bingen.go | @bingen annotations for containerv2 fixture package. |
| tests/container/container_codecs.go | Generated codec output for container fixture. |
| tests/container/container.go | Container fixture type (v1). |
| tests/container/bingen.go | @bingen annotations for container fixture package. |
| tests/aliases/bingen.go | @bingen annotations for alias fixture package. |
| tests/aliases/aliases.go | Alias/typedef fixture types. |
| sonar-project.properties | Sonar configuration for sources/tests/coverage exclusions. |
| pkg/util/stringutil/stringutil.go | String interning/banking utility for reducing duplicate allocations. |
| pkg/util/bufferpool_test.go | Unit tests for buffer pooling behavior and concurrency. |
| pkg/util/bufferpool.go | Tiered sync.Pool byte-slice pooling implementation. |
| pkg/util/bufferhelper.go | Low-level binary read/write helpers for primitive types. |
| pkg/util/buffer.go | Main Buffer type providing binary read/write primitives + string optimizations. |
| pkg/types/types.go | AST-based type collection and generator IR creation. |
| pkg/meta/annotationset.go | Parsing/collection of global @bingen annotations into version sets. |
| pkg/meta/annotation.go | Annotation parsing primitives and constants. |
| pkg/generator/vars/vars.go | Variable naming helpers and target formatting utilities. |
| pkg/generator/support.go | Generated-code templates for config, string tables, stream helpers, etc. |
| pkg/generator/streaming.go | Generator support for producing streamable decoders. |
| pkg/generator/indent.go | Indentation helper used by generator output formatting. |
| pkg/generator/debug.go | Debug scope utilities for generator output. |
| pkg/generator/contextscope.go | Scope abstraction for indentation-aware writing. |
| pkg/generator/context.go | Generator context implementation (vars, indentation, options). |
| go.mod | Declares module path and Go version. |
| cmd/bingen/bingen.go | CLI entrypoint that loads types and writes generated codec source. |
| README.md | Public documentation for installation, usage, and feature set. |
| .gitignore | Basic ignore rules for IDEs/binaries/coverage output. |
| .github/workflows/vulnerability-scan.yaml | Trivy vulnerability scanning workflow. |
| .github/workflows/update-go-version.yaml | Scheduled automation to bump Go version in go.mod. |
| .github/workflows/sonar.yaml | SonarCloud scan workflow triggered from Build/Test. |
| .github/workflows/scorecard.yml | OSSF Scorecard workflow for supply-chain checks. |
| .github/workflows/golangci-lint.yaml | Lint workflow via golangci-lint action. |
| .github/workflows/format-check.yaml | gofmt enforcement workflow. |
| .github/workflows/codeql.yaml | CodeQL static analysis workflow. |
| .github/workflows/build-test.yaml | Build/test/coverage workflow for Go packages. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if defaultValue == "" { | ||
| return | ||
| } | ||
| target := ctx.NextVar() | ||
| ctx.Writeln("var %s %s = %s // default", target, t.TypeName(), defaultValue) |
There was a problem hiding this comment.
In StreamSetTypeDefault, non-basic non-nilable fields with an empty default return without yielding anything. That means version-gated fields can be silently omitted from the stream, contradicting the "stream each field" contract and making consumers handle missing fields inconsistently. Consider always yielding the type’s zero value (or an explicit sentinel) when the field is absent, even if no default is provided.
| if defaultValue == "" { | |
| return | |
| } | |
| target := ctx.NextVar() | |
| ctx.Writeln("var %s %s = %s // default", target, t.TypeName(), defaultValue) | |
| target := ctx.NextVar() | |
| if defaultValue == "" { | |
| ctx.Writeln("var %s %s // default", target, t.TypeName()) | |
| } else { | |
| ctx.Writeln("var %s %s = %s // default", target, t.TypeName(), defaultValue) | |
| } |
| if window == "month" { | ||
| // now | ||
| start := now | ||
| // 00:00 today, accounting for timezone offset | ||
| start = start.Truncate(time.Hour * 24) | ||
| start = start.Add(offset) | ||
| // 00:00 1st of this month | ||
| start = start.Add(-24 * time.Hour * time.Duration(start.Day()-1)) | ||
|
|
||
| end := now | ||
|
|
||
| return NewWindow(&start, &end), nil | ||
| } | ||
|
|
||
| if window == "month" { | ||
| // now | ||
| start := now |
There was a problem hiding this comment.
There are two identical if window == "month" branches back-to-back. The second one is dead/unreachable and likely meant to be a different named window. Remove the duplicate or fix the condition to the intended window keyword.
| func (w Window) MarshalJSON() ([]byte, error) { | ||
| buffer := bytes.NewBufferString("{") | ||
| buffer.WriteString(fmt.Sprintf("\"start\":\"%s\",", w.start.Format(time.RFC3339))) | ||
| buffer.WriteString(fmt.Sprintf("\"end\":\"%s\"", w.end.Format(time.RFC3339))) | ||
| buffer.WriteString("}") |
There was a problem hiding this comment.
MarshalJSON dereferences w.start and w.end without nil checks, but Window explicitly allows open intervals (nil start/end). Calling MarshalJSON on an open window will panic. Either return an error for open windows or encode nil/omitted start/end safely.
| var labelStrs []string | ||
| for k, prop := range p.Labels { | ||
| labelStrs = append(labelStrs, fmt.Sprintf("%s:%s", k, prop)) | ||
| } | ||
| strs = append(strs, fmt.Sprintf("Labels:{%s}", strings.Join(strs, ","))) | ||
|
|
||
| var AnnotationStrs []string | ||
| for k, prop := range p.Annotations { | ||
| AnnotationStrs = append(AnnotationStrs, fmt.Sprintf("%s:%s", k, prop)) | ||
| } | ||
| strs = append(strs, fmt.Sprintf("Annotations:{%s}", strings.Join(strs, ","))) | ||
|
|
There was a problem hiding this comment.
AllocationProperties.String() builds labelStrs/AnnotationStrs but then formats Labels/Annotations using strings.Join(strs, ",") instead of joining the label/annotation slices. This produces incorrect output (and for Labels/Annotations currently reuses the already-built strs). Use the dedicated labelStrs and AnnotationStrs slices when formatting these sections.
| dirs := strings.Split(wd, "/") | ||
| if dirs[len(dirs)-1] == "tests" { | ||
| testDir = wd | ||
| return | ||
| } | ||
|
|
||
| for i := len(dirs); i >= 0; i-- { | ||
|
|
||
| tDir := filepath.Join(filepath.Join(dirs[:i]...), "tests") | ||
| if dirExists(tDir) { | ||
| testDir = tDir |
There was a problem hiding this comment.
getTestDir splits the working directory using strings.Split(wd, "/"), which is not portable (will fail on Windows paths). Use filepath utilities (e.g., filepath.Dir/filepath.SplitList or splitting on os.PathSeparator) to walk up directories in an OS-agnostic way.
| func (w Window) Duration() time.Duration { | ||
| if w.IsOpen() { | ||
| // TODO test | ||
| return time.Duration(math.Inf(1.0)) | ||
| } |
There was a problem hiding this comment.
Window.Duration() attempts to represent an open window duration as time.Duration(math.Inf(1.0)). Converting +Inf to an integer-based time.Duration is not meaningful and can yield overflow/implementation-dependent results. Consider returning a separate (time.Duration, bool)/error for open windows, or using a well-defined sentinel (e.g. math.MaxInt64) documented as "infinite".
| thanosDur := 3 * time.Hour | ||
| if offset < thanosDur && true { | ||
| diff := thanosDur - offset | ||
| offset += diff |
There was a problem hiding this comment.
if offset < thanosDur && true has a redundant && true, which makes this look like a leftover debug flag or incomplete conditional. If this is intended to be gated (e.g., only when using Thanos), replace the constant with a real condition or remove it.
| func DurationOffsetStrings(_ time.Duration, _ time.Duration) (string, string) { | ||
| return "", "" | ||
| } |
There was a problem hiding this comment.
DurationOffsetStrings is currently a stub that always returns empty strings. This breaks DurationOffsetForPrometheus and Window.DurationOffsetStrings, which will produce invalid query fragments. Implement the actual formatting logic or remove/guard the callers so this can’t return empty results silently.
| for i := 0; i < 17; i++ { | ||
| length := 1 << i | ||
| bp.pools[i].New = func() any { | ||
| return make([]byte, length) | ||
| } | ||
| } |
There was a problem hiding this comment.
newBufferPool captures the loop-local length variable in the sync.Pool.New closure. Because length is reassigned each iteration, all pools will end up allocating the same (final) size slice, defeating tiering and wasting memory. Capture the value per-iteration (e.g., assign to a new local and close over that) before setting New.
| } | ||
|
|
||
| fiScope := ctx.PushScope("fi = BingenFieldInfo{") | ||
| fiScope.Writeln("Type: reflect.TypeFor[%s](),", f.Type.Name()) |
There was a problem hiding this comment.
BingenFieldInfo.Type is built using reflect.TypeFor[%s]() with f.Type.Name(). For pointer fields, Name() does not include the *, so the reflected type will be wrong (e.g. T instead of *T). Use the pointer-qualified type name (e.g. f.Type.TypeName()) when generating this reflect type.
| fiScope.Writeln("Type: reflect.TypeFor[%s](),", f.Type.Name()) | |
| fiScope.Writeln("Type: reflect.TypeFor[%s](),", f.Type.TypeName()) |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 49 out of 52 changed files in this pull request and generated 9 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func (gcf *genContextFactory) NewContext(opts *types.GenerateTypeOpts) GeneratorContext { | ||
| if opts == nil { | ||
| opts = &types.GenerateTypeOpts{} | ||
| } | ||
|
|
||
| return &genContext{ | ||
| b: gcf.buffer, | ||
| i: NewIndent(4), | ||
| vsc: fmt.Sprintf("%sCodecVersion", titleName(opts.SetName)), | ||
| opts: opts, | ||
| loops: vars.NewAlphaVarNames(vars.SkipAllBut(vars.I, vars.J)...), | ||
| maps: vars.NewAlphaVarNames(vars.SkipAllBut(vars.Z, vars.V)...), | ||
| vars: vars.NewAlphaVarNames(vars.I, vars.J, vars.K, vars.Z, vars.V), | ||
| oks: vars.NewAlphaVarNames(), | ||
| errs: vars.NewAlphaVarNames(), | ||
| errHandler: gcf.errorHandler, | ||
| } |
There was a problem hiding this comment.
NewContext ignores the indentionSize provided to NewGeneratorContextFactory and always uses NewIndent(4). This makes the factory parameter ineffective and prevents callers from controlling indentation. Use gcf.indentionSize when constructing the Indent implementation.
| if window == "month" { | ||
| // now | ||
| start := now | ||
| // 00:00 today, accounting for timezone offset | ||
| start = start.Truncate(time.Hour * 24) | ||
| start = start.Add(offset) | ||
| // 00:00 1st of this month | ||
| start = start.Add(-24 * time.Hour * time.Duration(start.Day()-1)) | ||
|
|
||
| end := now | ||
|
|
||
| return NewWindow(&start, &end), nil | ||
| } |
There was a problem hiding this comment.
There are two identical if window == "month" blocks; the second one is dead code (the first one returns). Remove the duplicate block to avoid confusion and keep parsing logic maintainable.
| thanosDur := 3 * time.Hour | ||
| if offset < thanosDur && true { | ||
| diff := thanosDur - offset | ||
| offset += diff | ||
| duration -= diff | ||
| } |
There was a problem hiding this comment.
if offset < thanosDur && true { contains a redundant && true, which makes the condition harder to read and suggests a leftover debug toggle. Remove && true (or replace with a real feature flag/config check if intended).
| func TestBasicIndent(t *testing.T) { | ||
| i := generator.NewIndent(4) | ||
| i.OutN(2) | ||
| i.In() | ||
| fmt.Printf("%sTest\n", i) | ||
| } |
There was a problem hiding this comment.
This test only prints the indentation result and has no assertions, so it can pass even if Indent is broken. Consider asserting on the expected indentation string (or using t.Log if the output is only diagnostic).
| ### Install | ||
| Go modules are verified against a public database on known modules. Since this repository is private, you have to explicitly instruct `go get` to bypass public verification. To do this, set the `GOPRIVATE` environment variable to include this repository. For example: | ||
| ```bash | ||
| export GOPRIVATE=github.com/opencost/bingen | ||
| ``` |
There was a problem hiding this comment.
README install instructions state the repository is private and require setting GOPRIVATE, but the PR description says this is being open sourced. Update the README to reflect the intended public installation flow (or clarify if parts are still private) to avoid confusing new users.
| r := b.String() | ||
| if r != "" { | ||
| t.Errorf(r) | ||
| return false | ||
| } |
There was a problem hiding this comment.
t.Errorf(r) treats r as a format string; if r ever contains % it will be interpreted as formatting verbs and can cause confusing output. Use t.Error(r) or t.Errorf("%s", r) to log the built message safely.
| // create the key and value using the func (the key could be deallocated later) | ||
| value := f() | ||
| sb.m[value] = value |
There was a problem hiding this comment.
LoadOrStoreFunc stores the computed value into the map using value as the key (sb.m[value] = value) instead of using the provided key. This breaks de-duplication (subsequent lookups by the original key will miss) and can cause unbounded growth. Store under key (or consistently use value for both lookup and storage, but then the initial existence check must also use value).
| // create the key and value using the func (the key could be deallocated later) | |
| value := f() | |
| sb.m[value] = value | |
| // create the value using the func; store it under the provided lookup key | |
| value := f() | |
| sb.m[key] = value |
| // TODO:CLEANUP make this unmarshalable (make Start and End public) | ||
| func (w Window) MarshalJSON() ([]byte, error) { | ||
| buffer := bytes.NewBufferString("{") | ||
| buffer.WriteString(fmt.Sprintf("\"start\":\"%s\",", w.start.Format(time.RFC3339))) | ||
| buffer.WriteString(fmt.Sprintf("\"end\":\"%s\"", w.end.Format(time.RFC3339))) | ||
| buffer.WriteString("}") | ||
| return buffer.Bytes(), nil | ||
| } |
There was a problem hiding this comment.
MarshalJSON dereferences w.start/w.end unconditionally (w.start.Format(...), w.end.Format(...)). For open windows (where either is nil), this will panic during JSON marshaling. Handle nil start/end explicitly (e.g., encode as null/empty string, or return an error for open windows).
| func (w Window) Set(start, end *time.Time) { | ||
| w.start = start | ||
| w.end = end | ||
| } |
There was a problem hiding this comment.
Set uses a value receiver (func (w Window) Set(...)), so assignments to w.start/w.end only modify a copy and have no effect on the caller. Change the receiver to *Window (or return the updated Window) so the method actually updates the instance.
open sourcing of OpenCost bingen