Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

proposal: cmd/go: allow && and || operators and parentheses in build tags #25348

Closed
mundaym opened this issue May 11, 2018 · 34 comments
Closed

Comments

@mundaym
Copy link
Member

mundaym commented May 11, 2018

The review for CL 107628 reminded me of this idea,so I wanted to make a quick note of it.

I think that this:

// +build (!arm64 && !s390x) || (arm64 && !go1.11) || gccgo || appengine

Would be a lot easier for a Go developer to parse (and verify) than the current syntax we use for build tags:

// +build !arm64,!s390x arm64,!go1.11 gccgo appengine

Build tags are hard to test so we should make them as easy to review as possible.

This might be a Go 2 suggestion, but it could probably also be added to Go 1. For Go 2 I would also be tempted to remove multi-line build tags which are also difficult for Go developers to parse.

@gopherbot gopherbot added this to the Proposal milestone May 11, 2018
@robpike
Copy link
Contributor

robpike commented May 11, 2018

I agree that the build tag logic is too cutesy but it's hard to change outright. It might be possible to support both forms transparently, or to use a different keyword such as +build2.

I never liked the current form. I'm not even sure I could ever do anything complex without reading the docs carefully.

@AlexRouSg
Copy link
Contributor

AlexRouSg commented May 11, 2018

Instead of +build2, if there's going to be a new keyword anyway, lets take this chance to be consistent and have something like go:build

@cznic
Copy link
Contributor

cznic commented May 11, 2018

I don't like the current syntax at all, but breaking every 3rd part tool relying on it, created in the last nearly decade, shouldn't be taken lightly. Ugly syntax or not - it's not broken.

Also, going from a regular language to a CF one may result in some tools/scripts never getting fixed/upgraded with a full parser.

@rsc
Copy link
Contributor

rsc commented May 21, 2018

I'm sorry for the old syntax, but I was trying to keep people from doing very complex things. Probably that was a mistake, but the current syntax is general enough that we probably shouldn't add a second one.

@josharian
Copy link
Contributor

Perhaps someone could write a simple third party tool that accepts a simpler syntax on the command line (with && and ||) and spits out appropriate build tag lines.

@josharian
Copy link
Contributor

And as a bonus if the tool also went the other way, you could have a very nice IDE integration, in which you could simply edit pseudo-build tags. cc @mvdan @dominikh @fatih

@mvdan
Copy link
Member

mvdan commented May 22, 2018

Would be great if this was part of x/tools, since it seems like the main users would be the official Go repos themselves. Perhaps x/tools/cmd/buildtags?

@mengzhuo
Copy link
Contributor

@josharian you might need to consider "go version" in build tags is different from what it looks like.

!go1.11 seems to be ( go1.x < go1.11 )

however it really means NOT ( if go1.x in ( go1.1 to go1.11) )

@rsc
Copy link
Contributor

rsc commented Jun 4, 2018

As I said above, introducing and encouraging extra complexity here is probably not a win. I think we should leave well enough alone.

@rsc rsc closed this as completed Jun 4, 2018
@robpike
Copy link
Contributor

robpike commented Jun 4, 2018

All proposals add complexity.

But for the record: This is just the kind of change that could add clarity to the code in Go 2.

@smasher164
Copy link
Member

Reopening both to merge in #36279, and to reevaluate consensus on this proposal.

@CAFxX
Copy link
Contributor

CAFxX commented Dec 31, 2019

Perhaps someone could write a simple third party tool that accepts a simpler syntax on the command line (with && and ||) and spits out appropriate build tag lines.

This does not help with readability though, that is arguably much more important than convenience when writing.

@mvdan
Copy link
Member

mvdan commented Dec 31, 2019

For what it's worth, I implemented Josh's idea as a prototype in https://golang.org/cl/117735. I don't think it has the issue that @CAFxX mentions, because both the Go-like and final syntax are next to each other in the final source:

//go:generate buildtags appengine || (!arm64 && !s390x) || (arm64 && !go1.11) || gccgo
// +build appengine !arm64,!s390x arm64,!go1.11 gccgo

In other words, the //go:generate buildtags line is the source of truth, and maintains the parseable // +build line right below it. So if you find the Go syntax easier to read, like me, you can read the first line.

I only abandoned the CL because I couldn't find a single reviewer in over a year. But if there is interest, I can reopen it and do a rebase. Even if we decide to go with direct support for +build2 or go:build, it could be a way for us to experiment with the feature on any Go version.

@rsc rsc added this to Incoming in Proposals (old) Jan 8, 2020
@rsc
Copy link
Contributor

rsc commented Jan 8, 2020

I wrote above in #25348 (comment):

I'm sorry for the old syntax, but I was trying to keep people from doing very complex things. Probably that was a mistake, but the current syntax is general enough that we probably shouldn't add a second one.

This still seems true to me. Thanks for dedup'ing #36279 into this proposal, but this still seems like a likely decline.

Leaving open for a week for final comments.

@rsc rsc moved this from Incoming to Likely Decline in Proposals (old) Jan 8, 2020
@rasky
Copy link
Member

rasky commented Jan 8, 2020

I’m not sure I understand the rationale for declining. Everybody agrees that the current syntax is confusing. Go2 seems a good avenue to fix it for good. We also have a way to deprecate the current syntax (with go version checks) and move on to the new syntax for good (rather than having two syntaxes working at the same time). Also, it would be absolutely trivial to provide a go fix translation (heck, even go fmt could change it if we wanted do).

Why should we avoid changing just because the current one is “general enough”? The proposal itself was not even commented upon, and it provides a far superior syntax, much easier to read and understand.

If this proposal must be declined, i think it needs a stronger rationale than “the current syntax is general enough”.

@ianlancetaylor
Copy link
Contributor

I don't see how we can realistically remove the old syntax. We can only use version checks to deprecate existing syntax in the most extreme of situations, or for constructs that are very rarely used, as doing that forces people to modify their code as soon as they want to take advantage of new language versions. Every deprecation is a barrier to moving to new versions of Go. This doesn't seem to me to meet that bar.

@josharian
Copy link
Contributor

josharian commented Jan 8, 2020

@ianlancetaylor right but if both flavors were accepted and (say) gofmt rewrote to the nicer form, over time, the nicer form would dominate. And it wouldn't matter that we had an old, vestigial form.

I'd say the cost in question is that of having two formats instead of one. Note that having gofmt rewrite to the newer form eliminates many of those costs, such as training.

@jimmyfrasche
Copy link
Member

jimmyfrasche commented Jan 8, 2020

Introduce the new syntax in go1.X. For modules that are go1.X or greater, have go fmt automatically rewrite old syntax to new or go fix to update it all at once.

Later, introduce go1.Y. Have modules that are go1.Y error out on the old syntax and require the user to go fmt/fix it for the new syntax.

Even if the old syntax would always be there in the tool chain, it would cease to be user facing quickly and even if the upgrade is forced on a maintainer it should be relatively painless.

Edit: essentially what @josharian said

@ianlancetaylor
Copy link
Contributor

@jimmyfrasche My point is that requiring the user to update for the new syntax is a painful step. Of course it can be done. The question is whether we get enough benefit to impose that pain.

But to be clear my only point is that I don't think we can remove the old syntax. I'm agnostic as to whether we should also support a new syntax.

@rasky
Copy link
Member

rasky commented Jan 9, 2020

@ianlancetaylor when you say “we can’t remove”, I don’t understand whether you mean “we can’t disable support for the old syntax starting from Go 1.X for any X” or “we can’t remove the code parsing the old syntax from the toolchain ever”. I’m assuming you mean the former.

Why don’t we just try? It’s a relatively isolated corner of the language syntax, and the worst that can happen is that we make a giant mistake of the migration plan and we end up having two syntax for build tags forever. The best it can happen is that we learn if the Go ecosystem/community moves fast enough to allow a backward incompatible change to happen in a couple (?) of years. It’s a good story to write either case, and the worst case seems far less worse than doing the same mistake on other parts of the language.

I think this issue is a good “reality check” for the Go 2 migration plan. My feeling is that if this can’t be done, then Go 2 can’t succeed in changing mistakes of the past (maybe it will succeed in other areas).

@mvdan
Copy link
Member

mvdan commented Jan 9, 2020

I generally agree with @rasky. People can upgrade to newer versions of the Go language (potentially with breaking changes like disabling the old syntax here) at their pace, since they can keep an older version of Go in their go.mod files as needed.

@networkimprov
Copy link

@rasky we've been told there is no plan to remove (vs deprecate) mistakes of the past; I assume that's still true. "Go 2" is just a tag for backwards-compatible additions.

@ianlancetaylor
Copy link
Contributor

@rasky I mean that we can never remove the old build tags syntax. I'm not sure what you mean by "why don't we just try?" Try what, exactly?

I outlined my thoughts on Go 2 transitions at https://github.com/golang/proposal/blob/master/design/28221-go2-transitions.md. Following that plan, we can remove items from the language if necessary, but we have to consider that a very heavy cost. We should only do it for clear mistakes that lead people to write incorrect programs. We shouldn't do it just to make things nicer.

the worst that can happen is that we make a giant mistake of the migration plan

No, the worst that can happen is that people find it painful to update their existing packages, and decide that Go is an unstable platform, and move to something else. That is hyperbole in this case, but I think it's the right general guideline to keep in mind when it comes to breaking existing code.

@tandr
Copy link

tandr commented Jan 9, 2020

@rasky we've been told there is no plan to remove (vs deprecate) mistakes of the past; I assume that's still true. "Go 2" is just a tag for backwards-compatible additions.

Off-topic, but yeah... that whole excitement of "Go2" becoming like a Modula-2 moment for Pascal (or C++ for C) kind of fizzled out by now. "Go3" anyone?

@smasher164
Copy link
Member

I'd say the cost in question is that of having two formats instead of one. Note that having gofmt rewrite to the newer form eliminates many of those costs, such as training.

I'm in favor of adopting @josharian's suggestion to support both formats. The two formats are mutually exclusive, except for the case of a single variable, where the constraint evaluates the same regardless of the format. We'd just have to look for (, ), ||, or && when parsing the // +build constraint.

@rsc rsc moved this from Likely Decline to Active in Proposals (old) Jan 15, 2020
@rsc
Copy link
Contributor

rsc commented Jan 15, 2020

Changed back to active discussion.

@griesemer
Copy link
Contributor

I didn't have a particularly strong opinion about this topic but I must admit that the existing notation always confuses me, too. In math x y means x*y (a multiplicative operation) and that makes me think x y means x & y (also a multiplicative operation). Consequently I read a b, c d as a && b || c && d but that is exactly the opposite of what the current notation means. The only part that seems obvious to me is the "!". The existing notation works of course and (probably, @rsc ?) was born from a desire to have very simple and efficient implementation, but the time has probably come to make this nicer.

After reviewing this discussion and briefly looking at @mvdan 's code I also vote for @josharian 's suggestion. Accept both variants and have a tool to rewrite to the new form (it doesn't have to be gofmt; in general now we expect that gofmt does not modify a source file beyond white space formatting). The tools is a nice-to-have, accepting the new format and documenting it would be the first step.

@mvdan 's code uses the go/parser to parse the boolean expression for simplicity (a single function call). If that is not efficient enough for some reason, it's trivial to write a simple boolean expression parser. Splitting into tokens doesn't require a full blown scanner, it probably can be done inline or on a line base.

@mvdan
Copy link
Member

mvdan commented Jan 16, 2020

The existing notation works of course and (probably, @rsc ?) was born from a desire to have very simple and efficient implementation

If we want to continue keeping users from writing complex logic, we could always add arbitrary limits to the expresion parser. For example, error if one tries to use more than two levels of nested parentheses. We could even allow just one level of parentheses, which is equivalent to the complexity of the current expression language, I think.

Accept both variants and have a tool to rewrite to the new form

The way I'm reading this, it seems like the idea is to skip the go generate middle ground entirely, and add support into the build tool directly. Do we have an idea what the tool could be? Perhaps it's time to revive go fix?

In any case, I'm happy to donate or re-submit whatever parts of the abandoned CL that could still be useful.

@rsc
Copy link
Contributor

rsc commented Feb 12, 2020

For the record, I'm coming around to the idea of doing this, but I want to work through the transition plan. I'm not ignoring this issue. :-)

@rsc
Copy link
Contributor

rsc commented Mar 11, 2020

Still working on plan and not ignoring.

@rsc
Copy link
Contributor

rsc commented Mar 25, 2020

Almost have a proposal written. Soon!

@rsc
Copy link
Contributor

rsc commented Apr 8, 2020

I have a draft design (not a proposal) that I'd like to discuss with the community, but it seems like now is not a good time with the pandemic going on. Putting on hold, but I will definitely return to this and start a discussion when it seems more appropriate to do so.

@rsc rsc moved this from Active to Hold in Proposals (old) Apr 8, 2020
@rsc rsc changed the title proposal: allow && and || operators and parentheses in build tags proposal: cmd/go: allow && and || operators and parentheses in build tags Apr 8, 2020
@rsc
Copy link
Contributor

rsc commented Jun 30, 2020

Delayed by all that is 2020, but I've published the draft design I was working on in early April.

Design: https://golang.org/s/go-build-design
Video: https://golang.org/s/go-build-video
Code: https://golang.org/s/go-build-code
Q&A: https://golang.org/s/go-build-reddit

For Q&A about the draft design, let's use the Reddit thread. If there is a lot of discussion, my hope is that Reddit's proper threading will be more manageable than a long GitHub sequence here.

(This proposal issue is still officially on hold anyway. I'm not officially proposing anything, just sharing the design for feedback.)

@rsc
Copy link
Contributor

rsc commented Sep 2, 2020

The response to the draft design has been uniformly positive, so I've proposed in #41184 to accept the design, doing the prep in Go 1.16, landing the main functionality in Go 1.17, and finishing the transition in Go 1.18.

Will close this now, superseded by #41184. Thanks for the excellent discussion everyone.

@rsc rsc closed this as completed Sep 2, 2020
@rsc rsc moved this from Hold to Declined in Proposals (old) Sep 2, 2020
@golang golang locked and limited conversation to collaborators Sep 2, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
No open projects
Development

No branches or pull requests