@griesemer

The documentation is clear - the position information is undefined, i.e., it can be whatever it happens to be. The implementation sets up a file set so that the rest of the code can work. It's not clear that the position information should be "cleared".

If this is important, it can always be done manually.

Leaving open for further comments, but I'm not sure that there's anything that needs to be done here.

The problem is that the incorrect Pos information tricks go/printer into writing source code that won't parse.

I have worked around this problem by copying the AST but excluding Pos values. I didn't see any other was to invalidate the incorrect Pos info. It was boring and tedious to write this and took the better part of a day.

In retrospect, it might have been simpler to use ParseExprFrom and include my FileSet. One might ask why PsrseExpr exists at all.

I understand that it would be difficult to change the current implementation of ParseExpr to ensure that the Pos values are invalid. I have a workaround. I just hope this issue doesn't waste anyone else's time.

I've seen wishful mention of some maybe eventual reimplementation of the AST library to simplify how comments are represented. Maybe during such a reimplementation, ParseExpr could be fixed as well. That a flaw is documented does not cause it to not be a flaw.

I won't presume to suggest how the go implementers should manage and prioritize their workloads. I hit a pitfall; it held me up for a while; I found a workaround. Feel free to close this ticket if you think that's the reasonable thing to do. You won't convince me that this is not a bug just because there's a warning in the documentation about it. I don't believe that the implementor of ParseExpr intentionally designed this behavior, but rather accepted it as a consequence of the rest of the parser it's built on. I do respect that there are probably many more important things to work on than this though

@MarkNahabedian ParsExpr exists for historic reasons - originally it was used in gofmt's rewrite implementation if I remember correctly (cmd/gofmt/rewrite.go:39) which happens not to care about position information (at least at that time). Later uses recognized that it was necessary to have control over position information, hence we added ParseExprFrom.

I'm sorry that you feel like you wasted time, but the parser interface is pretty small and documents both functions next to each other, so it does seem the documentation is readily available and also easy to find.

I am not trying to "convince you that this is not a bug" as we have found ourselves years ago (2015) in similar situations. But it is sufficient for other applications.

We do know that creating "valid" syntax trees from different AST parses is very cumbersome and fiddly. It may seem obvious in retrospect, but when we wrote the scanner/parser/printer etc. go/* libraries a decade ago, we wrote them primarily with a simple gofmt in mind, at a time when no other language was considering formatting all their source code by default automatically. We clearly didn't anticipate to which extent people would use them for programmatic code manipulation.

The compiler does use a brand-new and stand-alone package (cmd/compile/internal/syntax) which creates a syntax tree that has position information attached independent of some file set. That makes manipulating that tree much simpler. It also contains a printer that formats independent of position information, purely based on tree structure, exactly to avoid all these problems. The syntax tree doesn't track comments yet, though, and the printer doesn't do alignment a la gofmt. Eventually they should do it, and once we're sufficiently happy with the API, it might become an alternative choice to the go/* libraries (and move out from "internal"). In the meantime, you're free to copy from it.

Finally, there is a golang.org/x/tools/go/ast/astutil, a library that contains a rewrite mechanism (astutil.Apply) that might be helpful to do tree manipulations such as changing position information.

I'm going to close this as "unfortunate". We're not going to make changes to ParseExpr.

Ok. Thanks much for the explanation and historical context. It is extremely rare that someone is able to anticipate all the ways others might (ab)use their library.

I'll take a look at astutil.