don't include unnecessary duplicate types in function args about gofumpt

[twpayne]

👋 @SchumacherFM we finally meet online!

To be fair, if the code generator is incorrectly ignoring arguments in valid Go code, this sounds like a bug in the code generator and is unrelated to gofumpt.

[mvdan]

I agree with @twpayne that the generator should be fixed in any case, but we can add @SchumacherFM's surprise to the bucket of reasons why we probably shouldn't do this transformation by default (see my last comment).

[SchumacherFM]

Hey @twpayne to meet in person is boring, so I decided to stalk you also online ;-)

We will fix our code generator, even if you decided not to implement this feature.

[mvdan]

Sure, I agree. Want to send a PR? :)

[kovetskiy]

What will happen to such declarations?

func myLongFunctionNameThatDoesSomethingReallyImportant(
	one BigStruct,
	two BigStruct,
	three BigStruct,
) {

People often use such declarations for the following reasons:

1. It's more obvious what type one or two has
2. It's much easier to delete a variable, you just click line and then press shortcut to delete the whole line instead
   but in the following code you can't just drop line with three, you need to join it with two:

func myLongFunctionNameThatDoesSomethingReallyImportant(
	one,
	two,
	three BigStruct,
) {

[mvdan]

Good point. I think a heuristic here can be simple enough. That is, if two parameters have the exact same type and are on the same line, we should deduplicate the type.

Still happy to accept a patch, as this seems like a somewhat easy change (with a test). Otherwise I'll probably get to it soon.

[twpayne]

Thanks for considering this!

Sure, I agree. Want to send a PR? :)

I'm fully into a big refactoring of a large project right now, so won't have time to submit a PR in the short term. If this issue is still open once I'm done with that, I'll be happy to look at submitting a PR, but please don't wait for me.

[mvdan]

@twpayne fyi, the further changes I mentioned in your first PR are now done in master: 9ba260d

[twpayne]

Very neat! Thank you :)

[mvdan]

@seebs and @dominikh bring up a valid concern on Slack: sometimes the type separation can be a hint to the reader that two parameters don't belong together. For example, (min, max int) versus (n int, max int).

I personally don't use that in my code, nor have I seen it much in the wild, but I'm interested to see if it's common. That is, if many others would start being annoyed by gofumpt always joining parameter types (as long as they're on the same line).

[twpayne]

Personally I've had the opposite problem: I refactor some code and don't notice that I've duplicated types for related parameters.

Dave Cheney wrote a nice blog post, "Be wary of functions which take several parameters of the same type", which gives a good rationale as to why you should avoid something like (n int, max int) if you can.

I realize that this is a subjective issue, and there is no correct answer that will work all the time for everybody, but my personal opinion is that de-duplicating types is more often correct than the opposite, especially with the multiline protection in place.

[mvdan]

I agree, I tend to use different types altogether if two parameters are easily confused and that could lead to bugs. I think we're mainly worrying about lower-level cases where you don't want to introduce the extra complexity of defining your own parameter types, or where changing the func signature would be a breaking change.

[twpayne]

Another option would be to move this from gofumpt to a separate, optional linter to be plugged into golangci-lint. It can then be disabled on a case-by-case basis with a //nolint: comment.

[seebs]

I would definitely not want it happening to my code, but I probably wouldn't have been running gofumpt in the first place.

I think I've seen things like this show up in particular in a graphics context. Consider:

Draw(r, g, b, a float32, x, y float32)

In particular, note that if you wanted to refactor to use float64 coordinates, you could do it with a single edit to one type, rather than needing to alter the entire line.

[mvdan]

It can then be disabled on a case-by-case basis with a //nolint: comment.

I don't think we want to go that route, though. A linter that makes suggestions driven by magic comments and options would be a very different project from gofumpt. We want to stick to a design similar to gofmt wherever possible.

Having said all this, I do think we could make a distinction between:

• Formatting rules that the vast majority of Go developers can live with, and which generally conform to "idiomatic Go"
• Formatting rules which are more opinionated, and which might go too far for some developers

The former should be a default, while the latter could be opt-in via a flag similar to gofmt's -s, and it oculd include extra rules like this one. Such a flag could be enabled when trying to aggressively "prettify" generated or badly formatted Go code, or it could be run from time to time and selectively applied via git add -p or git diff.

[SchumacherFM]

FYI: I've tried out the new feature from @twpayne and the new formatting breaks our code base.

We have function signatures like

func (s *Service) GetX(w http.ResponseWriter, r *http.Request, token string, addrkey string) (model.Summary, error)

from these services a code generator creates Go RPC and Typescript code. If the new formatted signature looks like this

func (s *Service) GetX(w http.ResponseWriter, r *http.Request, token, addrkey string) (model.Summary, error)

then the code generator will ignore the second argument addrkey and that breaks everything of course. For now I've just reverted the gofumpt binary to the version a91da47 with which we can live until we fix that code generator ...
I also do not want any code comments which makes gofumpt to skip the formatting for a code block ;-)

[twpayne]

Hmm, maybe this de-duplication would be a better fit as an "opinionated" (and therefore disabled by default) style check in https://github.com/go-critic/go-critic. I can see the argument that it is too strict to be applied universally.

[mvdan]

I still think there's value in this being part of a formatting tool, because quickly and aggressively formatting a bad/generated piece of code shouldn't require installing and running a full linter. Of course, a linter can do static analysis and give extra suggestions, but those tend to be far more complex than simple formatting decisions.

[mvdan]

@twpayne this rule is now available behind the -extra flag; this should make the default behavior less controversial, while keeping the feature available for those cases where it can still be useful. If you have any further ideas or feedback, feel free to leave a comment or open a new issue.

[twpayne]

Thank you, the -extra flag is a very good compromise :)