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: generate allow arguments to span multiple lines #46050

Closed
andig opened this issue May 8, 2021 · 15 comments
Closed

proposal: cmd/go: generate allow arguments to span multiple lines #46050

andig opened this issue May 8, 2021 · 15 comments

Comments

@andig
Copy link
Contributor

@andig andig commented May 8, 2021

Go does encourage use of generated code by using go generate and the respective //go:generate build comments.

Working with implementing optional interfaces that a go object might chose to provide to extend a base interface type at runtime, I've come across the need to write long build comments like https://github.com/andig/evcc/blob/master/internal/vehicle/vehicle.go#L28

For readability it would be great if these could be supplied as multi-line comment. This could look like this:

//go:generate go run ../../cmd/tools/decorate.go 
//-p vehicle -f decorateVehicle -b api.Vehicle -o vehicle_decorators 
//-t "api.ChargeState,Status,func() (api.ChargeStatus, error)" 
//-t "api.VehicleRange,Range,func() (int64, error)"

I would like to propose to allow multi-line comments, given that the comment marker // is not followed by a space ( ).

@seankhliao seankhliao changed the title build: allow multi-line build comments proposal: cmd/go: generate allow arguments to span multiple lines May 8, 2021
@gopherbot gopherbot added this to the Proposal milestone May 8, 2021
@seankhliao
Copy link
Contributor

@seankhliao seankhliao commented May 8, 2021

since comment structure is not enforced, how would it differentiate between the below being a single directive go:generate do something foo or 2 separate directives go:generate do something and foo (a directive recognized by an external tool)?

//go:generate do something
//foo
@andig
Copy link
Contributor Author

@andig andig commented May 8, 2021

A (partial) remedy might be requiring a shell-line line separator like so:

//go:generate go run ../../cmd/tools/decorate.go \
//-p vehicle -f decorateVehicle -b api.Vehicle -o vehicle_decorators \
//-t "api.ChargeState,Status,func() (api.ChargeStatus, error)" \
//-t "api.VehicleRange,Range,func() (int64, error)"

Is it possible to query github for such structures to see if that would break existing repos?

Alternatively (though much weaker) could be a heuristic of treating ^- as a continued comment statement.

@mvdan
Copy link
Member

@mvdan mvdan commented May 8, 2021

I don't think it's wise to make go:generate comments more like shell input, to be honest. The parsing of go:generate does currently support basic quoting, as that is a common need. But I think it's deliberate to not go further than that, in the direction of supporting more shell-like features.

There's also the question of whether such "escaped newlines" should also apply to other directives, like go:build and go:embed. Hopefully not, but that would make them more inconsistent, as right now they can all be parsed the same way - just taking the entire line that starts with //go:.

Also, how often are long go:generate lines a necessity? I've written dozens of them in many projects before, and usually they're well under 100 characters. I can't say I've seen any generate directives that were long enough to be painful to read or maintain. Perhaps you could collect some stats about this.

@andig
Copy link
Contributor Author

@andig andig commented May 8, 2021

Also, how often are long go:generate lines a necessity? I've written dozens of them in many projects before, and usually they're well under 100 characters. I can't say I've seen any generate directives that were long enough to be painful to read or maintain. Perhaps you could collect some stats about this.

That's a fair question. In my project I'm using them all over the place to generate code for implementing arbitrary optional interfaces

Perhaps you could collect some stats about this.

Any pointer how to?

@mvdan
Copy link
Member

@mvdan mvdan commented May 8, 2021

I think @rsc used to have a large corpus of Go code, but I don't think that's up to date and publicly available. You could try https://github.com/mvdan/corpus.

@andig
Copy link
Contributor Author

@andig andig commented May 9, 2021

Not scientific, but I've tried this on the mod cache of the top 1000 modules:

❯ awk '{print length}' generate.txt  | sort -g | hist -b 200 -s 30

 403|            o
 389|            o
 375|            o o
 361|           oo ooo
 347|           oo ooo
 333|     o     oooooo
 319|     o  o  oooooo
 306|     o  o  oooooo
 292|     ooooo oooooo
 278|     ooooo oooooo
 264|     ooooo oooooo
 250|     ooooo oooooo
 236|     ooooo oooooo   o
 222|     ooooo oooooo   o
 209|     oooooooooooo   oo
 195|     ooooooooooooo  oo
 181|     ooooooooooooo ooo
 167|     ooooooooooooo ooo oo
 153|     ooooooooooooo ooo oo    o
 139|     ooooooooooooooooo oo o  o   o   o
 125|     oooooooooooooooooooo o oo o o   o
 111|     oooooooooooooooooooooo oo o o  oo        o
  98|  o  oooooooooooooooooooooo oooooo  ooo       oo
  84|  oo oooooooooooooooooooooooooooooo oooo      oo
  70|  oo oooooooooooooooooooooooooooooo ooooooo ooooo
  56|  oo oooooooooooooooooooooooooooooooooooooo ooooo  o
  42|  ooooooooooooooooooooooooooooooooooooooooo ooooo  o  oo
  28|  ooooooooooooooooooooooooooooooooooooooooo oooooooooooo   o
  14|  oooooooooooooooooooooooooooooooooooooooooooooooooooooooooo oooo   ooo   o                               o
   1| ooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooo oo o oo o o oo oooo  ooooooooo         o o  oo               o  o  o        o         o      o                o
     ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------

----------------------------------
|            Summary             |
----------------------------------
|       observations: 8963       |
|      min value: 17.000000      |
|        mean : 80.812228        |
|     max value: 593.000000      |
----------------------------------
@bcmills
Copy link
Member

@bcmills bcmills commented May 10, 2021

If you need a really long go:generate command, note that it is always possible to wrap the actual binary you intend to run in an actual shell script, or even a .go source file to ensure portability to every platform that supports go run: https://play.golang.org/p/TFw7oBUzLfr

That adds its own overhead, of course, but if the long lines are really that much of a problem...

@bcmills
Copy link
Member

@bcmills bcmills commented May 10, 2021

Or, you could change the generator itself to parse its parameters from the file (identified via the GOFILE environment variable) instead of using command-line arguments.

(Actually, is the -p argument already redundant with the GOPACKAGE to begin with?)

//go:generate go run ../../cmd/tools/decorate.go 
//evcc:package vehicle
//evcc:function decorateVehicle
//evcc:base api.Vehicle
//evcc:file vehicle_decorators
//evcc:type api.ChargeState,Status,func() (api.ChargeStatus, error)
//evcc:type api.VehicleRange,Range,func() (int64, error)
@bcmills
Copy link
Member

@bcmills bcmills commented May 10, 2021

@ianlancetaylor ianlancetaylor added this to Incoming in Proposals May 11, 2021
@rsc
Copy link
Contributor

@rsc rsc commented May 12, 2021

The backslash version #46050 (comment) is better than the non-backslash version, but even then we will run into issues with continued strings and such. It will just lead to more and more weird corner cases. Given that the usual thing is "go:generate go run x.go" why not put all that complexity into x.go where you have a real parser? Or even "go:generate bash x.bash". It doesn't seem like we need multiline input as a feature here when it can be moved to a better place.

@rsc rsc moved this from Incoming to Active in Proposals May 12, 2021
@rsc
Copy link
Contributor

@rsc rsc commented May 12, 2021

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

@mvdan
Copy link
Member

@mvdan mvdan commented May 12, 2021

Given that the usual thing is "go:generate go run x.go" why not put all that complexity into x.go where you have a real parser? Or even "go:generate bash x.bash".

To play the devil's advocate for a moment: interpreters like bash might not be installed, and go run isn't particularly fast for large binaries as it needs to link every time. Though, arguably, that's something we could improve in go run itself.

Largely, I agree that the complexity should be put elsewhere, if it doesn't comfortably fit into a single line.

@andig
Copy link
Contributor Author

@andig andig commented May 12, 2021

Thank you for taking the time to comment so extensively.

Performance was never my concern, more readability. I appreciate the comments above, especially the use if the GOFILE and GOPACKAGE variables that I wasn‘t aware of. It seems that moving parsing into the action generator shouldn‘t be too complicated. I‘d prefer this approach over writing yet another stub that calls the actual generator.

Based on the options lined out I‘d be happy to withdraw this proposal.

@andig andig closed this May 12, 2021
@andig
Copy link
Contributor Author

@andig andig commented May 13, 2021

@rsc rsc moved this from Active to Declined in Proposals May 19, 2021
@rsc
Copy link
Contributor

@rsc rsc commented May 19, 2021

No change in consensus, so declined.
— rsc for the proposal review group

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Proposals
Declined
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
6 participants