Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce BadNode and MultiError #230

Merged
merged 11 commits into from
Dec 21, 2024
Merged

Introduce BadNode and MultiError #230

merged 11 commits into from
Dec 21, 2024

Conversation

makenowjust
Copy link
Collaborator

@makenowjust makenowjust commented Dec 18, 2024

This PR introduces BadNode and MultiError.

  • BadNode is a placeholder node for a source fragment containing syntax errors.
  • MultiError is an error consisting of a list of *memefish.Error.
  • Now, the parser can report multiple errors and return an AST node even if errors are reported.

Example:

$ go run ./tools/parse/main.go 'select (1 +) * (% 2)'
--- Error
syntax error: :1:12: unexpected token: )
  1|  select (1 +) * (% 2)
   |             ^
syntax error: :1:17: unexpected token: %
  1|  select (1 +) * (% 2)
   |                  ^

--- AST
&ast.QueryStatement{
  Query: &ast.Select{
    Results: []ast.SelectItem{
      &ast.ExprSelectItem{
        Expr: &ast.BinaryExpr{
          Op:   "*",
          Left: &ast.ParenExpr{
            Lparen: 7,
            Rparen: 11,
            Expr:   &ast.BadExpr{
              BadNode: &ast.BadNode{
                NodePos: 8,
                NodeEnd: 11,
                Tokens:  []*token.Token{
                  &token.Token{
                    Kind: "<int>",
                    Raw:  "1",
                    Base: 10,
                    Pos:  8,
                    End:  9,
                  },
                  &token.Token{
                    Kind:  "+",
                    Space: " ",
                    Raw:   "+",
                    Pos:   10,
                    End:   11,
                  },
                },
              },
            },
          },
          Right: &ast.ParenExpr{
            Lparen: 15,
            Rparen: 19,
            Expr:   &ast.BadExpr{
              BadNode: &ast.BadNode{
                NodePos: 16,
                NodeEnd: 19,
                Tokens:  []*token.Token{
                  &token.Token{
                    Kind: "%",
                    Raw:  "%",
                    Pos:  16,
                    End:  17,
                  },
                  &token.Token{
                    Kind:  "<int>",
                    Space: " ",
                    Raw:   "2",
                    Base:  10,
                    Pos:   18,
                    End:   19,
                  },
                },
              },
            },
          },
        },
      },
    },
  },
}

--- SQL
SELECT (1 +) * (% 2)

l := p.Lexer.Clone()
defer func() {
if r := recover(); r != nil {
// When parsing is failed on tryParseHint or tryParseWith, the result of these methods are discarded
Copy link
Contributor

@apstndb apstndb Dec 18, 2024

Choose a reason for hiding this comment

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

I have concerning discarding hints and CTEs means input statement can't be recovered by SQL(). I think it don't satisfy users needs. I believe that users expect parse-unparse is safe operation even if BadNode.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Parsing results are discarded, but the contents (tokens) are preserved. Therefore, your concern is not a problem.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I mean, parse-unparse is still safe after this PR.

Copy link
Contributor

@apstndb apstndb Dec 18, 2024

Choose a reason for hiding this comment

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

Thank you for explanation.
I understood all handleParse*Error will eat tokens between l.Token.Pos and their stop position.

@apstndb
Copy link
Contributor

apstndb commented Dec 20, 2024

I feel like this specification hasn't been fully discussed, and I have some personal concerns about the current behavior.

As a memefish contributor, it's clear that this change introduces new complexity. I believe there should be a strong enough benefit to justify introducing this complexity.

Furthermore, as a developer of tools that use memefish, I don't find the current specification sufficiently useful. Because Badnode implements too many interfaces, it cannot be used effectively for type assertions. In fact, it has the potential to introduce bugs, so I would likely choose to avoid using BadNode altogether.

My suggestion is to break down the Node into smaller, more specific types for implementing interfaces. If we did this, it would be genuinely more useful than before this PR, and I might actually use it going forward.

PoC branch: feature/bad-node...apstndb:memefish:feature/poc-bad-nodes

Typical code with my usecase
feature/bad-node HEAD https://go.dev/play/p/iOLRu_m1q3F
apstndb/feature/bad-nodes HEAD https://go.dev/play/p/lsXS1D-S8Q3
master HEAD https://go.dev/play/p/OydF15UWANp

@apstndb
Copy link
Contributor

apstndb commented Dec 20, 2024

Yeah, I may prefer ParseStatements() rather than ParseStatement() for this usecase.

feature/bad-node HEAD: https://go.dev/play/p/P0SgHl6vUGg
apstndb/feature/poc-bad-node HEAD: https://go.dev/play/p/jnujP1DM2vo

makenowjust added a commit that referenced this pull request Dec 20, 2024
makenowjust added a commit that referenced this pull request Dec 20, 2024
@makenowjust makenowjust changed the title Introduce BadNode and ErrorList Introduce BadNode and MultiError Dec 21, 2024
@makenowjust makenowjust merged commit 6f345ac into main Dec 21, 2024
4 checks passed
@makenowjust makenowjust deleted the feature/bad-node branch December 21, 2024 00:42
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.

2 participants