google / google/googlesql

Fix incomplete-type errors when building GoogleSQL with C++23

Open
#178 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
C++
Stars
2.6k
Forks
260
PR merge metrics
No merged PRs in 30d

Description

Please incorporate the incomplete-type ownership fixes proposed in [PR #175](https://github.com/google/googlesql/pull/175) so GoogleSQL can be built by C++23 consumers without carrying a downstream source patch. This requests compatibility with a consumer-selected C++23 toolchain, not a change to GoogleSQL's C++20 default.

## Why this is needed

The Cloud Spanner emulator encountered C++23 compilation failures where `std::unique_ptr` destruction or assignment is instantiated before the owned type is complete. [cloud-spanner-emulator#375](https://github.com/GoogleCloudPlatform/cloud-spanner-emulator/pull/375) carries the GoogleSQL fixes as a downstream patch. Incorporating the fixes would let the emulator drop its downstream patch after updating to a GoogleSQL release containing them, and let other consumers use the same fixes when adopting C++23.

The affected definitions remain in GoogleSQL master `289c4bc6cf6e134129a032ccb2e4f849e000279c`:

- Generated AST constructors, node-owning mutators, and interleaved factory functions can require node types defined later in the generated header.
- `DotStarSourceExprInfo::lateral_reference_state` has a redundant `= nullptr` initializer while `LateralReferenceState` is forward-declared.
- `RewriterVisitor`'s constructor precedes `WithEntryRewriteState`, and `SQLBuilder`'s constructor/destructor precede `SQLBuilder::AnalyticFunctionInfo`.
- Reference-implementation argument classes own `ValueExpr` and `RelationalOp` before those classes are defined.

## Requested change

Move the operations that require complete types after the corresponding type definitions, and generate AST constructors and node-owning mutators in `resolved_ast.cc`. Preserve ownership, initialization, public signatures, inline builder methods, and the C++20 default. The linked PR applies the existing emulator patch directly to the seven GoogleSQL source/template files.

## Validation

Regenerated all 12 C++ AST outputs before and after the change. The builder and visitor/node-kind outputs are byte-identical. Checked matching declarations and unchanged bodies for all moved generated constructors and mutators, and verified that factories follow all node definitions. C++ compilation and runtime tests were not run.

I understand GoogleSQL does not accept external code contributions; the PR is a reference implementation for maintainers to incorporate through the internal development process.

Contributor guide

Open the contributing guide

Research direction

Start by reviewing PR #175 and the affected generated AST definitions, forward-declared ownership sites, RewriterVisitor, SQLBuilder, and reference-implementation argument classes. Regenerate all 12 C++ AST outputs and compare the results against the stated unchanged builder and visitor/node-kind outputs. Done means the seven source/template files incorporate the fixes while preserving ownership, initialization, public signatures, inline builder methods, and the C++20 default.

Written by the indexing model from the issue text.

Assessment

Tech stack
cpp
Domain
build-system, compilers
Issue type
Bug
Difficulty
5/5
Estimated time
Over a week
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.