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

x/tools/imports: fill in missing type information in composite literals #9827

Open
Sajmani opened this issue Feb 10, 2015 · 11 comments

Comments

@Sajmani
Copy link
Contributor

commented Feb 10, 2015

Idea from campoy + discussions with crawshaw.

The spec now permits some types to be elided from composite literals: "Within a composite literal of array, slice, or map type T, elements that are themselves composite literals may elide the respective literal type if it is identical to the element type of T. Similarly, elements that are addresses of composite literals may elide the &T when the element type is *T."

Struct literals must still specify the types for each element, since without them the literals are difficult to understand. But these types are fully determined by the outermost type of the literal expression and the field names. So it should be possible to fill them in automatically. Since goimports is already processing the AST and adding missing imports, let's see whether it can also fill in these missing types.

For example:

goimports <<EOF
func main() {
  fmt.Println(image.Rectangle{Min: {X: 2}})
}
EOF
should output:
package main

import (
  "fmt"
  "image"
)

func main() {
  fmt.Println(image.Rectangle{Min: image.Point{X: 2}})
}
@neild

This comment has been minimized.

Copy link
Contributor

commented Feb 10, 2015

I know this has been discussed before, but I disagree with this premise: "literals must still specify the types for each element, since without them the literals are difficult to understand."

Literals can be difficult to understand without the types, but I don't believe there is any difference in comprehensibility between

rect := image.Rectangle{Min: image.Point{X: 2}}
and
rect.Min.X = 2

(Actually, there is a difference--the latter is clearly easier to read.)

We allow the latter; why not the former?

@crawshaw

This comment has been minimized.

Copy link
Contributor

commented Feb 10, 2015

@neild language design discussions happen on the golang-nuts mailing list, not the issue tracker. This is a goimports feature request.

@crawshaw crawshaw changed the title goimports: fill in missing type information in composite literals tools/imports: fill in missing type information in composite literals Feb 10, 2015

@bcmills

This comment has been minimized.

Copy link
Member

commented Feb 10, 2015

I think neild has the right idea, though. If this problem is worth solving, it seems much better addressed by making type-elision more uniform rather than having goimports add redundant information for non-imports.

@crawshaw

This comment has been minimized.

Copy link
Contributor

commented Feb 10, 2015

Again, this does not belong on the issue tracker. Please take this conversation to golang-nuts.

@bcmills

This comment has been minimized.

Copy link
Member

commented Feb 10, 2015

The question of whether the goimports feature is worth adding certainly does belong on the issue tracker.

@crawshaw

This comment has been minimized.

Copy link
Contributor

commented Feb 10, 2015

The language change has been discussed before and rejected because it makes code easier to write in return for making code harder to read. The goimports feature makes code easier to write, and maintains the current easier to read Go.

If you want to claim that the language should change, that should happen on golang-nuts. If you want to claim that goimports should not be modified because the language should be changed, that is a language change and should also be on golang-nuts. If you want to argue that the language should stay the same and this should not be part of goimports at all, that should be made here. It is however, not the argument you made.

@bcmills

This comment has been minimized.

Copy link
Member

commented Feb 10, 2015

The argument I am making is:
If the underlying problem is worth addressing, then goimports is not the right place to address it.
I am specifically avoiding the question of whether the underlying problem is worth addressing.

Goimports transforms ambiguous programs into unambiguous programs by guessing at the intended imports. That task must be performed under the supervision of the package author, because only they are in a position to resolve the ambiguity. But there is nothing ambiguous about a composite literal - either it is well-formed or it is not.

@crawshaw

This comment has been minimized.

Copy link
Contributor

commented Feb 10, 2015

I want this on my save hook, where goimports is right now. Unless Brad disagrees, I don't see why another tool is necessary.

@bradfitz

This comment has been minimized.

Copy link
Member

commented Feb 10, 2015

goimports is a tool to turn almost-valid programs into more-valid programs, so I think this is in scope. See also: https://github.com/sqs/goreturns

I actually had a conversation with @sqs about whether we should make goimports pluggable so people could add their own passes. But then do we link them all in? And then does it have a config file so people can enable/disable certain passes? Or is the bar only that non-annoying/usually-mostly-correct ones go in?

This is what I wanted to discuss today in our weekly meeting, but we didn't get to it in time.

I don't know where the line is, but I think this composite literal-fleshing-out thing isn't crossing the line.

I agree with @crawshaw that this is the best of both worlds: it lets the author write a program where they know what they meant, but it transforms it into one that's more readable (with the types included), which is why in the Go 1 release meeting when @rsc had prototyped more type elision, we looked at before & after code samples for a bunch of composite literals and pretty much everybody agreed that the version including more types was more readable, which is why the language requires you to write them in all but some of the most obvious cases. But if the programmer knows what they're doing, a program can make it more explicit.

@mikioh mikioh added the repo-tools label Feb 10, 2015

@Sajmani

This comment has been minimized.

Copy link
Contributor Author

commented Feb 11, 2015

Paraphrasing from a discussion with @rsc --

  • extending goimports beyond imports slippery slope towards fixing all kinds of issues
  • for example, Russ considered writing tool that attempts to fix all the type errors in a program by inserting conversions
  • this essentially converts a program in some non-Go language to Go
  • if we do anything, it should be a separate tool. gosugar, perhaps.

@robpike has similar concerns.

@adg

This comment has been minimized.

Copy link
Contributor

commented Feb 11, 2015

extending goimports beyond imports slippery slope towards fixing all kinds of issues

FWIW, we have a great track record of maintaining traction on slippery slopes.

@mikioh mikioh changed the title tools/imports: fill in missing type information in composite literals imports: fill in missing type information in composite literals Feb 15, 2015

@rsc rsc added this to the Unplanned milestone Apr 10, 2015

@rsc rsc changed the title imports: fill in missing type information in composite literals x/tools/imports: fill in missing type information in composite literals Apr 14, 2015

@rsc rsc modified the milestones: Unreleased, Unplanned Apr 14, 2015

@rsc rsc removed the repo-tools label Apr 14, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.