Panic with glob use

[epage]

Trying to figure out why its not working with clap (syn parse error), I'm trying out various projects of mine and got a panic with toml_edit

$ RUST_BACKTRACE=1 cargo breaking -a 12dfe318048b8774f4aba3ca8fe9ea49393a0bb6
thread 'main' panicked at 'not yet implemented: Glob(UseGlob { star_token: Star })', src/public_api/imports.rs:242:17
stack backtrace:
   0: rust_begin_unwind
             at /rustc/59eed8a2aac0230a8b53e89d4e99d55912ba6b35/library/std/src/panicking.rs:517:5
   1: core::panicking::panic_fmt
             at /rustc/59eed8a2aac0230a8b53e89d4e99d55912ba6b35/library/core/src/panicking.rs:101:14
   2: cargo_breaking::public_api::imports::flatten_use_tree::flatten_use_tree_inner
             at /home/epage/src/personal/cargo-breaking/src/public_api/imports.rs:242:17
   3: cargo_breaking::public_api::imports::flatten_use_tree::flatten_use_tree_inner
             at /home/epage/src/personal/cargo-breaking/src/public_api/imports.rs:207:17
   4: cargo_breaking::public_api::imports::flatten_use_tree::flatten_use_tree_inner
             at /home/epage/src/personal/cargo-breaking/src/public_api/imports.rs:207:17
   5: cargo_breaking::public_api::imports::flatten_use_tree
             at /home/epage/src/personal/cargo-breaking/src/public_api/imports.rs:247:5
   6: <cargo_breaking::public_api::imports::ExportedItemsVisitor as syn::gen::visit::Visit>::visit_item_use
             at /home/epage/src/personal/cargo-breaking/src/public_api/imports.rs:189:30
   7: syn::gen::visit::visit_item
             at /home/epage/.cargo/registry/src/github.com-1ecc6299db9ec823/syn-1.0.82/src/gen/visit.rs:2189:13
   8: syn::gen::visit::Visit::visit_item
             at /home/epage/.cargo/registry/src/github.com-1ecc6299db9ec823/syn-1.0.82/src/gen/visit.rs:359:9
   9: syn::gen::visit::visit_file
             at /home/epage/.cargo/registry/src/github.com-1ecc6299db9ec823/syn-1.0.82/src/gen/visit.rs:1859:9
  10: syn::gen::visit::Visit::visit_file
             at /home/epage/.cargo/registry/src/github.com-1ecc6299db9ec823/syn-1.0.82/src/gen/visit.rs:288:9
  11: cargo_breaking::public_api::imports::PathResolver::new
             at /home/epage/src/personal/cargo-breaking/src/public_api/imports.rs:34:9
  12: cargo_breaking::public_api::PublicApi::from_ast
             at /home/epage/src/personal/cargo-breaking/src/public_api.rs:46:24
  13: cargo_breaking::glue::extract_api
             at /home/epage/src/personal/cargo-breaking/src/glue.rs:44:15
  14: cargo_breaking::run
             at /home/epage/src/personal/cargo-breaking/src/lib.rs:26:23
  15: cargo_breaking::main
             at /home/epage/src/personal/cargo-breaking/src/main.rs:4:5
  16: core::ops::function::FnOnce::call_once
             at /rustc/59eed8a2aac0230a8b53e89d4e99d55912ba6b35/library/core/src/ops/function.rs:227:5
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.

[epage]

I can see how globs can make things complicated. Maybe add logging support and just warn the user that its not being handled, until support can be implemented?

[scrabsha]

Hi! Thanks for opening an issue
We're slowly moving from "parsing the crate code with syn" to "using rustc and cargo directly". This process allows us to have complete dependency resolution (which we can't get with syn) and to reuse as much code from rustc as possible.
The rewrite is progressing very slowly for now. You can look at the rustc-backend branch for more.

That being said, you are completely right by saying that we should not panic on glob imports. As you suggested, logging something is IMO the most appropriate solution for now.

We'll also need to update the readme to clarify the ongoing rewrite situation.

Thanks!

[epage]

While I'm not as closely tied into what problems you have run into, I find that unfortunate. One of the appealing aspects of cargo-breaking was that it worked on stable, unlike semverver. As I mentioned in #31, some of the release automation tools were hoping to depend on the lib portion for checking for breaking changes. As a user, I also find its easier to install and deal with stable tools than ones tied to a specific nightly release (and all of the special components this requires).

[scrabsha]

One of the appealing aspects of cargo-breaking was that it worked on stable, unlike semverver.

To be clear, the cargo-breaking that you're using on main uses the nightly toolchain.

TLDR: in order to get macro expansion to work, we're parsing the output of cargo expand (this is not exactly true, but that's a good approximation), which requires nightly. As such, in its current state in main, cargo-breaking can be built with any toolchain, but in order to run it, the nightly toolchain must be installed. If you think that this is a bad idea since we lose hygiene, don't worry, this only applies to variables, while we only care about items :)

As a user, I also find its easier to install and deal with stable tools than ones tied to a specific nightly release (and all of the special components this requires).

Yes. I agree with you. I don't like forcing people to use nightly either.

This really is not optimal for people who want to use cargo-breaking as a library, but i see no other stable option which allow us to be correct and to not have to reimplement everything on our own (well, actually we could use RUSTC_BOOTSTRAP, but i hope we will never have to do this).

Something that i'd like to experiment with is using the rustdoc json output to generate the diagnostics. I already have some pet projects and a couple of blogposts to write about it, and i'm confident that this would make cargo-breaking development easier. The problem is that this is nightly-only too (because rustdoc -w is an unstable option). We would end up in the same situation as main where we could use any toolchain to build but require nightly to be installed when the tool is run. Once again, this is not ideal, but i don't see any other solution.

[epage]

For myself, I find delegating to a nightly toolchain (with a generally wide gamut of support) much more palatable than only running with a specific nightly. I am also willing to accept some level of loss of completeness rather than build against a specific nightly toolchain.

I recognize different people might weigh things out differently, depending on their circumstances.

That said, rustdoc -w seems like the ideal route if it works.

[scrabsha]

[...] I find delegating to a nightly toolchain (with a generally wide gamut of support) much more palatable than only running with a specific nightly.

I agree. After thinking about it more, i think this is still not completely safe: the output of rustdoc -wjson is unstable, meaning that the rustdoc devs will change its format at any moment. There's a specifield field in the output to denote this. This means that in the worse scenario, we may have to quickly release a new version that supports a newer nightly toolchain.

That being said, i do think that the output of rustdoc -wjson changes less frequently than the internal API of rustc itself, so that's an improvement. For both the user and the cargo-breaking devs.

The only loss of information i can think of is the exact size of a given type. In my opinion that's not a big deal, since size can be platform-dependant, and is not stable across compilations (see the doc of mem::size_of), but other people seem to have other opinion on this (see #33).

The "rustdoc approach" would be really awesome. It deserves a POC before i start re-re-implementing everything again. I currently don't time for this. I will give it a try in January.

[epage]

Once I get my clap3 rc0 out next week (was hoping cargo breaking was going to help catch breaking changes lost through the eons of time), I'll use the gap before the official release to tackle a PoC.

I might start it as a separate crate and see about merging so I can iterate with CI and Issues :).

[epage]

That being said, i do think that the output of rustdoc -wjson changes less frequently than the internal API of rustc itself, so that's an improvement.

I think this is a key part. Similarly, the "cargo expand" output is unstable ... but its fairly stable. I imagine rustdoc fits between "cargo expand" and rustc-backend in terms of stability while keeping the level of work relatively low compared to "cargo expand", being a worthwhile trade off.

[epage]

You can checkout my prototype, cargo-api.

Example commands

• cargo api will show the user how cargo-api sees your crate
• cargo api --diff will find the last tag in the workspace's git repo and diff the APIs

So far, it can extract APIs into a hopefully workable data model but no diffing is implemented yet.

Interesting flags

• --manifest-path, --package, --exclude nearly like regular cargo commands
• --dump-raw mode to see what rustdoc shows
• --format flag (all modes) for choosing between how to render the results (not all formats are supported in all modes yet)
• --git rev, --path Cargo.toml, and --registry pkgname (not yet implemented) choose what you diff against

Interesting implementation details

• It doesn't implement support for alternative ways of grabbing the API (syn, etc) but it leaves the door open for it
• It also looks at manifests so we can find breaking changes in those (bumping major on public dependencies, removing feature flags, etc)
• It checks out the old git rev to an alternative directory so it won't clash with any local edits and allows caching for faster operations in the future

rustdoc limitations:

• ICE with rustdoc json backend  rust-lang/rust#89097
• [rustdoc-json] Assertion error on blanket impls rust-lang/rust#83718
• Doesn't capture enum discriminant.

[scrabsha]

Hi @epage

I have discussed with my employer about the future of cargo-breaking and we concluded that using rustc-as-a-library brings a lot of problems that nobody here is willing to solve. As such, we have decided that it is better to switch to a rustdoc-based approach, even if it forces us to fix some small bugs (such as the ones you linked in your comment).

We don't want our tool to compete with yours. It would be a waste of time. So instead of continuing cargo-breaking maintenance, we decided that I should contribute to cargo-api instead, if that's good to you :)

If you don't plan to maintain cargo-api, that's fine. I'll rewrite cargo-breaking instead. What we really do not want to happen is two tools using similar approach to do the same thing competing with each other.

[epage]

That sounds great!

I agree about collaborating and not duplicating efforts. My intention with cargo-api was to test the idea and demo my thoughts on the UI for you all. I am fine with you taking over the code base or us working together (that in part is depending on my availability since I have a decent amount of other stuff I'm maintaining it'd be great if I can be less involved in some of my projects :) )

I think the only questions are:

• Name / UI moving forward: As I call out in an issue, the name already needs to be changed. I'm leaning towards cargo crate-api, cargo breaking felt too limited in scope / didn't jive with how i was exposing things.
• Where to develop it: here is under the crate-ci org
• License: I went with the standard Rust licenses. I am the only committer so far, so it can be re-licensed if needed (except for the code forked from rustdoc).

Somehow visibility on it got raised, so I have heard from one or two other parties interested in the concept. I'm hoping they'll contribute rather than say "eh, someone else started the project I was going to do, I'll let them take care of it all".

[scrabsha]

Let address each point separately:

Name / UI moving forward: As I call out in an issue, the name already needs to be changed. I'm leaning towards cargo crate-api, cargo breaking felt too limited in scope / didn't jive with how i was exposing things.

Something we could do is trying to find what's common between our two projects. From my understanding, we both focus on what API item has changed between now and a given git revision (or tag, whatever) in a project. We could collaborate to write a generic library (with a generic-enough name) which addresses this. Once we get a prototype, we could build tools which use this library. For instance, i could use it in order to guess the next crate version, or someone else could automatically generate changelogs (looking at you cargo-release 👀).

The name cargo-breaking does not feel right for an API change detection library, but (IMO) it feels right for breaking change detection and appropriate versioning.

Where to develop it: here is under the crate-ci org

We could build the change detection library anywhere. Both me and my colleague have no requirement over this. The crate-ci org would be a good idea since it is central and neutral, just as the library needs to be.

License: I went with the standard Rust licenses. I am the only committer so far, so it can be re-licensed if needed (except for the code forked from rustdoc).

Once again, we could go with MIT/Apache (which is fairly common afaik) for the "central" library and let everyone use the license they want for their final binary crate.

The only reason why we use MPL is because when we started working on cargo-breaking, we wanted to avoid problems where big orgs start selling services built with our code. Our business plan has shifted since this, and we don't care anymore these days.

I think this partially addresses this comment.

[epage]

If you want to focus collaboration on the crate-api crate, thats fine. Our CLIs on top will end up being fairly thin.

btw I'd recommend checking out resolve_source_path in cargo-api to speed things up / avoid clobbering user files.

[o0Ignition0o]

Just read about https://github.com/rust-lang/rust-semverver which may or may not come in handy

[obi1kenobi]

I recently started https://crates.io/crates/cargo-semver-checks which is rustdoc-based and may also be of interest. From my chat with @epage the other day, it sounded like its query-based checking model offers some advantages over alternative options, and he'd be interested (time permitting) to help with CLI usability & ease of adoption improvements in cargo-semver-checks.

TL;DR of the query-based approach: every semver check is implemented as a strongly-typed declarative query. The query language is a GraphQL derivative, using GraphQL syntax but with semantics that allow better expressiveness and performance, and is provided by a project called Trustfall. This lowers the cost of writing and maintaining checks, which in turn has at least two positives:

• It becomes feasible and maintainable to split checks into specialized cases that can provide better ergonomics and nicer error messages. For example, instead of a single "type X is no longer " check, it's cheap and easy to add separate checks for auto-traits, common derives, non-derive built-in traits like From, and user-defined traits — each with custom error messages and specific advice on how to resolve the issue.
• It makes it easy to implement related checks like "removal of the last private field of an exhaustive pub struct." This check is valuable because it non-obviously adds "can construct the struct using a literal" to the public API, the removal of which is a semver-major breaking change.

It's still early days for cargo-semver-checks, and it's far from perfect. But it does work and does catch semver issues in the real world. If you get a chance to check it out, I'd love your feedback. I could also certainly use help maintaining it since y'all are very experienced and I'm still fairly new to the Rust ecosystem.

[o0Ignition0o]

Hey,

I've been following the cargo-semver-checks updates very closely and it's a really interesting approach!

I'll have to dig much deeper to understand how trustfall works, but I totally agree our limited time and efforts could be used to focus on one crate.

Let's see what @scrabsha and @zdimension think, But IIRC @zdimension had a look previously and thought the way you folks tackled this is brilliant, so I'd be surprised if they weren't willing to focus on cargo-semver-checks as well :)