uber-go / cff Goto Github PK

We should add a release workflow for CI that fires automatically if we push a tag beginning with v.
This should:

• extract changelog entries for that release and update the release notes. I've had success with parse-changelog in extracting changelog sections.
• optionally, use goreleaser to build and publish binaries for different platforms to github.

We believe that cff works on Windows just fine,
but the tests currently have hard-coded Linu paths.

This is tracking fixing those and re-enabling Windows builds.

We need a README that covers:

• Introduction: What is CFF
• Installation: How to install it
• Get Started: A short step-by-step tutorial that goes from zero to working example
• More
  • difference between Parallel and Flow
  • example of using Parallel

We'll want to link to the API reference from the README as well.

That matches what go test, go build, etc. expect.

Tests inside internal/tests and examples/ all introduce dependencies that aren't CFF dependencies.
We should eliminate these by using unpublished submodules for integration tests.

We should delete the provided emitter implementations to reduce the dependency footprint of using cff.
Custom implementations are easy to write, and requirements vary.

More importantly, this simplifies CFF's API contract:
it removes the messages we'll log and the metrics we'll emit from the contract.

Alternatively, we can move them to subpackages.
If we keep them around, we should reconsider having their constructors (ZapEmitter and TallyEmitter) return interfaces instead of structs.

We should add a make lint target and a CI component for it that does:

• staticcheck
• verifies that all code is gofmt-ed

We should not do this until #3 so that generated code samples aren't considered part of the linting.

After the golang 1.22 upgrade and latest cff installation, cff generation is failing with NilPointer Dereference.

panic: runtime error: invalid memory address or nil pointer dereference [recovered]
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x63e999e] goroutine 349 [running]:
go/types. (*Checker) .handleBailout (0x000162200, 0x000035c60)
/usr/local/go/src/go/types/check.go:367 +0x88
panic (10×6599a0, 06/86
/usr/local/go/src/runtime/panic.go:770 +0x132
go/types. (*StdSizes). Sizeof (0x0, {0x65e97f8, 0x67bcf20})
/us/local/go/src/go/types/sizes.go:228 +0x31e
go/types. (*Config). sizeof(...)
/usr/local/go/src/go/types/sizes.go:333
go/types. representableConst. func1(0x65e97f8?, 0x67bcf20?})
/us/local/go/src/go/types/const.go: 76 +0x9e
go/types representableConst ({0x65eb670, 0x67b0500}, 0x000162200, 0x67bcf20, 0x000034078)
/us/local/go/src/go/types/const.go:92 +0x192
go/types. (*Checker). representation(0x000162200, 0x0007cddcO, 0x67bcf20)
/us/local/go/src/go/types/const.go:256 +0x65
go/types. (*Checker). implicitTypeAndValue(0x000162200, 0xc0007cddcO, {0x6597f8, 0x67bcf20})
/us/local/go/src/go/types/expr.go: 375 +0x30d
go/types. (*Checker). convertUntyped (0xc000162200, 0xc0007cddcO, {0x65e97f8, 0x67bcf20})
/us/local/go/src/go/types/const.go: 289 +0x3f
go/types. (*Checker) -matchTypes (0x000162200, 0x0007cdd80, 0x0007cddcO)
/us/local/go/src/go/types/expr.go:926 +0x79
go/types. (*Checker). binary(0x000162200, 0x0007cdd80, {665eac80, Oxc0002fe7eO}, {0x65ea770, 0xc0003425a0}, {0x65ea7d0
/us/local/go/src/go/types/expr.go: 800 +0x166
go/types. (*Checker).exprInternal (0x000162200, 0x0, 0x0007dd80, {0x65ac80, 0x0002fe7eO}, {0x0, 0x0})
/us/local/go/src/go/types/expr.go: 1416 +0x206
go/types. (*Checker). rawExp(0x000162200, 0x0, 0x0007cdd80, {0x65eac80?, 0x0002fe70?, {0x0?, 0x07}, 0x0)
/us/local/go/src/go/types/expr.go: 979 +0x19e
go/types. (*Checker). expr(0x000162200, 0x65a060?, 0x0007cdd80, {0x65eac80?, 0x0002fe7e07})
/usr/local/go/src/go/types/expr.go:1513 +0x30
go/types. (*Checker). stmt(0x000162200, 0x0, {0x65eafe®, 0xc0007cce80})
/usr/local/go/src/go/types/stmt.go:570 +0x11f2
go/types. (*Checker).stmtList(0x000162200, 0x0, {0x000342840?, 0x0?, 0x077)
/us/local/go/src/go/types/stmt-go: 121 +0x85
go/types. (*Checker). funcBody (0x000162200, 0x6597f8?, {0x000286d8?, 0x67bd1002}, 0xc0007cda40, 0xc0002fe870, {0x07, 0
/us/local/go/src/go/types/stmt.go:41 +0x331
go/types. (*Checker). funcDecl. func1()
/us/local/go/src/go/types/decl.go: 852 +0x3a
go/types. (*Checker) -processDelayed (0xc000162200, 0x0)
/us/local/go/src/go/types/check. go: 467 +0x162
go/types. (*Checker).checkFiles(0x000162200, {0x0002a8270, 0x1, 0x1})
/us/local/go/src/go/types/check.go: 411 +0x1cc
go/types. *Checker). Files(...)
/us/local/go/src/go/types/ check.go: 372
golang.org/x/tools/go/packages. (*loader). loadPackage(0xc00014c000, 0xc0011bd7c0)
/Users/xyz/go/pkg/mod/golang.org/x/[email protected]/go/packages/packages.go:1001 +0x76f
golang. org/x/tools/go/packages. (*loader). LoadRecursive. func1()
/Users/xyz/go/pkg/mod/golang.org/x/[email protected]/go/packages/packages.go:838 +0x1a9
sync. (*0nce) . doS low(0x736e692209090a2c?, 0x226f672e74636570?) /usr/local/go/src/sync/once.go:74 +0xc2
sync. (*0nce) .Do (...)
/usr/local/go/src/sync/once.go: 65
golang.org/x/tools/go/packages. (*loader). loadRecursive(0x6f4764656c69706d?, 0x203a2273656c6946?)
/Users/xyz/go/pkg/mod/golang.org/x/[email protected]/go/packages/packages.go:826 +0x4a
golang.org/x/tools/go/packages. (*loader). loadRecursive. func1.1(0x642209090a2c226f?)
/Users/xyz/go/pkg/mod/golang.org/x/[email protected]/go/packages/packages.go:833 +0x26
created by golang.org/x/tools/go/packages. (*loader). loadRecursive.funcl in goroutine 342
/Users/xyz/go/pkg/mod/golang.org/x/[email protected]/go/packages/packages.go:832

In #52,
we disabled validation of pkg.go.dev links
because without a release published,
the repository isn't yet present on pkg.go.dev.

This can be fixed once a release is tagged.

We use sally for go.uber.org.
We need to add an entry there for cff.

We need to set up CI for the repository.
We can model it after Fx or Zap's setup.
It should include the following checks:

• run tests with race detection
• verify generated code is up to date
• post coverage data to codecov.io (probably opt-out generated files?)

Eventually, since cff has a binary component, we'll likely also want to set up goreleaser to publish binaries to GitHub (which will likely also affect install instructions), but that's all open sourcing.

'cff' currently suffixes generated files with _gen.
This means we cannot use cff in test files because they'll become foo_test_gen.go.
We should special case test files:
if the file name ends with _test.go, the generated file should be foo_gen_test.go.

Generated files will currently show in GitHub PRs.
We should mark them generated with gitattributes per https://docs.github.com/en/repositories/working-with-files/managing-files/customizing-how-changed-files-appear-on-github.

A major piece of debt in aquaregia_test (which verifies error messages for failing test cases)
is that it just searches for N instance of error X in the output.
We can do better: we should assert error messages at specific positions.

One way we could do this is how analysistest expects error messages: with // want comments next to where the errors are expected.

We should also move all the test cases to a testdata/ folder so we don't have to tag them with the failing constraint.

CI currently uses a plain 'cff' binary to generate code.
With Go 1.20's integration testing coverage support,
we can use a coverage-instrumented binary here and feed those results into codecov as well.

The 'cff' build constraint is currently optional.
If someone fails to add the constraint, we won't generate the inverse of the constraint in the generated file.
This is a problem because if someone does that, they'll start seeing "duplicate declaration" errors.

We should make the 'cff' build constraint required for files processed by cff.
However, for our internal consumption of the tool, the constraint is optional.
To that end, the "constraint is required" condition should be modifiable with a simple patch.
Perhaps a global _requireBuildConstraint bool in the main package.

Once we have a CI job that verifies that the generated code is up to date (per #6),
we no longer need magic_clean_test to verify that the generated code is up to date.
We can delete the test.

Depends on #6

The tests in internal/tests currently have a hacky "invoke cff twice" setup:

• once to generate the code for the test that needs --auto-instrument
• once to generate everything else

The test that relies on auto instrument is tagged with autoinstrument,
but we generate everything with auto-instrument,
and then replace it in the second call.

This is inefficient and we should fix it.

Right now, if a task panics, we do roughly this:

fmt.Errorf("task panic: %w", panicval)

We should instead use a structured error similar to the following:

type PanicError struct {
  Value any
  Traceback string
}

So that users can determine if an error was due to a panic or something else,
and act upon that information however they choose.

BeginFlow is used by cff.Parallel and cff.Flow.
It should be renamed to BeginScheduler or NewScheduler.

Instead of these several scoped objects,
we should maybe consider something like fxevent.Logger
where we have a single interface that accepts event objects,
and we have several kinds of event objects.
I'm not certain that this is desirable, but it's worth exploring.