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

go/printer: GenDecl should print valid syntax without a token.Pos #23003

Closed
willfaught opened this Issue Dec 5, 2017 · 12 comments

Comments

Projects
None yet
5 participants
@willfaught
Copy link
Contributor

willfaught commented Dec 5, 2017

This:

var myGenDecl = &ast.GenDecl{
	Doc: doc,
	Tok: token.TYPE,
	Specs: []ast.Spec{
		&ast.TypeSpec{
			Comment: comment,
			Name:    &ast.Ident{Name: name},
			Type:    expr,
		},
	},
}

prints with go/printer.Fprint like this:

var b = &bytes.Buffer{}

if err := printer.Fprint(b, token.NewFileSet(), myGenDecl); err != nil {
	panic(err)
}

as something like this:

type// Doc line 1
// Doc line 2
MyDeclaredType MyUnderlyingType

I suspect this is because GenDecl.TokPos is unset, because I have no position information for it. I just want to print arbitrary Go syntax using the ast and printer packages. The nodes aren't coming from the parser package.

Other node types have syntax that don't require position information and do print valid syntax, like the parentheses in ParenExpr. It should likewise be possible to do so for GenDecl without assigning TokPos a real value.

Since the above behavior happens for the zero value for TokPos, what about using something like -1 to trigger printing GenDecl.Tok after the doc, for valid Go syntax? -1 is a valid Pos, as it turns out, according to Pos.IsValid. Alternatively, it could check whether the Pos equals the Pos of the file it's in, since, I assume, that's always the package declaration, and never a GenDecl.

I realize this is likely to get shot down, but maybe it could at least inform Go 2 designs. Any help making printing GenDecl would be very much appreciated. If there's a workaround, I'd be interested in that, too.

@gopherbot gopherbot added this to the Proposal milestone Dec 5, 2017

@gopherbot gopherbot added the Proposal label Dec 5, 2017

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

ianlancetaylor commented Dec 5, 2017

@mvdan

This comment has been minimized.

Copy link
Member

mvdan commented Dec 5, 2017

Note that a redesign of go/ast is being written, so that would likely be the better answer in the long term rather than to modify how go/ast works. /cc @griesemer

@griesemer

This comment has been minimized.

Copy link
Contributor

griesemer commented Dec 5, 2017

This is working as intended, or at best unfortunate.

Work-around. The printer will put the parentheses there if you provide any valid position; just make one up. Comments are interspersed based on positions, so you could just increment a token.Pos for each node.

@griesemer griesemer closed this Dec 5, 2017

@griesemer griesemer removed this from the Proposal milestone Dec 5, 2017

@griesemer griesemer added Unfortunate and removed Proposal labels Dec 5, 2017

@griesemer

This comment has been minimized.

Copy link
Contributor

griesemer commented Dec 5, 2017

PS: The go/printer needs to have some signal as to whether parentheses were present or not; and that signal is whether the ast.GenDecl.Lparen is a valid position or not. If it is, no matter what it is, it will print the parentheses. That's the design as is, and we're not going to change it at this point.

Note that com/compile has started using a newer and improved syntax tree (cmd/compile/internal/syntax) and if anything, going forward we may want to make that one non-internal. But probably not for a while. (That's what @mvdan was referring to.)

@griesemer griesemer changed the title proposal: go/ast: GenDecl should print valid syntax without a token.Pos go/printer: GenDecl should print valid syntax without a token.Pos Dec 5, 2017

@willfaught

This comment has been minimized.

Copy link
Contributor

willfaught commented Dec 5, 2017

@griesemer Thanks for the workaround! I'll try that Pos trick with the Tok and Doc.

For what it's worth, I glanced at cmd/compile/internal/syntax this morning, and noticed that Field only has one name, unlike go/ast.Field, and the function type params is a slice of Field, which seems to mean the exact syntax is lost (e.g. whether it was x, y int or x int, y int). Having an AST that you can convert to/from text without information loss would be very useful.

@griesemer

This comment has been minimized.

Copy link
Contributor

griesemer commented Dec 5, 2017

@willfaught Apropos cmd/compile/internal/syntax: No, exact syntax is not lost. Please do not draw premature conclusions from the (as of yet meagerly documented) source. The syntax.printer knows how to re-create the original source exactly by looking at the parameter types (are they the same pointer or not). The point of this is that tools operating on the syntax tree don't need to be bothered with the intricacies of the actual syntax and we are able to present a simpler node structure. And it's easy for the printer (the only code where it matters) to reconstruct the original source.

@griesemer

This comment has been minimized.

Copy link
Contributor

griesemer commented Dec 5, 2017

In fact, the documentation is already there:

Field struct {
	Name *Name // nil means anonymous field/parameter (structs/parameters), or embedded interface (interfaces)
	Type Expr  // field names declared in a list share the same Type (identical pointers)
	node
}
@willfaught

This comment has been minimized.

Copy link
Contributor

willfaught commented Dec 5, 2017

Interesting. Sounds cool. Sorry for drawing a hasty conclusion. It seemed similar to go/types.Var, but I admittedly only glanced around. I'll take a much longer look at it.

@willfaught

This comment has been minimized.

Copy link
Contributor

willfaught commented Dec 6, 2017

For posterity, here's exactly what you have to do:

func GenDeclSyntax(g *ast.GenDecl) string {
	if g == nil {
		return ""
	}

	var b = &bytes.Buffer{}
	var f = token.NewFileSet()
	var n int
	var p token.Pos = 1

	if g.Doc != nil {
		for _, c := range g.Doc.List {
			c.Slash = p
			n += len(c.Text)
			p++
		}
	}

	f.AddFile("", 1, n)
	g.TokPos = p

	if err := printer.Fprint(b, f, g); err != nil {
		panic(err)
	}

	return strings.TrimRight(b.String(), "\n")
}

You have to add a file to the file set with a size large enough for the doc. The file name can be zero, but the file offset must be at least 1.

@griesemer

This comment has been minimized.

Copy link
Contributor

griesemer commented Dec 6, 2017

If it's just about getting the printer to print the '('s, you should be able to get away with much less. You simply need to provide a valid token.Pos.

@willfaught

This comment has been minimized.

Copy link
Contributor

willfaught commented Dec 6, 2017

The issue was the type token being printed before the doc, producing invalid Go syntax, as in the example in the original post:

type// Doc line 1
// Doc line 2
MyDeclaredType MyUnderlyingType

Sorry if that wasn't clear. I referenced the parenthesized expression as an example of an expression that prints as correct Go syntax without Poses for the parentheses, producing something like (subexpr) instead of ()subexpr. Maybe that's not a perfect example, if the doc in the general declaration is what makes the difference.

If I omit any of the steps in the GenDeclSyntax code, then I get the invalid Go syntax in the above example.

I actually prefer there being no parentheses, since there's only one spec.

@willfaught

This comment has been minimized.

Copy link
Contributor

willfaught commented Dec 6, 2017

By "invalid Go syntax", I just meant undesired syntax, as in it isn't the syntax you'd write by hand for an equivalent declaration, and the doc wouldn't show up for it in godoc (probably?).

@golang golang locked and limited conversation to collaborators Dec 6, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.