Invalid name generation when using python reserved keywords #89

QuentinN42 · 2025-07-25T16:51:00Z

Fixes #88

To fix this issue, here is my proposition:

 class Author(pydantic.BaseModel):
+     model_config = pydantic.ConfigDict(
+        validate_by_alias=True,
+        validate_by_name=True,
+    )
     id: int
+    class_: str = pydantic.Field(
+        alias="class",
+    )
-    class: str

I'm replacing the class reserved keyword by class_.
I add validate_by_name to allow us to define the class using Author(class_=...).
I add validate_by_alias to allow JSON serde.

Changes overview

I've created a new file : internal/poet/reserved.go to append _ to reserved names.
I've modified internal/gen.go::pydanticNode to add model_config = pydantic.ConfigDict
I've modified internal/gen.go::fieldNode:

If not reserved keyword, code unchanged.
Else if using pydantic, add the _ and add pydantic.Field(...)
Else if dataclasses, only add the _

Added tests

Questions / possible issues

To run the tests, I just clonned the repo and ran make test is it correct ?
I had to update all comment from sqlc v1.28.0 to sqlc v1.29.0 is this intended ?
The wasm sha256 changed, should I commit that ?
In internal/poet/reserved.go I hardcoded reservedKeywords we should double check it and check if we want to keep it like that.
Do you want me to add more tests ?

…n reserved keywords Signed-off-by: QuentinN42 <quentin@lieumont.fr>

Signed-off-by: QuentinN42 <quentin@lieumont.fr>

…figDict Signed-off-by: QuentinN42 <quentin@lieumont.fr>

Signed-off-by: QuentinN42 <quentin@lieumont.fr>

Copilot

Pull request overview

This PR addresses the handling of Python reserved keywords in generated code by transforming field names that conflict with keywords (e.g., class → class_) and adding appropriate Pydantic configuration for serialization/deserialization.

Key Changes:

Introduced reserved keyword detection and field name transformation logic (internal/poet/reserved.go)
Modified Pydantic models to include model_config with validate_by_alias=True and validate_by_name=True
Updated field generation to handle reserved keywords differently for Pydantic (with alias) vs dataclasses (simple rename)

Reviewed changes

Copilot reviewed 50 out of 52 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
internal/poet/reserved.go	New file with reserved keyword list and transformation functions
internal/poet/reserved_test.go	Tests for reserved keyword detection and transformation
internal/poet/builders.go	Modified `Name()` builder to apply field name transformation
internal/gen.go	Updated `pydanticNode()` to add model_config and `fieldNode()` to handle reserved keywords
internal/gen_test.go	New unit tests for field node generation with reserved keywords
internal/endtoend/endtoend_test.go	Modified error message output in test
internal/endtoend/testdata/emit_pydantic_models_with_reserved_keywords/*	New end-to-end test for Pydantic with reserved keywords
internal/endtoend/testdata/dataclasses_with_reserved_keywords/*	New end-to-end test for dataclasses with reserved keywords
internal/endtoend/testdata/emit_pydantic_models/db/models.py	Updated to include model_config in all Pydantic models
internal/endtoend/testdata/*/sqlc.yaml	Updated WASM SHA256 checksums
internal/endtoend/testdata//python/.py	Updated version comments from v1.28.0 to v1.29.0
examples/src//.py	Updated version comments from v1.28.0 to v1.29.0

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-11-24T16:21:58Z

internal/gen.go

+		Body: poet.Nodes(
+			&pyast.Assign{
+				Targets: []*pyast.Node{
+					{
+						Node: &pyast.Node_Name{
+							Name: &pyast.Name{Id: "model_config"},
+						},
+					},
+				},
+				Value: poet.Node(
+					&pyast.Call{
+						Func: &pyast.Node{
+							Node: &pyast.Node_Attribute{
+								Attribute: &pyast.Attribute{
+									Value: &pyast.Node{
+										Node: &pyast.Node_Name{
+											Name: &pyast.Name{
+												Id: "pydantic",
+											},
+										},
+									},
+									Attr: "ConfigDict",
+								},
+							},
+						},
+						Keywords: []*pyast.Keyword{
+							{
+								Arg:   "validate_by_alias",
+								Value: poet.Name("True"),
+							},
+							{
+								Arg:   "validate_by_name",
+								Value: poet.Name("True"),
+							},
+						},
+					},
+				),
+			},
+		),


The model_config with validate_by_alias=True and validate_by_name=True is being added to ALL Pydantic models, even those without reserved keywords. This adds unnecessary validation overhead for models that don't need it. Consider only adding this configuration when the model actually contains fields with reserved keyword names, or at minimum make it conditional based on whether any field in the model uses reserved keywords.

Copilot · 2025-11-24T16:21:59Z

internal/poet/builders.go

 	return &ast.Node{
 		Node: &ast.Node_Name{
-			Name: &ast.Name{Id: id},
+			Name: &ast.Name{Id: FieldName(id)},


Critical bug: Applying FieldName(id) to ALL uses of poet.Name() will break code generation. The Name function is used for many purposes beyond field names, including:

Built-in constants like "True", "False", "None" (lines 629, 633 in gen.go)

Module names like "dataclasses", "enum" (lines 576, 782, 787)

Type names like "str", "int" (lines 781, 1052, 1145)

Variable names like "self", "conn", "row", "result" (lines 866, 868, 1007, 1031, etc.)

Once the reserved keywords list includes "True", "False", "None", "for", "return", "yield", "is", "not", "in", "or", "and", "as", "with", "if", "else", etc., this will transform all these identifiers incorrectly. This change should be reverted, and FieldName() should only be applied in specific contexts where field names are being generated.

Copilot · 2025-11-24T16:21:59Z

internal/poet/reserved_test.go

+	}
+}
+
+func Test_FieldName_ShouldUdateReservedKeywords(t *testing.T) {


Typo in function name: "ShouldUdateReservedKeywords" should be "ShouldUpdateReservedKeywords"

Copilot · 2025-11-24T16:21:59Z

internal/poet/reserved.go

+var reservedKeywords = []string{
+	"class",
+	"if",
+	"else",
+	"elif",
+	"not",
+	"for",
+	"and",
+	"in",
+	"is",
+	"or",
+	"with",
+	"as",
+	"assert",
+	"break",
+	"except",
+	"finally",
+	"try",
+	"raise",
+	"return",
+	"yield",
+}


The reserved keywords list is incomplete. Python 3.x has 35 keywords, but only 17 are included here. Missing keywords include: False, None, True, async, await, continue, def, del, from, global, import, lambda, nonlocal, pass, while, and match/case (Python 3.10+). This could cause syntax errors when database columns use these names.

Copilot · 2025-11-24T16:22:00Z

internal/endtoend/endtoend_test.go

 			got, err := cmd.CombinedOutput()
 			if diff := cmp.Diff(string(want), string(got)); diff != "" {
-				t.Errorf("sqlc diff mismatch (-want +got):\n%s", diff)
+				t.Errorf("sqlc diff mismatch (-want +got):\n%s", got)


This change breaks the test logic. The error message should show diff (the actual difference between want and got) to help debug test failures, not got (the raw output). This makes it harder to understand what went wrong when tests fail.

Suggested change

t.Errorf("sqlc diff mismatch (-want +got):\n%s", got)

t.Errorf("sqlc diff mismatch (-want +got):\n%s", diff)

QuentinN42 added 5 commits July 25, 2025 15:37

feat: added invalid test for invalid name generation when using pytho…

19e8d56

…n reserved keywords Signed-off-by: QuentinN42 <quentin@lieumont.fr>

fix: added reserved python keywords

f928244

Signed-off-by: QuentinN42 <quentin@lieumont.fr>

refactor: use poet.Node

24fcd39

Signed-off-by: QuentinN42 <quentin@lieumont.fr>

test: added more tests to the poet pkg and generated the pydantic.Con…

a5d1ee8

…figDict Signed-off-by: QuentinN42 <quentin@lieumont.fr>

feat: added dataclasses tests

068fd25

Signed-off-by: QuentinN42 <quentin@lieumont.fr>

devstein requested review from Copilot, devstein and kyleconroy and removed request for kyleconroy November 24, 2025 16:17

Copilot started reviewing on behalf of devstein November 24, 2025 16:18 View session

Copilot finished reviewing on behalf of devstein November 24, 2025 16:20

Copilot AI reviewed Nov 24, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Invalid name generation when using python reserved keywords #89

Invalid name generation when using python reserved keywords #89

Uh oh!

QuentinN42 commented Jul 25, 2025

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Nov 24, 2025

Uh oh!

Copilot AI Nov 24, 2025

Uh oh!

Copilot AI Nov 24, 2025

Uh oh!

Copilot AI Nov 24, 2025

Uh oh!

Copilot AI Nov 24, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

	t.Errorf("sqlc diff mismatch (-want +got):\n%s", got)
	t.Errorf("sqlc diff mismatch (-want +got):\n%s", diff)

Invalid name generation when using python reserved keywords #89

Are you sure you want to change the base?

Invalid name generation when using python reserved keywords #89

Uh oh!

Conversation

QuentinN42 commented Jul 25, 2025

Changes overview

Questions / possible issues

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Key Changes:

Reviewed changes

Uh oh!

Copilot AI Nov 24, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 24, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 24, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 24, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 24, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant