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

feature: added a pest_debugger crate (fixes #98) #736

Merged
merged 2 commits into from
Nov 24, 2022

Conversation

tomtau
Copy link
Contributor

@tomtau tomtau commented Nov 20, 2022

based on the old PR by @dragostis: #277

Changes that were made:

  • debugger core context was refactored and extracted to a lib (so that it could be used in other frontends, e.g. editor plugins)
  • CLI was extended using rustyline helpers to provide file completions, history etc.
  • applied suggestions from @hansihe from the old PR (Added initial debugger implementation. #277 (comment)):
  1. added ba (add breakpoints at all rules) which is useful for stepping through the entire grammar, plus breakpoint deletions and loading input directly from readline;
  2. added command line arguments.
  • changed the listener function to return a boolean, so that the debugger can signal back to a parsing thread to finish before reaching its input's EOF.

@tomtau tomtau requested a review from a team November 20, 2022 06:43
@tomtau tomtau requested a review from a team as a code owner November 20, 2022 06:43
@tomtau tomtau requested review from NoahTheDuke and removed request for a team November 20, 2022 06:43
@tomtau tomtau linked an issue Nov 20, 2022 that may be closed by this pull request
@tomtau
Copy link
Contributor Author

tomtau commented Nov 20, 2022

Hm, two unintended semver violations:

Description:
A publicly-visible function now takes a different number of parameters.
        ref: https://doc.rust-lang.org/cargo/reference/semver.html#fn-change-arity
       impl: https://github.com/obi1kenobi/cargo-semver-check/tree/v0.13.0/src/queries/function_parameter_count_changed.ron

Failed in:
  pest_meta::validator::validate_undefined now takes 2 parameters instead of 3, in meta/src/validator.rs:186
  pest_meta::validator::validate_pest_keywords now takes 1 parameters instead of 2, in meta/src/validator.rs:141
  pest_meta::validator::validate_rust_keywords now takes 1 parameters instead of 2, in meta/src/validator.rs:120
--- failure auto_trait_impl_removed: auto trait no longer implemented ---

Description:
A public type has stopped implementing one or more auto traits. This can break downstream code that depends on the traits being implemented.
        ref: https://doc.rust-lang.org/reference/special-types-and-traits.html#auto-traits
       impl: https://github.com/obi1kenobi/cargo-semver-check/tree/v0.13.0/src/queries/auto_trait_impl_removed.ron

Failed in:
  type Vm is no longer Sync, in vm/src/lib.rs:37
  type Vm is no longer RefUnwindSafe, in vm/src/lib.rs:37
  type Vm is no longer Send, in vm/src/lib.rs:37

@tomtau
Copy link
Contributor Author

tomtau commented Nov 20, 2022

The first semver violation is unrelated to the current PR, it's due to this older change: https://github.com/pest-parser/pest/pull/554/files that slipped the older semver violation checks... shall we add back the original fn arity and just pass it the statics, or leave it as it is (those functions aren't probably externally depended on / they looked that they shouldn't have been made public anyway)?

The second semver violation is legit... I guess two possible ways around it are: 1) create a new wrapper struct (e.g. TracingVm) that will have the listener callback and isn't Send + Sync + RefUnwindSafe, 2) wrap the listener field in Arc and Mutex?
@NoahTheDuke any thoughts?

@tomtau
Copy link
Contributor Author

tomtau commented Nov 20, 2022

or one more alternative is to change ListenerFn to Box<dyn Fn(String, &Position<'_>) -> bool + Sync + Send + RefUnwindSafe> ... but it'd require using a different channel implementation, e.g. the crossbeam one: https://docs.rs/crossbeam-channel/0.5.6/crossbeam_channel/struct.Sender.html (the standard library one is only Send, i.e. it isn't Sync and RefUnwindSafe)

@tomtau
Copy link
Contributor Author

tomtau commented Nov 20, 2022

or one more alternative is to change ListenerFn to Box<dyn Fn(String, &Position<'_>) -> bool + Sync + Send + RefUnwindSafe> ... but it'd require using a different channel implementation, e.g. the crossbeam one: https://docs.rs/crossbeam-channel/0.5.6/crossbeam_channel/struct.Sender.html (the standard library one is only Send, i.e. it isn't Sync and RefUnwindSafe)

maybe this is the easiest / most straightforward: 7f8e605

based on the old PR by @dragostis: pest-parser#277

Changes that were made:
- debugger core context was refactored and extracted to a lib (so that
it could be used in other frontends, e.g. editor plugins)
- CLI was extended using rustyline helpers to provide file completions,
history etc.
- applied suggestions from @hansihe from the old PR
(pest-parser#277 (comment)):
1. added `ba` (add breakpoints at all rules) which is useful
for stepping through the entire grammar, plus breakpoint deletions
and loading input directly from readline;
2. added command line arguments.
- changed the listener function to return a boolean, so that
the debugger can signal back to a parsing thread to finish
before reaching its input's EOF.

Co-authored-by: Dragoș Tiselice <dragostiselice@gmail.com>
@NoahTheDuke
Copy link
Member

fn-change-arity

Bummer that I introduced a SemVer violation. I agree that those functions probably shouldn't be public, but they are and we have no plans on hiding them. I think you're right that re-adding the argument and just calling it with the static variable would be a good solution.

That being said, it's been a year and no one has complained. Is there any way for us to survey public usage of Pest and see if anyone is even using the functions? Could help us know 1) our options and 2) the best way to move forward.

auto_trait_impl_removed

I don't know much about this stuff (Send and Sync specifically) but your proposed solution looks like it works well.

@tomtau
Copy link
Contributor Author

tomtau commented Nov 23, 2022

That being said, it's been a year and no one has complained. Is there any way for us to survey public usage of Pest and see if anyone is even using the functions? Could help us know 1) our options and 2) the best way to move forward.

@NoahTheDuke if we take code available on GH as an indicator, then those functions weren't used by anyone except from within pest itself (code search only finds pest forks with those functions):

https://github.com/search?q=validate_pest_keywords+language%3ARust&type=code&l=Rust
https://github.com/search?q=validate_rust_keywords+language%3ARust&type=code&l=Rust
https://github.com/search?q=validate_undefined+language%3ARust&type=code&l=Rust

@NoahTheDuke
Copy link
Member

That's great news. Then we're probably good with leaving them as is and maybe noting it in a changelog some place.

@tomtau tomtau merged commit 8c602d8 into pest-parser:master Nov 24, 2022
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.

Grammar backtrace
2 participants