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

Add Natvis visualizations for some Regex types #849

Closed
wants to merge 5 commits into from

Conversation

ridwanabdillahi
Copy link

@ridwanabdillahi ridwanabdillahi commented Mar 18, 2022

Viewing Regex types under a debugger on Windows can be a bit difficult to comprehend. This change adds Natvis visualizations for a few of the common Regex types to help support the effort of debugging Regex on Windows.

RFC 3191 proposes adding support for embedding debugging visualizations such as Natvis in a Rust crate. This RFC has been approved, merged and implemented.

Natvis is a framework that can be used to specify how types should be viewed under a supported debugger, such as the Windows debugger (WinDbg) and the Visual Studio debugger.

This PR adds:

  • Natvis visualizations for some of the core types, {Captures, Match, Regex} and bytes::{Capture, Match, Regex}.
  • Tests for testing visualizers embedded in the regex crate.
  • Documentation for writing, embedding and testing visualizations defined for the regex crate.
  • Updates to the CI pipeline to ensure tests for visualizers are run so they do not break silently.
  • A new feature for the regex crate to enable the unstable debugger_visualizer feature.

Copy link
Member

@BurntSushi BurntSushi left a comment

Choose a reason for hiding this comment

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

Hiya! Sorry about the late response. I am just seeing this PR now. It must have slipped down my inbox.

Overall though, I'm on board with getting something like this merged. But I have some concerns that I'd like to try and address first, somewhat in order of priority:

  • Is there an easy way to automatically test that these definitions stay in sync with the code? Since they don't use the public API and instead rely on internal details, this seems pretty important.
  • Should we be adding docs to this repo for how to use this file? Does it get automatically picked up by the tooling? Just as a forcing function for this, I have no idea how to use this file. I'm not a Windows developer, but I do have a Windows machine I could test with. Personally, I don't like having stuff in the repo that even I have no idea how to use. So personally speaking, I would expect the docs here to be an end-to-end example of how to use this file. (I'd say, assume the user has a Rust toolchain installed and knows how to use it, but probably assume nothing else.)
  • It looks like this only adds support for the top-level {Captures, Match, Regex} types, but I think we basically want to duplicate these for bytes::{Capture, Match, Regex}.
  • Can we include a link to reference documentation for how to edit this natvis file?
  • Does it have to live in the root of this repo? It seems like a not-great situation to just throw every debugger specific config in the root of the repo.

Thanks again for this and apologies for the late response.

@ridwanabdillahi
Copy link
Author

ridwanabdillahi commented Jun 7, 2022

@BurntSushi

Thanks for your response!

Is there an easy way to automatically test that these definitions stay in sync with the code? Since they don't use the public API and instead rely on internal details, this seems pretty important.

Currently there isn't an easy way of integrating tests solely for debugger visualizers. The Rust Compiler has an internal testing framework it uses for testing debug info such as Natvis visualizations. The plan is to extend this testing framework to be usable by any and all crates. This is currently being developed by members of the Rust team and would be leveraged in crates such as this.

Should we be adding docs to this repo for how to use this file?

I'm not sure if there needs to be docs solely for this repo. The RFC has docs in terms of how to use the new attribute as well as references to both Natvis documentation and Pretty Printers API for Linux. Is there any documentation specific to the regex crate that you think needs to be added here?

Does it get automatically picked up by the tooling?

As defined in the RFC, in order for this file to be picked up, the #![debugger_visualizer] attribute can be used to instruct the compiler to embed this visualizer file into the crate. This way any crate that takes a dependency on regex, either directly or as a transitive dependency, would automatically have the visualizations applied when viewing regex types under a debugger which supports the given visualizer type. Since the #![debugger_visualizer] feature is still unstable I have not added it to the crate in this PR but the plan would be to add a simple #![debugger_visualizer(natvis_file = 'path/to/natvis')] to automatically pull in and embed the specified visualizer.

Just as a forcing function for this, I have no idea how to use this file. I'm not a Windows developer, but I do have a Windows machine I could test with.

One of the benefits of this feature would be that a Rust developer does not need to use this file. Once the Natvis file is embedded in the crate, spinning up a debugger that supports Natvis will automatically show the custom visualizations that have been defined for a given type.

If the question is How do I write up Natvis visualizations? There is a great amount of detail in the documentation for Natvis such as:

  • It looks like this only adds support for the top-level {Captures, Match, Regex} types, but I think we basically want to duplicate these for bytes::{Capture, Match, Regex}.

Yes this can certainly be done! I will look into adding Natvis for these types as well.

Can we include a link to reference documentation for how to edit this natvis file?

As stated above is this something we want to add specifically to the regex crate? The RFC contains such links already.

Does it have to live in the root of this repo? It seems like a not-great situation to just throw every debugger specific config in the root of the repo.

This is up to the crate owners. The Natvis files can live in any directory. The debugger_visualizer attribute takes a relative path to the visualizer file so it can live in a separate debug_metadata/natvis directory or whatever directory structure is requested.

@BurntSushi
Copy link
Member

Note: for context, I try hard to look at things with beginner eyes. In this context, I think I actually have beginner eyes. Not only do I rarely use a debugger, but I also rarely use Windows or any of its debuggers. So the entire end-to-end flow is totally foreign to me. Yet, this PR is proposing adding stuff to this project that directly impacts and (hopefully) improves the quality of life for folks who use debuggers on Windows. I know very little about that, but I think it's important that not only should I know how to run through a full end-to-end flow to test things like this, but the regex project itself should maintain its own independent documentation for how to work through that flow. The docs will likely be slightly redundant, but they will also be coupled and tied to the regex project which gives an opportunity to write about real examples for the actual things you want to test.

Anyway, sorry for the long response here. I didn't have time to make it shorter.

Currently there isn't an easy way of integrating tests solely for debugger visualizers. The Rust Compiler has an internal testing framework it uses for testing debug info such as Natvis visualizations. The plan is to extend this testing framework to be usable by any and all crates. This is currently being developed by members of the Rust team and would be leveraged in crates such as this.

OK, so to me this means that this file is virtually guaranteed to break until there's some way to test it. Which I guess maybe we just have to live with? And rely on people using this file to submit patches fixing it when they notice? (If they notice, what is the failure mode for accessing a field that doesn't exist?)

I'm not sure if there needs to be docs solely for this repo. The RFC has docs in terms of how to use the new attribute as well as references to both Natvis documentation and Pretty Printers API for Linux. Is there any documentation specific to the regex crate that you think needs to be added here?

The docs in the RFC are totally incomplete IMO. I cannot follow them to test this file. I expect the docs to include the full end-to-end flow for users. That includes getting the debugger setup and all of that. These are also the same instructions I would like to follow in order to manually test this file.

The full picture doesn't have to live in this repo, but it has to live somewhere IMO. If it isn't in this repo, then I would expect to be able to link to some official resource that gives that full end-to-end flow.

As defined in the RFC, in order for this file to be picked up, the #![debugger_visualizer] attribute can be used to instruct the compiler to embed this visualizer file into the crate. This way any crate that takes a dependency on regex, either directly or as a transitive dependency, would automatically have the visualizations applied when viewing regex types under a debugger which supports the given visualizer type. Since the #![debugger_visualizer] feature is still unstable I have not added it to the crate in this PR but the plan would be to add a simple #![debugger_visualizer(natvis_file = 'path/to/natvis')] to automatically pull in and embed the specified visualizer.

Hmmm okay. I'd prefer to close this PR then until the feature gets stabilized. Because I think that if I merged as-is, the file wouldn't actually be doing anything, right? We can always revive the PR.

One of the benefits of this feature would be that a Rust developer does not need to use this file. Once the Natvis file is embedded in the crate, spinning up a debugger that supports Natvis will automatically show the custom visualizations that have been defined for a given type.

Yeah I understand that, but I want to be able to test this. IMO, I'm going to maintain this repo that advertises support for debugger visualizations, then I want to test that it works.

I've been through this type of thing in the past. I do not like bringing things into a project that I don't understand or don't know how to test.

If the question is How do I write up Natvis visualizations? There is a great amount of detail in the documentation for Natvis such as:

OK, so the TODO item here is to work through those docs and write down the specific steps needed to check the natvis file here. I'm not necessarily asking you to do that---I can do it---but that's the kind of detail I want to see written down so that I can run through the full flow myself.

Yes this can certainly be done! I will look into adding Natvis for these types as well.

Thanks!

As stated above is this something we want to add specifically to the regex crate? The RFC contains such links already.

Emphatically, yes. Consider:

  • There are no tests verifying that the natvis file is in sync with the internal representation details of the corresponding types, so the natvis file will break at some point.
  • Someone on Windows making use of the natvis file (or a resource generated for it, to be more precise) notices that their debug representations have either disappeared or become worse in some way. Specifically for regex types.
  • They google about debug reprs for values are generated in their debugger and learn about natvis files for the first time.
  • They venture into the regex repo to look a natvis file to see if they can fix it.
  • They realize they don't know the format of this file. At this point, they can just go google it and hopefully they land on the right page. Or we can put a link to the docs in the natvis file to save them that search step.

I don't think the RFC helps here, even if its target audience was beginners. The RFC doesn't describe how to edit natvis files. I think we need to link to natvis documentation somewhere in the natvis file itself.

Let's leave a breadcrumb trail for folks to follow.

This is up to the crate owners. The Natvis files can live in any directory. The debugger_visualizer attribute takes a relative path to the visualizer file so it can live in a separate debug_metadata/natvis directory or whatever directory structure is requested.

Great! Perfect.

@ridwanabdillahi
Copy link
Author

@BurntSushi Thank you for taking the time to give such a detailed response.

I know very little about that, but I think it's important that not only should I know how to run through a full end-to-end flow to test things like this, but the regex project itself should maintain its own independent documentation for how to work through that flow.

I agree, asking maintainers of a crate to take ownership of these visualizer files such as a Natvis file without sufficient documentation and references would not be ideal. I'll be sure to create this documentation for the regex crate and add examples of how to create, edit, embed, and test the Natvis files as well as the links to the Natvis schema and framework.

OK, so to me this means that this file is virtually guaranteed to break until there's some way to test it. Which I guess maybe we just have to live with?

I'll work with some of the members of the Rust team who have been working on the testing infrastructure for debug info and see if I can pull in the testing infrastructure into this crate so we can ensure the Natvis is never broken. This work has already been started so hopefully I can pull this into regex and add the necessary tests into the CI pipeline. I'll keep this PR as draft until I can get the testing infrastructure set up so that we can have confidence in the Natvis that will be shipped out with this crate.

And rely on people using this file to submit patches fixing it when they notice? (If they notice, what is the failure mode for accessing a field that doesn't exist?)

If a Natvis visualization is broken for whatever reason, the debugger still allows viewing the default view without any special visualizations being applied. This means the user could look at the default representation of types which was the case before adding any of these custom visualizations.

Hmmm okay. I'd prefer to close this PR then until the feature gets stabilized. Because I think that if I merged as-is, the file wouldn't actually be doing anything, right? We can always revive the PR.

Yeah I understand that, but I want to be able to test this.

That's totally valid. I'll start working on trying to integrate some tests for the Natvis I have already defined for regex into the CI pipeline as well as local testing so that we can have confidence these files do not regress or break silently. In the meantime, I'll keep this PR as draft and continue the work to invest in adding the right testing solution.

OK, so the TODO item here is to work through those docs and write down the specific steps needed to check the natvis file here. I'm not necessarily asking you to do that---I can do it---but that's the kind of detail I want to see written down so that I can run through the full flow myself.

I believe I can write the docs for this as well. Thank you for taking the time to engage with me on this, I'll begin writing up the docs we've talked about here for the regex crate.

Let's leave a breadcrumb trail for folks to follow.

Agreed. The documentation I'll write up should have sufficient information on how to write, edit, test and embed visualizer files that are supported by the debugger_visualizer feature. Thank you for being clear and concise on the type of documentation missing, I'll work on those right away.

Great! Perfect.

Is there a specific directory structure you would like to see for the first Natvis file I have added for the regex crate?

@ridwanabdillahi
Copy link
Author

Hello @BurntSushi

I updated the PR with the changes you requested and added both testing and documentation about debugger visualizers and how to test them locally. Do you have any time to give these changes a look over?

@BurntSushi
Copy link
Member

All right, so apologies for taking so long to get back to you, but I've been heads down working on #656. And this issue is dealing with stuff a bit outside my normal scope.

OK, so firstly, I am going to close this PR for now. The reason for that is that this relies on an unstable feature. I mentioned this above in a comment that I think we should close this until it's stabilized. Basically, I am not currently interested in adding more unstable feature dependencies to this crate. I've done it in the past and it has generally been a maintenance headache. While I'm not 100% opposed to it in all cases, it needs to provide a very compelling benefit. I don't think it clears that bar here.

Secondly, I'm not a fan of including the debug tester libraries here. I gave them each a quick skim, and I suspect we could drop the proc macro and just inline what we need from the actual tester. There isn't much code. Again, the reason for this is that I try hard not to be beholden to the maintenance policies of others for regex.

With all that said, the docs here look lovely. Thank you so much for doing that.

I'd be happy to reconsider a change like this once the feature is stabilized. Note that it is currently being tracked here: rust-lang/rust#95939

And here is its entry in the unstable book: https://doc.rust-lang.org/beta/unstable-book/language-features/debugger-visualizer.html

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.

None yet

2 participants