cockroachdb / cockroachdb/cockroach
scbuild: astAnnotator doesn't duplicate the AST in the right way
- Dominant language
- Go
- Stars
- 32.5k
- Forks
- 4.1k
- PR merge metrics
- PR metrics pending
Description
**Describe the problem**
This code:
```go
func newAstAnnotator(original tree.Statement) (*astAnnotator, error) {
// Clone the original tree by re-parsing the input back into an AST. We need
// to keep tagged dollar quotes in case they're necessary to parse the
// original statement.
statement, err := parser.ParseOne(tree.AsStringWithFlags(original, tree.FmtTagDollarQuotes))
```
Has multiple problems:
1. we do not always have a guarantee that pretty-print the entire statement will return something that can be parsed.
- generally, we nurture this guarantee only for scalar expression (for dsql execution), but not as much for entire statements
- yes, we have some unit testing to get there, but the unit testing is often incomplete (e.g. #101978)
- a simple deep copy of the tree would achieve the necessary outcome without placing a correctness requirement on pretty-printing
2. it's expensive! The computation of pretty-printing is extremely hungry wrt memory allocations and puts a lot of pressure on the memory GC. A simple deep copy of the tree would be sufficient here.
3. arguably, it doesn't need to be performed upfront; we only need the copy the first time the AST is modified (i.e. lazy copy)
Jira issue: CRDB-27196
Epic CRDB-60948
Contributor guide
Assessment
This issue has not been assessed yet.