search

servo / rust-url

URL parser for Rust
https://docs.rs/url/
Apache License 2.0
1.31k stars 325 forks source link

Add Natvis visualizations for the `Url` type #783

Closed ridwanabdillahi closed 2 years ago

ridwanabdillahi commented 2 years ago

This change adds Natvis visualizations for the Url type in the url crate to help improve the debugging experience on Windows.

Debugging types such as the Url type under a Windows debugger, WinDbg or the Visual Studio debugger, can be a bit difficult to dissect exactly what data is contained in each type. The Rust compiler does have Natvis support for some types, but this is limited to some of the core libraries and not supported for external crates.

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:

codecov-commenter commented 2 years ago

Codecov Report

Merging #783 (46ec473) into master (a72f83d) will not change coverage. The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #783   +/-   ##
=======================================
  Coverage   85.23%   85.23%           
=======================================
  Files          22       22           
  Lines        3894     3894           
=======================================
  Hits         3319     3319           
  Misses        575      575
Impacted Files Coverage Δ
url/src/lib.rs 75.76% <ø> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

ridwanabdillahi commented 2 years ago

This is great stuff, thank you. Please fix the warnings if possible.

Great, I'll fix the warnings and update the PR. Thanks for the review!

ridwanabdillahi commented 2 years ago

Please fix the CI / Test (ubuntu-latest, 1.45.0) and cargo fmt lint. Thanks!

Sorry about the lint failures @valenting

Looking at the CI/Test failure, this is actually caused by a new version of the serde_derive crate that was published yesterday and was not caused by this change. The error shows that version 1.0.143 of the serde or serde_derive crates require the resolver Cargo feature which was not stable in the version of Cargo used by the Rust 1.45.0 toolchain.

I'm not entirely sure why this new version of these crates requires the resolver Cargo feature when the prior versions did not. I was able to reproduce this locally on the master branch by running a cargo update followed by a cargo build --all-targets.

One solution would be to explicitly depend on version 1.0.142 or lower of these crates but I'm not sure if that's what we want to do here. Thoughts?

valenting commented 2 years ago

One solution would be to explicitly depend on version 1.0.142 or lower of these crates but I'm not sure if that's what we want to do here. Thoughts?

How about we don't run the serde CI task on 1.45.0?

ridwanabdillahi commented 2 years ago

How about we don't run the serde CI task on 1.45.0?

From what I can tell there isn't a specific serde CI task. If you mean disabling the entire 1.45.0 CI job then that should probably be a separate change from this PR with a relevant issue opened and linked.

I'm not sure if the url crate has a minimum supported Rust version (MSRV) that's dependent on the 1.45.0 Rust toolchain but the crate maintainers would probably be better served to answer that question.

ridwanabdillahi commented 2 years ago

@valenting

I opened up an issue for this CI failure https://github.com/servo/rust-url/issues/785 and a corresponding PR https://github.com/servo/rust-url/pull/786 to update the minimum rust-version to 1.51.0 which is when the resolver feature was stabilized

bors-servo commented 2 years ago

:umbrella: The latest upstream changes (presumably #786) made this pull request unmergeable. Please resolve the merge conflicts.

valenting commented 2 years ago

It seems the test failed on Windows.

ridwanabdillahi commented 2 years ago

It seems the test failed on Windows.

The latest nightly compiler contained changes to the debuginfo type layout for enums. The Natvis definitions have been updated and the test is passing.