Make substitutions be ast based not string based

[ncw]

In parseTemplateAndArgs we use parse the incoming template arguments using the ast. This checks them for syntax errors etc, but we then store those as a string with format.Node(&buf, token.NewFileSet(), arg).

// Parse the arguments string Template(A, B, C)
func parseTemplateAndArgs(s string) (name string, args []string) {
    expr, err := parser.ParseExpr(s)
    if err != nil {
        fatalf("Failed to parse %q: %v", s, err)
    }
    debugf("expr = %#v\n", expr)
    callExpr, ok := expr.(*ast.CallExpr)
    if !ok {
        fatalf("Failed to parse %q: expecting Identifier(...)", s)
    }
    debugf("fun = %#v", callExpr.Fun)
    fn, ok := callExpr.Fun.(*ast.Ident)
    if !ok {
        fatalf("Failed to parse %q: expecting Identifier(...)", s)
    }
    name = fn.Name
    for i, arg := range callExpr.Args {
        var buf bytes.Buffer
        debugf("arg[%d] = %#v", i, arg)
        format.Node(&buf, token.NewFileSet(), arg)
        s := buf.String()
        debugf("parsed = %q", s)
        args = append(args, s)
    }
    return
}

It would be better to store the ast at this point and substitute the ast rather than the name in replaceIdentifier. This would remove the ugliness of replacing things that aren't identifiers with a string, and then we would no longer need this code.

    // gofmt one last time to sort out messy identifier substution
    fset, f = parseFile(outputFileName)
    outputFile(fset, f, outputFileName)

[Logiraptor]

I've got this very close to finished, except for one part. See here:

// Replace the identifers in f
func replaceIdentifier(f *ast.File, old, new string) {
    // Inspect the AST and print all identifiers and literals.
    ast.Inspect(f, func(n ast.Node) bool {
        switch x := n.(type) {
        case *ast.Ident:
            // We replace the identifier name
            // which is a bit untidy if we weren't
            // replacing with an identifier
            if x.Name == old {
                x.Name = new
            }
        }
        return true
    })
}

This function is what has previously been doing all of the replacement work for the whole system. When using strings, it's ok to stick map[string]int into x.Name since it won't be looked at until printing. When using the ast as intended, however, I'd need to replace the *ast.Ident value itself with an ast.Expr. I see two ways of going about this.

The first, and more obvious way is to use a very large type switch in ast.Inspect which will look at every node's children and replace where necessary.

The second would be inspired by the gofmt source here. I think it'd be significantly shorter, but harder to reason about.

Personally I'm partial to the first option, but I'd like to get your opinion on this before I start since it's a large piece of code either way, and make sure I'm not missing an easy way to do this operation in general.

Thanks,
Patrick

[ncw]

The problem we have is that you need the parent of the *ast.Ident so you can update it. Using ast.Ident doesn't give you the parent of the node, and neither does ast.Walk.

So the choices are either re-implementing ast.Walk keeping track of the parent and hence the long switch statement, or doing something a bit more clever (like gorun).

The important question here is what is the bigger maintenance burden? If the go ast changes in the future which will need more work done on it? I think in that case the big switch approach loses because it is quite tightly bound to the workings of the ast - any change at all to the ast internals will require changes.

I'd therefore go with the gofmt approach. gofmt is trying to do essentially what we are trying to do in the rewriteFile function. If we were to lift that, then we would need apply and set. We wouldn't need match as we only need to match *ast.Ident not anything else. We could probably simplify the code too as we don't need to be so polite with invalid code - gotemplate can return a fatal error when it finds any.

There is a possible third approach to which would use Walk, but you'd keep track of the parent node by looking at the stack level - eg by using runtime.Callers. When the stack level increases you'd know that the previous item was the parent. I don't really recommend this, but it is possible!

It is OK to copy the code from gofmt - I've already re-used quite a bit of code from go internals and it is attributed in the README. I'd copy the file from gofmt into a separate file probably and delete all the unused functions leaving the copyright attribution at the top, then put an extra comment saying what you did to the original source. Put any new functions in template.go rather than in there.

Good luck!

[ncw]

When you've got something to share, push it into a branch here and we can collaborate on the final touches. Thanks! Nick

[Logiraptor]

I've pushed my work to the ast-arguments branch. The tests are currently failing. I've added two new files:

• replace.go contains the source copied from gofmt
• replace_test.go contains tests for just the replacement code. I figure it's complex enough to warrant it's own set of tests. There is only one test in there now, but we'll need more.

If you run just the Replace tests, you'll see some weird behaviour that I haven't yet figured out.

$go test -run Replace
--- FAIL: TestReplacements (0.00s)
    replace_test.go:121: Test[0] basic test
    replace_test.go:80: Output is wrong
        Got
        -------------
        package tt

        func Add(a, b int,

        ) int {
            var sum int = a + b
            return sum
        }

        -------------
        Expected
        -------------
        package tt

        func Add(a, b int) int {
            var sum int = a + b
            return sum
        }

        -------------
    replace_test.go:112: Diff
        ----
        --- /tmp/gotemplate_test977224189/src/out.go.actual 2014-12-24 15:13:41.349298847 -0600
        +++ /tmp/gotemplate_test977224189/src/out.go    2014-12-24 15:13:41.349298847 -0600
        @@ -1,8 +1,6 @@
         package tt

        -func Add(a, b int,
        -
        -) int {
        +func Add(a, b int) int {
            var sum int = a + b
            return sum
         }
FAIL
exit status 1
FAIL    github.com/ncw/gotemplate   0.005s

Notice it's printing

func Add(a, b int) int {...}

as

func Add(a, b int,

) int {...}

I'll keep looking into it. Let me know if you notice what's wrong.

[ncw]

I've pushed a number of changes to the branch (suggest you git pull --rebase your work if any not committed). I've changed the identifiers back into strings from *ast.Ident which simplifies the code a lot. I thought there might be a problem with the mappings map with *ast.Ident in it, but appently not!

Putting better error handling in the replace fields has thrown up that some of the replacements are invalid. Possibly some of that change might need to be reverted!

I'm sending this out now, so you can take a look - I've got to go and do other stuff right now!

I think the problem might be to do with the line numbering - the fact that we aren't keeping it in the replaced items means that gofmt then things it needs a new line, hence the extra comma and new line. Not sure what to do about that yet!

[ncw]

I've managed to fix the tests - if you look through the commits you'll see how. I had to import subst from gofmt to copy the nodes and rewrite all the line numbers.

Can you review it and fix stuff as you see appropriate.

Thanks

Nick