compiler: fix what the star dedupe claims, and prove it is needed

Review of the previous commit turned up three things.

The dedupe in expandCore was justified by a CTE analyzed twice, which
does not happen — a CTE is analyzed once and cached. Instrumenting it to
panic on a duplicate showed the corpus never hits it at all. The real
source is an expression typed a second time when a later clause refers to
the output name it was given: "SELECT (SELECT * FROM baz LIMIT 1) AS x
FROM foo GROUP BY x" reports the star in the subquery once for GROUP BY
and once for the target list, and dropping the dedupe turns that query
into an overlapping edit. The comment now says so and the query is part
of the star_expansion_core case on every engine, where it generates what
the legacy path generates.

expandCore returned an error it never produced, so it returns only edits.

newParser was set on the core path alone, leaving a nil func field on a
Compiler built the other way. Nothing calls it there today, but a field
that is only sometimes valid is one to get wrong later, so the legacy
path sets it too and takes its own parser from it.

Author: claude

internal/compiler/engine.go

@@ -38,10 +38,11 @@ type Compiler struct {
 	coreAnalysis bool
 	coreDialect  core.Option
 
-	// newParser builds a parser for the configured engine. The core path
-	// analyzes statements concurrently, and a parser holds enough state that
-	// two goroutines cannot share one, so a goroutine that has to parse
-	// something of its own — the query text it just rewrote — builds its own.
+	// newParser builds a parser for the configured engine, and is set for
+	// every engine either path supports. The core path analyzes statements
+	// concurrently, and a parser holds enough state that two goroutines cannot
+	// share one, so a goroutine that has to parse something of its own — the
+	// query text it just rewrote — builds its own rather than taking c.parser.
 	newParser func() Parser
 
 	schema []string
@@ -82,7 +83,7 @@ func NewCompiler(conf config.SQL, combo config.CombinedSettings, parserOpts opts
 
 	switch conf.Engine {
 	case config.EngineSQLite:
-		c.parser = sqlite.NewParser()
+		c.newParser = func() Parser { return sqlite.NewParser() }
 		c.catalog = sqlite.NewCatalog()
 		c.selector = newSQLiteSelector()
 
@@ -96,11 +97,11 @@ func NewCompiler(conf config.SQL, combo config.CombinedSettings, parserOpts opts
 			}
 		}
 	case config.EngineMySQL:
-		c.parser = dolphin.NewParser()
+		c.newParser = func() Parser { return dolphin.NewParser() }
 		c.catalog = dolphin.NewCatalog()
 		c.selector = newDefaultSelector()
 	case config.EnginePostgreSQL:
-		c.parser = postgresql.NewParser()
+		c.newParser = func() Parser { return postgresql.NewParser() }
 		c.catalog = postgresql.NewCatalog()
 		c.selector = newDefaultSelector()
 
@@ -116,6 +117,7 @@ func NewCompiler(conf config.SQL, combo config.CombinedSettings, parserOpts opts
 	default:
 		return nil, fmt.Errorf("unknown engine: %s", conf.Engine)
 	}
+	c.parser = c.newParser()
 	return c, nil
 }
 

internal/compiler/expand_core.go

@@ -13,16 +13,18 @@ import (
 // its subqueries, so there is nothing left to look up here: what remains is
 // deciding how each name is written, which is the engine's business and not
 // the core's.
-func (c *Compiler) expandCore(raw *ast.RawStmt, stars []core.StarExpansion) ([]source.Edit, error) {
+func (c *Compiler) expandCore(raw *ast.RawStmt, stars []core.StarExpansion) []source.Edit {
 	if len(stars) == 0 {
-		return nil, nil
+		return nil
 	}
 	edits := make([]source.Edit, 0, len(stars))
 	seen := make(map[int]bool, len(stars))
 	for _, star := range stars {
-		// A statement analyzed more than once — the same CTE referenced twice,
-		// say — reports its stars once per pass. Editing one twice would
-		// overlap, so only the first is kept.
+		// The analyzer types an expression again when a later clause refers to
+		// the output name it was given, so "SELECT (SELECT * FROM t) AS x ...
+		// GROUP BY x" reports the star in the subquery once per pass. The
+		// passes agree on what the star covers, and editing the same reference
+		// twice would overlap, so only the first is kept.
 		if seen[star.Location] {
 			continue
 		}
@@ -70,5 +72,5 @@ func (c *Compiler) expandCore(raw *ast.RawStmt, stars []core.StarExpansion) ([]s
 			New:      strings.Join(cols, ", "),
 		})
 	}
-	return edits, nil
+	return edits
 }

internal/compiler/parse_core.go

@@ -65,11 +65,7 @@ func (c *Compiler) parseQueryCore(raw *ast.RawStmt, src string, pre *preprocess.
 		for _, p := range res.Parameters {
 			params = append(params, Parameter{Number: p.Number, Column: coreParamColumn(p, namedParams)})
 		}
-		edits, err := c.expandCore(raw, res.Stars)
-		if err != nil {
-			return nil, err
-		}
-		expanded, err = source.Mutate(rawSQL, edits)
+		expanded, err = source.Mutate(rawSQL, c.expandCore(raw, res.Stars))
 		if err != nil {
 			return nil, err
 		}

internal/endtoend/testdata/star_expansion_core/mysql/go/models.go

@@ -12,3 +12,6 @@ SELECT * FROM (SELECT * FROM bar) sub;
 
 -- name: StarExpansionCTE :many
 WITH t AS (SELECT * FROM bar) SELECT * FROM t;
+
+-- name: StarExpansionAliasedSubquery :many
+SELECT (SELECT * FROM baz LIMIT 1) AS x FROM foo GROUP BY x;

internal/endtoend/testdata/star_expansion_core/mysql/go/query.sql.go

@@ -1,2 +1,3 @@
 CREATE TABLE foo (a text, b text, `group` text);
 CREATE TABLE bar (a text, c text);
+CREATE TABLE baz (z text);

internal/endtoend/testdata/star_expansion_core/mysql/query.sql

@@ -15,3 +15,6 @@ WITH t AS (SELECT * FROM bar) SELECT * FROM t;
 
 -- name: StarExpansionReturning :one
 INSERT INTO bar (a, c) VALUES ($1, $2) RETURNING *;
+
+-- name: StarExpansionAliasedSubquery :many
+SELECT (SELECT * FROM baz LIMIT 1) AS x FROM foo GROUP BY x;