gofumpt: break long lines when it's appropriate to do so

I’ve merged the long-lived PR into master. Note that the feature is not enabled by default just yet, because I want to get some early feedback first.

If you would like to help:

go install mvdan.cc/gofumpt@master

Then run it with the special env var:

GOFUMPT_SPLIT_LONG_LINES=on gofumpt -l -w .

If you find a bug or a regression, or a case where a line was split when it clearly shouldn’t have been, please leave a comment here. This issue will remain open until the feature is enabled by default. Thank you!

I don’t think we will do 3; there is nothing wrong with a call like:

fmt.Printf("some %v format %v string etc etc...", arg1, arg2,
    arg3, arg4, arg5, arg6)

FWIW personally I think that looks ugly, and it’s easy to miss arguments; when I reformat code like that, I either choose all args on one line or all args on separate lines.

I’d be interested to see how the different approaches end up looking when run on real code.

The irony is that Effective Go is formatted with a consistent line length, presumably for readability reasons.

It’s not just about punch cards - typographers have been thinking about this stuff for the past 100 years or so.

Thanks for the interest! This is indeed part of my grand plan 😃

I’ve left this feature out of the initial implementation, simply because it’s one of the tricky ones. A number of decisions must be taken:

• Do we want this to be configurable? Following gofmt, I’d very much rather not add a flag.
• How do we measure line length? A number of characters, a number of bytes, the line width when printed on screen?
• If we break lines after a limit, what should that limit be? 80 is probably too aggressive. 100? 120?
• Do we also want to break long strings and long comments?

The closest existing tool is https://github.com/walle/lll, which counts runes (characters), and defaults to a maximum of 80. rustfmt seems to enforce a configurable limit, defaulting to 100: https://github.com/rust-lang/rustfmt/blob/master/Configurations.md#max_width

My current thinking is to start small and conservative. That is, use a simple large limit like 120 runes, and only add newlines in non-controversial places, like between expressions in an expression list (such as your example). If we’re happy with that, we can look at more complex cases, like splitting up strings and comments, or dealing with nested expressions.

I think counting runes is the only sane default we can use. Counting bytes is just far too basic. Measuring width via https://godoc.org/golang.org/x/text/width would work, but only for monospace fonts that happen to agree with that package. gofmt aligns comments, so it must also measure the length of lines, for which it uses https://golang.org/pkg/text/tabwriter/:

The Writer assumes that all Unicode code points have the same width; this may not be true in some fonts or if the string contains combining characters.

So I think it’s fair to follow gofmt here, for the sake of simplicity and consistency.

Also cc @rogpeppe, who has thought about breaking super long lines for a while.

I either choose all args on one line or all args on separate lines.

This sounds like a good guideline to me. Anything in between is ugly and harder to read.

Yes, please no options.

It is either in or not. Adding options is a whole new can of worms that Go does not need.

This is awesome @mvdan thank you 😄 ❤️ I’ll post this here so not to pollute the PR with discussions.

Is the way func definitions and calls are split final? If not, do you have an idea of how you’d like them to look like?

---

Currently

	if err := f(argument1, argument2, argument3, argument4, argument5, argument6, &someComplex{argument7, argument8, argument9}); err != nil {
		panic(err)
	}

becomes (1)

	if err := f(argument1, argument2, argument3, argument4, argument5, argument6, &someComplex{
		argument7, argument8, argument9}); err != nil {
		panic(err)
	}

Is this an acceptably readable result?

---

Possible alternatives would be I guess something like (2)

	if err := f(
		argument1, argument2, argument3, argument4, argument5, argument6,
		&someComplex{
			argument7, 
			argument8, 
			argument9,
		},
	); err != nil {
		panic(err)
	}

or (3)

	if err := f(
		argument1,
		argument2,
		argument3,
		argument4,
		argument5,
		argument6,
		&someComplex{
			argument7, 
			argument8, 
			argument9,
		},
	); err != nil {
		panic(err)
	}

My personal preference is the last one (3), but I understand not many people like that.

---

ps. Same thing with the func definitions, though I agree that returns should be kept together.

Kubernetes does this nicely for func definitions which I really like.

func NewCertificateController(
	name string,
	kubeClient clientset.Interface,
	csrInformer certificatesinformers.CertificateSigningRequestInformer,
	handler func(*certificates.CertificateSigningRequest) error,
) *CertificateController {

(source)

Sorry, but as I commented in another issue, it’s unlikely that gofumpt will ever have any options of its own - just like gofmt doesn’t have any formatting optins.

Another example, this time in rustfmt (Rust is like Go in that if conditions do not require parens):

    if isThisAReallyReallyReallyReallyReallyLongVariableName == 123 && doReallyReallyReallyLongNamedCheck() == true {
        return true;
    }

becomes:

    if isThisAReallyReallyReallyReallyReallyLongVariableName == 123
        && doReallyReallyReallyLongNamedCheck() == true
    {
        return true;
    }

From Effective Go: “Go has no line length limit. Don’t worry about overflowing a punched card. If a line feels too long, wrap it and indent with an extra tab.” https://golang.org/doc/effective_go.html#formatting

Just my two cents, I don’t think this should be part of gofumpt. Formatters should be there for when there is an obvious, idiomatic modification that can be made to source code, not for style preference.

Also not sure if this is the time or place but don’t understand why something like this would not be configurable, especially as part of something like golangci-lint. For example, if I love all the adjustments made by gofumpt except one (like this one), I have to stop using the tool entirely.

I agree with @geoah , 3 is my preference as well.

You’ve done exactly what your statement above claims to avoid: different projects with different styles.

That’s true in a way, but it’s impossible to avoid this separation. I can’t take over gofmt or attempt to replace it entirely. If anyone wants to experiment with a stricter gofmt (but backwards compatible, like @jokeyrhyme said), it has to be a separate project.

So I can’t get rid of the gofmt vs gofumpt separation. I can, however, prevent more dimensions to be added in the form of extra options.

in the case of line length, you’re going against the Go authors’ opinion of what idiomatic is

I already touched on this topic, but to repeat my thinking - their stance is generally that line length limits are silly, I agree. They do not say that line length does not matter. If that were the case, most idiomatic Go code wouldn’t typically fit within 80-120 columns or so. Significantly longer lines are definitely not considered idiomatic in general.

Again, there’s noone forcing you to use gofumpt. If gofmt is enough for you, that’s absolutely fine. I’m always looking for more input to fine-tune gofumpt to be as good as possible, but the lack of options and the backwards compatibility are pretty much non-negotiable. The moment this tool loses either, it would stop being useful as a companion to gofmt.’

@samlof assuming you’re talking about preventing prettier from joining lines, I don’t think you should worry about that, because gofumpt won’t join short lines into regular-sized ones. This issue is only about splitting very long lines as long as they are easy to split into more reasonable lines.

for now using https://github.com/segmentio/golines as an alternative.

I agree with @jokeyrhyme, basically.

@kylechadha I also used to think that line length shouldn’t matter. And I definitely agree that a hard limit on line length is a terrible idea. Somewhere in between, humans make a judgement call, and I’m trying to encode that into a heuristic. I think the current state of the PR is already a pretty good approximation, but I would need to finish it up and furiously test it on large amounts of “idiomatic” Go code.

As for the configurability - you quote the Go docs, so I should mention that one big design decision from gofmt is to not make formatting configurable. Once you do that, different projects will use different, incompatible styles. I definitely want to follow the same rule of thumb here, so this will not be configurable. If it’s annoying or incorrect, then we should fix or remove the feature entirely, not expect the user to turn it off.

If a line feels too long, wrap it and indent with an extra tab.

This is very subjective, and can be labor intensive to do consistently, and also a time-waster in code reviews, which is part of the value of an opinionated line-break tool

don’t understand why something like this would not be configurable

This is a delicate balancing act that the prettier folks have struggled with as well

Options tend to increase adoption, but also increase code complexity, so it’s sort of up to each community / author to decide whether adoption is more important than simplicity / reliability

Please remember that the first iteration of this feature needs to be very conservative. It seems to me like forcing arguments to all be on separate lines is exactly the opposite of conservative. Let’s go step by step.

If a conservative heuristic goes well and people seem to like it, we can push it further in the future. For example, by requiring that multi-line parameter and argument lists have each element in a separate line. We already enforce that rule for composite literals.

I guess I could be convinced that such a stricter format would be a good thing, but I don’t see it clearly right now, and I work with a lot of code that would be heavily modified by such a rule. Hence why I strongly prefer to leave it for a future incremental step.

https://github.com/golang/go/issues/11915 outlines more or less my concerns above. I see this project as a way to experiment with these features.

https://github.com/golang/go/issues/8146 is more interesting. It turns out that gofmt does break up long func lines, if they are “complex” enough:

const maxSize = 100
if headerSize+p.bodySize(b, maxSize) <= maxSize {

Looks like headerSize and bodySize are just approximations of the number of characters it will take to print each bit of code:

nodeSize determines the size of n in chars after formatting. The result is <= maxSize if the node fits on one line with at most maxSize chars and the formatted output doesn’t contain any control chars. Otherwise, the result is > maxSize.

So, in a way, gofmt already enforces certain line length limits. I think people are happy with 100 for top-level function declarations, since there’s zero indentation and there’s little reason to make them long. So this tells us that we definitely want at least 100 as a global limit.

in vscode it can be set in settings.json with:

  "go.toolsEnvVars": {
    "GOFUMPT_SPLIT_LONG_LINES": "on"
  },

In the past I’ve written Dart with dartfmt. I definitely miss the auto-line wrapping when writing Go. Especially the amount of noise, that not having auto-line wrapping, adds to MR discussions. Cheers for working on this!

Regarding wrapping long argument and collection lists, dartfmt’s approach is to wrap these differently depending on the presence of a trailing comma (been a while since I wrote any, so hopefully my memory is correct).

So this:

func foo(a string, b int, c float,) {
  ...
}

Becomes:

func foo(
  a string,
  b int,
  c float,
) {
  ...
}

And without the trailing comma:

func foo(reallyReallyReallyReallyReallyLongName string, otherReallyReallyReallyReallyReallyLongName int, yetAnotherreallyReallyReallyReallyReallyLongName float) {
  ...
}

Becomes:

func foo(reallyReallyReallyReallyReallyLongName string, otherReallyReallyReallyReallyReallyLongName int,
 yetAnotherreallyReallyReallyReallyReallyLongName float) {
  ...
}

Seems to work quite well in practise. Hopefully Go’s simpler syntax will make this easier than implementing “The Hardest Program I’ve Ever Written”. See: https://journal.stuffwithstuff.com/2015/09/08/the-hardest-program-ive-ever-written/

And: https://github.com/dart-lang/dart_style

Some formatted only with gofmt, some formatted with both gofmt / gofumpt.

I think there’s an effort to keep the output of gofumpt compatible with gofmt, that is: gofmt will take gofumpt-formatted code and not detect anything it needs to change

It’s in the README.md:

Enforce a stricter format than gofmt, while being backwards compatible. That is, gofumpt is happy with a subset of the formats that gofmt is happy with.

Anyone working on this?

Thanks all for the comments. This is definitely something I want to try; it’s just a matter of finding the time to properly write it and test it.

On the other hand, forcing this behavior by default could mean that gofumpt would be less of a drop-in replacement, if the line-breaking is too agressive. I personally never write lines over 100-120 characters long, but sometimes I do go over 80 when declaring functions or writing comments. I’d have to experiment with large amounts of Go code.

I think counting runes is the only sane default we can use. Counting bytes is just far too basic. Measuring width via https://godoc.org/golang.org/x/text/width would work, but only for monospace fonts that happen to agree with that package.

I also think gofumpt should make it clear what it’s optimizing the code’s presentation for: editors, diffs, just the $PAGER, etc. Counting graphemes would optimize for most graphical editors (and a few console-based ones); counting cell width would optimize for pretty diffs.

gofmt aligns comments, so it must also measure the length of lines, for which it uses https://golang.org/pkg/text/tabwriter/:

The Writer assumes that all Unicode code points have the same width; this may not be true in some fonts or if the string contains combining characters.

So I think it’s fair to follow gofmt here, for the sake of simplicity and consistency.

I agree that following upstream is the best approach for the project, so I suppose the scope could be clarified to “optimizing display for simple diffs or pagers, assuming all code points have equal width”. That being said, I’d like to see support for code points with different widths.

Some of the people who worked on https://github.com/psf/black have had lots of discussions on line-wrapping (stemming from a decision to break from PEP-8’s 79/80-col recommendation) and may be able to share thoughts.

If anyone else is looking for a decent solution, golines is an excellent option.

I’ve been trying to keep the env var on at all times, and I’ve been finding it’s a tiny bit too aggressive - especially on indented code. For example, it did this change:

                        typeInfo := info.TypeOf(x)
-                       if typeInfo != types.Typ[types.String] && typeInfo != types.Typ[types.UntypedString] {
+                       if typeInfo != types.Typ[types.String] &&
+                               typeInfo != types.Typ[types.UntypedString] {
                                return true
                        }

This definitely feels wrong. The indented line does reach to column 110 if we count tabs as 8 columns, but the non-indented line is just 86 characters, and we end up with two tiny lines of 41 and 44 non-indented characters.

The new format is not wrong per se, but I lean towards the old format, and I don’t think there’s anything wrong with it. I think it falls under “goes over the limit, but splitting the line isn’t better”.

I’ll probably tweak the heuristic to be a bit less aggressive on indented lines.

Yes, that definitely sounds like a bug in master. Please file a separate issue, and I’ll hide these comments since they’re off-topic for this feature/PR.

I put together a rough first cut of this feature in https://github.com/mvdan/gofumpt/pull/70. Please try it out and provide feedback.

To repeat the design choices here, at least for the initial implementation:

• It needs to be conservative. If any change seems annoying or makes code less readable, that’s a bug.
• It can’t be configurable, since we have no flags on top of plain gofmt.
• We can only insert newlines. Heavier refactors like splitting string literals are not allowed.

The heuristic isn’t documented, but you can get a rough idea of how it behaves by looking at the test cases added in the PR.

Looking forward to testing this feature!

Having that feature would be greatest impact on the go fmt ecosystem, to be honest, it is a lot of frustration to manually format Go code after working with Java for some time where code formatting is very powerful, for example: https://github.com/palantir/palantir-java-format https://github.com/google/google-java-format

different versions of the tool

Sorry, I don’t follow.

The plan is to try with a large limit like 120 first, which would apply to all files equally. Conservatively, it would likely ignore lines that can’t be split into shorter ones.

+1 for multiline function definitions and calls.

I understand that you don’t like options, I assume that includes different versions of the tool? Maybe something behind a build tag and people who really don’t want this can build the tool with something like an !lll build tag?