search

iomentum / cargo-breaking

Mozilla Public License 2.0
112 stars 7 forks source link

Panic with glob use #40

Open epage opened 2 years ago

epage commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

[...] 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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

You can checkout my prototype, cargo-api.

Example commands

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

Interesting flags

Interesting implementation details

rustdoc limitations:

scrabsha commented 2 years ago

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 commented 2 years ago

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:

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 commented 2 years ago

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 :eyes:).

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 commented 2 years ago

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 commented 2 years ago

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

obi1kenobi commented 2 years ago

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'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 commented 2 years ago

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 :)