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

chore(docs): Adds graph debug documentation to book #379

Merged
merged 4 commits into from
Jun 10, 2024

Conversation

afreeland
Copy link
Contributor

I ran across this comment while having some issues with regex support. Thought this was beneficial information and wanted to try and include it in the book so it was a little more visible to people that may be trying to debug and understand how things are working

@jeertmans jeertmans added the book Related to the book label Feb 19, 2024
Copy link
Collaborator

@jeertmans jeertmans left a comment

Choose a reason for hiding this comment

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

Hey! Thank you some much for your contribution! I left a few review comments that I'd like you to address before we can merge :-)

Feel free to accept or reject my suggestions!

Comment on lines 1 to 3
# Debug Graph

It may be beneficial during debugging to be able to visualize how Logos has generated its graph.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe add some introduction to the fact that Logos construct a graph internally to generate its code. Here, this comes a bit out of nowhere :-)

@@ -10,6 +10,7 @@
+ [Using `Extras`](./extras.md)
+ [Using callbacks](./callbacks.md)
+ [Common regular expressions](./common-regex.md)
+ [Debug Graph](./debug-graph.md)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd maybe add just a Debugging section, and, in the debug.md file, have one section talking about the graph. So we can later add some other sections, e.g., to talk about regex-syntax's HIR or else.

}
```

With our graph debugging enabled, we see the following output:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
With our graph debugging enabled, we see the following output:
Logos actually constructs a graph that contains the logic for matching tokens:

}
```

Lets look at `@9` for the character `.` we see that it will jump `=>` to `2`. We can then follow that by looking at `@2` which resolves to our `::Period` token. It will then continue to look for any matches in the case there is potential continuation after the `.` character. In the _input_ we provided there is not, since it is the end of our input.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
Lets look at `@9` for the character `.` we see that it will jump `=>` to `2`. We can then follow that by looking at `@2` which resolves to our `::Period` token. It will then continue to look for any matches in the case there is potential continuation after the `.` character. In the _input_ we provided there is not, since it is the end of our input.
This graph can help us understand how our patterns are matched, and maybe understand why we have a bug at some point.
Let's look at the last node, `9`, for the character `.`. We see that it will jump (`=>`) to `2`. We can then follow that by looking at `2` which resolves to our `::Period` token. It will then continue to look for any matches in the case there is potential continuation after the `.` character. In the _input_ we provided, there is not, since it is the end of our input.

Note that I have to read a bit more of the code to feel confident about how Graph actually works.

Also, this may be a bit counter-intuitive to take the example of ., where this is the last bit of our input string.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The thing I was to know (I guess the information is somewhere in the source code) is whether we always enter the graph via its last entry, or not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Totally agree about it being counter-intuitive about the . example. I used the . since it seemed a little more straight forward on the graph to understand. Could probably smooth over that some if I reworked some of the wording to lead into a bit more, instead of BLAM, understand this lol


Lets look at `@9` for the character `.` we see that it will jump `=>` to `2`. We can then follow that by looking at `@2` which resolves to our `::Period` token. It will then continue to look for any matches in the case there is potential continuation after the `.` character. In the _input_ we provided there is not, since it is the end of our input.

We also can try to identify how `fast` works by looking at `@9` that `f` jumps to `7`. This will then resolve the last letters of our word _fast_ by matching `ast` which jumps to `8`. Since our provided _input_ to the lexer does not include alphanumeric character after the word "fast" but rather whitespace, the token `::Fast` will be recognized. Then the graph will look for further potential continuation (ie `[g-z] => 4`)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
We also can try to identify how `fast` works by looking at `@9` that `f` jumps to `7`. This will then resolve the last letters of our word _fast_ by matching `ast` which jumps to `8`. Since our provided _input_ to the lexer does not include alphanumeric character after the word "fast" but rather whitespace, the token `::Fast` will be recognized. Then the graph will look for further potential continuation (ie `[g-z] => 4`)
We also can try to identify how `fast` works by looking at `9`, first, and seeing that `f` makes it jump to `7`. This will then resolve the last letters of our word _fast_ by matching `ast` which jumps to `8`. Since our provided _input_ to the lexer does not include alphanumeric character after the word "fast" but rather whitespace, the token `::Fast` will be recognized. Then the graph will look for further potential continuation (i.e., `[g-z] => 4`)

book/src/debug-graph.md Outdated Show resolved Hide resolved
@afreeland
Copy link
Contributor Author

Yeah, sorry for the kind of rushed PR...trying to sneak this in-between family time lol

Made some improvements to language and file structure based off of your recommendations. Hopefully, a little less jarring to someone reading it.

As for the enable debugging part, I totally agree, I am new to rust but I can try to put something together that properly uses a debug-mode kind of flag. Will keep it as a separate commit or could be a follow up PR if I get a chance. Would make life much nicer if something was an easy feature bit to flip.

jeertmans added a commit that referenced this pull request Feb 20, 2024
This adds a debug feature, similar to what Clap does, as requested by #379.

The current implementation only debugs a few parts of the code, and it may be interesting to add more debug messages in the different crates.
@jeertmans
Copy link
Collaborator

Yeah, sorry for the kind of rushed PR...trying to sneak this in-between family time lol

Made some improvements to language and file structure based off of your recommendations. Hopefully, a little less jarring to someone reading it.

As for the enable debugging part, I totally agree, I am new to rust but I can try to put something together that properly uses a debug-mode kind of flag. Will keep it as a separate commit or could be a follow up PR if I get a chance. Would make life much nicer if something was an easy feature bit to flip.

No issue, I did it myself since this was an easy fix. The issue you had was that you needed to declare a debug feature in sub-crates. See #382 for more details :-)

jeertmans added a commit that referenced this pull request Feb 20, 2024
* chore(lib): add debug feature

This adds a debug feature, similar to what Clap does, as requested by #379.

The current implementation only debugs a few parts of the code, and it may be interesting to add more debug messages in the different crates.

* chore(lint): run cargo fmt
Copy link
Collaborator

@jeertmans jeertmans left a comment

Choose a reason for hiding this comment

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

Nice work @afreeland! I left some more comments (sorry to be picky about some ^^'), but I hope you don't mind..

Note that #382 was merged and just added a new debug feature.

book/src/debugging.md Outdated Show resolved Hide resolved
book/src/debugging.md Outdated Show resolved Hide resolved
book/src/debugging.md Outdated Show resolved Hide resolved
book/src/debugging.md Outdated Show resolved Hide resolved
book/src/debugging.md Outdated Show resolved Hide resolved
book/src/debugging.md Outdated Show resolved Hide resolved
book/src/debugging.md Outdated Show resolved Hide resolved
book/src/debugging.md Outdated Show resolved Hide resolved
Co-authored-by: João Marcos <marcospb19@hotmail.com>
@jeertmans jeertmans mentioned this pull request Jun 2, 2024
@abhillman
Copy link

abhillman commented Jun 2, 2024

Users wishing to use this functionality may do so, pending this patch making it upstream to crates.io, with the following:

logos = { git = "https://github.com/maciejhirsz/logos.git#afreeland:debug-graph", features = ["debug"]}

@abhillman
Copy link

abhillman commented Jun 2, 2024

I haven't had a chance to dive in yet, but does logos have a runtime-accessible public API for fetching, manipulating, and constructing graphs exist besides using macros? Some reasons I can think of why those APIs would be useful:

fetching [related to this PR]

  • if a project contains multiple lexers, being able to fetch a graph and print could be easier for users than printing all graphs, as is done in this PR
  • in many cases, such an API would enable a developer to easily use the logos macro DSL to produce a lexer file for other tools (flex and its many derivations, for example)

manipulating/construction [out of scope for this PR, yet topically related]:

  • although this may require additional changes, this would allow for producing lexers at runtime

What this PR might look like if there is/were a fetch API

The documentation would mention something like this:

let graph = Token::graph();
println!("{:#?}", graph)

Continuing to use a feature flag to enable ::graph() (if something like it doesn't already exist) would still also probably make sense.

Thoughts?

@jeertmans
Copy link
Collaborator

I haven't had a chance to dive in yet, but does logos have a runtime-accessible public API for fetching, manipulating, and constructing graphs exist besides using macros? Some reasons I can think of why those APIs would be useful:

fetching [related to this PR]

  • if a project contains multiple lexers, being able to fetch a graph and print could be easier for users than printing all graphs, as is done in this PR
  • in many cases, such an API would enable a developer to easily use the logos macro DSL to produce a lexer file for other tools (flex and its many derivations, for example)

manipulating/construction [out of scope for this PR, yet topically related]:

  • although this may require additional changes, this would allow for producing lexers at runtime

What this PR might look like if there is/were a fetch API

The documentation would mention something like this:

let graph = Token::graph();
println!("{:#?}", graph)

Continuing to use a feature flag to enable ::graph() (if something like it doesn't already exist) would still also probably make sense.

Thoughts?

This is true that is might be nice to expose that as part of the public API. Would you mind creating an issue for that feature request?

@jeertmans jeertmans changed the title Adds graph debug documentation to book chore(docs): Adds graph debug documentation to book Jun 10, 2024
Comment on lines 56 to 58
#[cfg(feature = "debug")]
dbg!(graph);

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think you don't need this, because I already added a debug print of the graph in another place

Copy link
Collaborator

Choose a reason for hiding this comment

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

See here:

debug!("Generating code from graph: {graph:#?}");

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hey @afreeland! Could you address this review please? So I can accept your PR and merge it :-)

@jeertmans jeertmans merged commit 640882a into maciejhirsz:master Jun 10, 2024
18 checks passed
@eikopf eikopf mentioned this pull request Jul 29, 2024
akrantz01 referenced this pull request in akrantz01/antsi Aug 2, 2024
[![Mend
Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [logos](https://logos.maciej.codes/)
([source](https://togithub.com/maciejhirsz/logos)) | dependencies |
patch | `0.14.0` -> `0.14.1` |

---

### Release Notes

<details>
<summary>maciejhirsz/logos (logos)</summary>

###
[`v0.14.1`](https://togithub.com/maciejhirsz/logos/releases/tag/v0.14.1):
0.14.1 - Debug feature and fixes

#### What's Changed

- fix(doc): reset logos2 to logos by
[@&#8203;jeertmans](https://togithub.com/jeertmans) in
[https://github.com/maciejhirsz/logos/pull/372](https://togithub.com/maciejhirsz/logos/pull/372)
- chore(book): add JSON-borrowed parser example by
[@&#8203;jeertmans](https://togithub.com/jeertmans) in
[https://github.com/maciejhirsz/logos/pull/373](https://togithub.com/maciejhirsz/logos/pull/373)
- Add Rc<T> and Arc<T> sources by
[@&#8203;InfiniteCoder01](https://togithub.com/InfiniteCoder01) in
[https://github.com/maciejhirsz/logos/pull/340](https://togithub.com/maciejhirsz/logos/pull/340)
- Fix unicode dot by [@&#8203;RustyYato](https://togithub.com/RustyYato)
in
[https://github.com/maciejhirsz/logos/pull/376](https://togithub.com/maciejhirsz/logos/pull/376)
- chore(docs): cleanup examples by
[@&#8203;jeertmans](https://togithub.com/jeertmans) in
[https://github.com/maciejhirsz/logos/pull/381](https://togithub.com/maciejhirsz/logos/pull/381)
- chore(lib): add debug feature by
[@&#8203;jeertmans](https://togithub.com/jeertmans) in
[https://github.com/maciejhirsz/logos/pull/382](https://togithub.com/maciejhirsz/logos/pull/382)
- Cleanup unused Source features by
[@&#8203;kmicklas](https://togithub.com/kmicklas) in
[https://github.com/maciejhirsz/logos/pull/335](https://togithub.com/maciejhirsz/logos/pull/335)
- chore(deps): bump peaceiris/actions-mdbook from 1 to 2 by
[@&#8203;dependabot](https://togithub.com/dependabot) in
[https://github.com/maciejhirsz/logos/pull/387](https://togithub.com/maciejhirsz/logos/pull/387)
- Fix `Lexer::clone` leak and UB + tests by
[@&#8203;Jakobeha](https://togithub.com/Jakobeha) in
[https://github.com/maciejhirsz/logos/pull/390](https://togithub.com/maciejhirsz/logos/pull/390)
- fix(lib): correctly handle miss for loop in loop by
[@&#8203;lukas-code](https://togithub.com/lukas-code) in
[https://github.com/maciejhirsz/logos/pull/393](https://togithub.com/maciejhirsz/logos/pull/393)
- chore(lib): remove error branch from LUT if it is unreachable by
[@&#8203;RustyYato](https://togithub.com/RustyYato) in
[https://github.com/maciejhirsz/logos/pull/386](https://togithub.com/maciejhirsz/logos/pull/386)
- fix(docs): typo by
[@&#8203;joerivanruth](https://togithub.com/joerivanruth) in
[https://github.com/maciejhirsz/logos/pull/396](https://togithub.com/maciejhirsz/logos/pull/396)
- chore(docs): Adds graph debug documentation to book by
[@&#8203;afreeland](https://togithub.com/afreeland) in
[https://github.com/maciejhirsz/logos/pull/379](https://togithub.com/maciejhirsz/logos/pull/379)
- chore: drop python linting frmo pre-commit-config by
[@&#8203;LeoDog896](https://togithub.com/LeoDog896) in
[https://github.com/maciejhirsz/logos/pull/403](https://togithub.com/maciejhirsz/logos/pull/403)
- refactor: don't use deprecated max_value() method by
[@&#8203;LeoDog896](https://togithub.com/LeoDog896) in
[https://github.com/maciejhirsz/logos/pull/404](https://togithub.com/maciejhirsz/logos/pull/404)
- chore(version): bump logos version to 0.14.1 by
[@&#8203;jeertmans](https://togithub.com/jeertmans) in
[https://github.com/maciejhirsz/logos/pull/409](https://togithub.com/maciejhirsz/logos/pull/409)
- fix(docs): change old 0.14.0 by
[@&#8203;jeertmans](https://togithub.com/jeertmans) in
[https://github.com/maciejhirsz/logos/pull/410](https://togithub.com/maciejhirsz/logos/pull/410)

#### New Contributors

- [@&#8203;InfiniteCoder01](https://togithub.com/InfiniteCoder01) made
their first contribution in
[https://github.com/maciejhirsz/logos/pull/340](https://togithub.com/maciejhirsz/logos/pull/340)
- [@&#8203;RustyYato](https://togithub.com/RustyYato) made their first
contribution in
[https://github.com/maciejhirsz/logos/pull/376](https://togithub.com/maciejhirsz/logos/pull/376)
- [@&#8203;Jakobeha](https://togithub.com/Jakobeha) made their first
contribution in
[https://github.com/maciejhirsz/logos/pull/390](https://togithub.com/maciejhirsz/logos/pull/390)
- [@&#8203;lukas-code](https://togithub.com/lukas-code) made their first
contribution in
[https://github.com/maciejhirsz/logos/pull/393](https://togithub.com/maciejhirsz/logos/pull/393)
- [@&#8203;joerivanruth](https://togithub.com/joerivanruth) made their
first contribution in
[https://github.com/maciejhirsz/logos/pull/396](https://togithub.com/maciejhirsz/logos/pull/396)
- [@&#8203;afreeland](https://togithub.com/afreeland) made their first
contribution in
[https://github.com/maciejhirsz/logos/pull/379](https://togithub.com/maciejhirsz/logos/pull/379)
- [@&#8203;LeoDog896](https://togithub.com/LeoDog896) made their first
contribution in
[https://github.com/maciejhirsz/logos/pull/403](https://togithub.com/maciejhirsz/logos/pull/403)

**Full Changelog**:
maciejhirsz/logos@v0.14...v0.14.1

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined),
Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you
are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the
rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update
again.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR was generated by [Mend
Renovate](https://www.mend.io/free-developer-tools/renovate/). View the
[repository job log](https://developer.mend.io/github/akrantz01/antsi).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy40NDAuNyIsInVwZGF0ZWRJblZlciI6IjM3LjQ0MC43IiwidGFyZ2V0QnJhbmNoIjoibWFpbiIsImxhYmVscyI6W119-->

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
book Related to the book
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants