buildtags: support //go:build constraints

[mmcloughlin]

New go:build constraints will be preferred in Go 1.17. Therefore avo must support them.

https://golang.org/design/draft-gobuild
golang/go#25348
golang/go#41184

[FiloSottile]

👋 hi! Do you think you'll have time for this before the end of the month? If you'd like, I can look into contributing the change!

[mmcloughlin]

Hey! Sorry, yes I'll look at it this weekend :)

[dmitshur]

FWIW, in Go 1.17 the go/format package (like the gofmt binary) takes care of this¹, so if used, the output is guaranteed to be gofmt'ed (and continue to be over time if there are changes to gofmt behavior in future versions).

Edit: Hmm, I realize now that go/format can't help with the generated .s files, only .go ones. So it's a complete solution will still be more involved.

¹ https://tip.golang.org/doc/go1.17#go/format

[josharian]

@dmitshur I’m not sure whether go/format will continue to rewrite //+build to //go:build forever. Anyway, the nicer //go:build format will likely end up simplifying this code anyway once it can assume 1.17.

[josharian]

@dmitshur should go/format touch .s files, if only the build header lines? Honestly, I wouldn’t mind if it also normalized the rest of the files either. Worth filing an issue (or two?)?

[dmitshur]

should go/format touch .s files, if only the build header lines?

My personal preference would be to keep the scope of cmd/gofmt (and go/format, which is just an importable Go package version thereof) to operate on .go files only. That's hard enough as it is. :)

[mmcloughlin]

Sorry for the really slow response on this, I've been struggling to keep up with my open-source work these days. Thank you for your input and for working around this issue in the meantime.

There seem to be two issues here, firstly how to fix the immediate problem of avo not producing gofmt'ed output, and secondly how avo should support go:build constraints going forward. The main question is how to reconcile avo's existing buildtags package with the new go/build/constraint package. My proposal is below, any thoughts much appreciated.

Now: Update the printing of build constraints to go via go/format, both for the assembly and stub files printers. Since the formatter does not support assembly, this will need to use a slight hack: construct a source file in memory that just has the +build lines and a dummy package header, run it through format.Source and then extract the comment lines from it. This should be enough to ensure that for now avo output is gofmt-compatible in all Go versions.

Note the code to be updated is:

avo/printer/goasm.go

Lines 46 to 49 in b935256

	if len(f.Constraints) > 0 {
	p.NL()
	p.Printf(f.Constraints.GoString())
	}

avo/printer/stubs.go

Lines 21 to 24 in b935256

	if len(f.Constraints) > 0 {
	s.NL()
	s.Printf(f.Constraints.GoString())
	}

The constraints formatter would just be something like: https://play.golang.org/p/UlV0ZypCbCL.

Go 1.18+: The question here is what to do with the buildtags package and associated APIs:

avo/build/global.go

Lines 62 to 72 in b935256

	// Constraints sets build constraints for the file.
	func Constraints(t buildtags.ConstraintsConvertable) { ctx.Constraints(t) }
	// Constraint appends a constraint to the file's build constraints.
	func Constraint(t buildtags.ConstraintConvertable) { ctx.Constraint(t) }
	// ConstraintExpr appends a constraint to the file's build constraints. The
	// constraint to add is parsed from the given expression. The expression should
	// look the same as the content following "// +build " in regular build
	// constraint comments.
	func ConstraintExpr(expr string) { ctx.ConstraintExpr(expr) }

In practice almost everyone has used the ConstraintExpr() function. The buildtags package provides manipulation of +build constraints much like go/build/constraint, though it's a bit too over-engineered. If avo were written today I would never have written the buildtags package at all.

Therefore I think the best approach is that when Go 1.18 lands avo will migrate to the go/build/constraint package instead.

• Add GoBuild(expr string) function for specifying a go:build style constraint
• Add ir.File.GoBuild field
• Deprecate the buildtags package
• Deprecate the associated build.Constraint*() functions
• Deprecate/remove the ir.File.Constraint field

The alternative is to attempt to maintain the buildtags package and types:

• ConstraintExpr(string) attempts to parse go:build first, fallback to +build. Does this work? At the moment the +build prefix is not required, so are there valid +build expressions that would parse as valid go:build expressions with a different meaning? You could avoid this by having the go:build prefix required, even if the +build was not previously.
• Implement conversion between constraint.Expr and corresponding buildtags package types
• The printer package prints according to Go version

Thanks again!