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

Convert tree-sitter parse error exn to Parse_info.Parsing_error #1767

Merged
merged 1 commit into from
Oct 2, 2020

Conversation

aryx
Copy link
Collaborator

@aryx aryx commented Oct 1, 2020

There is already some code to automatically intercept Parse_info exns
to display their location, so better to use a consistent scheme
across pfff and tree-sitter parsers.

test plan:

(semgrep) pad@yrax:~/semgrep/tests/OTHER/parsing_errors$ semgrep -l ts -e 'FOO' .
warn: parse error
  --> err.ts:2
2 |          return 1+
  |          ^^^^^^^^^
= help: If the code appears to be valid, this may be a semgrep bug.
Could not parse err.ts as ts

Warnings exist. Run with `--strict` to turn warnings into errors.

better than the previous error that was always reported on the first line.
This also improves error location for the errors reported in
#1713

There is already some code to automatically intercept Parse_info exns
to display their location, so better to use a consistent scheme
across pfff and tree-sitter parsers.

test plan:
```
(semgrep) pad@yrax:~/semgrep/tests/OTHER/parsing_errors$ semgrep -l ts -e 'FOO' .
warn: parse error
  --> err.ts:2
2 |          return 1+
  |          ^^^^^^^^^
= help: If the code appears to be valid, this may be a semgrep bug.
Could not parse err.ts as ts

Warnings exist. Run with `--strict` to turn warnings into errors.
```

better than the previous error that was always reported on the first line.
This also improves error location for the errors reported in
#1713
@aryx aryx requested a review from mjambon October 1, 2020 14:46
let loc = mk_tree_sitter_error ts_error in
let info = { PI.token = PI.OriginTok loc; transfo = PI.NoTransfo } in
raise (PI.Parsing_error info)
end else raise exn
Copy link
Member

Choose a reason for hiding this comment

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

To get the constructor, you can do Printexc.exn_slot_name exn (since ocaml 4.02). But I don't see how to do the rest without Obj, other than catching the exception in the child (or make the parser return an Ok/Error).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yep, the parser could return an algebraic data type. Only exn (open variants) have this marshalling issue.
Still, I like this hacky solution :)

@aryx aryx requested review from mjambon and nbrahms October 1, 2020 16:00
Copy link
Collaborator Author

@aryx aryx left a comment

Choose a reason for hiding this comment

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

But removing the use of Parallel.invoke will also solve the issue and let us use exceptions properly.

let loc = mk_tree_sitter_error ts_error in
let info = { PI.token = PI.OriginTok loc; transfo = PI.NoTransfo } in
raise (PI.Parsing_error info)
end else raise exn
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yep, the parser could return an algebraic data type. Only exn (open variants) have this marshalling issue.
Still, I like this hacky solution :)

@aryx aryx merged commit 0ab5151 into develop Oct 2, 2020
@aryx aryx deleted the convert_exn branch October 2, 2020 06:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants