Skip to content

Reject braced constructor field without value - #422

Open
makenowjust wants to merge 1 commit into
cloudspannerecosystem:mainfrom
makenowjust:MakeNowJust/fix-braced-constructor-field
Open

Reject braced constructor field without value#422
makenowjust wants to merge 1 commit into
cloudspannerecosystem:mainfrom
makenowjust:MakeNowJust/fix-braced-constructor-field

Conversation

@makenowjust

Copy link
Copy Markdown
Collaborator

Fixes #418.

parseBracedNewConstructorField switched on : and { but had no default case, so a field followed by anything else (e.g. NEW A00{A0}, where the field is followed by }) produced a BracedConstructorField with a nil Value — and err == nil. SQL() and the position accessors then dereferenced the nil value and panicked.

A braced constructor field requires a value, so the parser now reports it as a syntax error:

syntax error: :1:15: expected token: {, :, but: }
  1|  NEW foo { bar }
   |                ^

Error recovery wraps the expression in an ast.BadExpr as usual.

Tests

  • New testdata case testdata/input/expr/!bad_new_braced_constructor_field_without_value.sql (NEW foo { bar }) with its generated result, alongside the existing !bad_new_braced_constructor.sql.

@makenowjust

Copy link
Copy Markdown
Collaborator Author

@apstndb Could you review this?

@apstndb apstndb left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

1| NEW foo { bar }
| ^
syntax error: testdata/input/expr/!bad_new_braced_constructor_field_without_value.sql:1:15: expected token: <eof>, but: }
1| NEW foo { bar }

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This error looks strange. We want to avoid it.

}

--- SQL
NEW foo { bar

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Furthermore, this lacks the closing brace }. It seems actually a bug.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug: ParseExpr accepts NEW A00{A0} without error, but SQL() panics with nil pointer dereference

2 participants