Semver Check for Removing a Bound on Impl

[DavidArchibald]

Issue overview

I found out about this project and then recently happened to read this interesting article which included a note about an existing "semver hazard":

Interestingly, while it is true that you can remove bounds from a struct (today, at least) and be at semver complaint3, this is not the case for impls. For example if I have

impl<T: Copy> MyTrait for Vec<T> { }

and I change it to impl<T> MyTrait for Vec<T> this is effectively introducing a new blanket impl, and that is not a semver compliant change (see RFC 2451 for more details).

I've checked and making this change gives no warning so it seems to not be implemented yet. There's a chance the article is wrong as I actually am not 100% sure how to reproduce this semver uncompliant behaviour because I keep running into coherence rules stopping me from reproducing something in another crate that would be broken by this change. However given their Rust experience I would be inclined to take this claim at face value.

---

An aside on contributing

I'd be interested in implementing this, largely out of just general interest, and I was looking to make a PR with this in due time but I quickly got out of my depth.

I began to put forward some work to making this and I would note that I spent about 30-45 minutes fumbling around because I didn't know GraphQL but more importantly because I didn't understand where CrateDiff, ImplParent, etc. all come from. I eventually figured out it comes from trustfall-rustdoc-adapter and I would personally recommend linking to this, specifically to rustdoc_schema.graphql in Contributing.md to save others time.

---

My work so far

I'll start with an apology for the length of this section. If I had more time to familiarize myself I think I would be able to write significantly less about this. I'm essentially trying to dump the majority of the information I have as I don't know how to filter it down to the useful portions.

This actually seems to be a bit trickier than I initially expected to implement. To start with it seems that vital data, specifically the generics property on Impl has not been typed in graphql yet. This in of itself would not be a critical problem as there's many types that aren't but unfortunately it seems to be a quite complex property. Besides being a deep type it also uses enums and there's no examples as far as I can tell of an enum implementation elsewhere.

Here's an example of the json given: {"trait_bound":{"trait":{"name":"Into","id":"2:3190:150","args":{"angle_bracketed":{"args":[{"type":{"kind":"primitive","inner":"u8"}}],"bindings":[]}}}}}. Yes, some of these are union types in Rust code and should reflect that way in GraphQL more than likely.

This has been my first go at implementing it in trustfall-rust-adapter on the graphql side (sent as a txt file for brevity): Generic.txt. It's quite likely there's invalid syntax or the like here but I think this is how it'd have to be implemented. While there's a lot of missing variants in these enums (| {} is used to allow an unimplemented variant) if the mapping is going to be 1:1 this is how it should look like.

*I'm not actually sure what an edge actually is contextually, it seems to be used for anything that isn't simple like a boolean though.

Unfortunately I think it may be even more complex from the Rust side of the adapter, there's so many new types to adapt for starters and enums seem annoying. For this reason I think maybe it should be decoded into a graphql type that looks like this:

type GenericTraitBound {
     bound_str: String; # something like "T: Copy", "T: Into<u8>" etc.
}

If this is possible, the logic would be as simple as if bound_str from baseline becomes null it's a semver breaking change. As a side-effect its also easy to display, turning the accurate graphql back into a string seems quite painful. Unfortunately this does limit checking other bounds changes that could easily be breaking.

[obi1kenobi]

Hi David! This is awesome! Thanks for digging into cargo-semver-checks and Trustfall so deeply, and sorry for the sharp edges. I literally only yesterday pulled the adapter and schema out of this repo and into trustfall-rustdoc-adapter in #133, and #139 fixes the schema link in CONTRIBUTING.md exactly as you suggest, but sadly hasn't merged yet. Really unfortunate timing, and I'm sorry about that.

I've added this new semver issue to the list in #5, which is where we keep track of which lints are being implemented where.

If you're still interested in going for this, I'm happy to mentor and help out in whatever form is most useful.

Implementing generics and their bounds well in the schema is going to be a bit tricky because of their complexity and recursive nature (Foo<Bar<Baz<A>>, Quux<B>>), so this won't be the easiest lint to get started with: it will require multiple new vertex types, edges, and properties in the Trustfall schema, and they aren't going to be trivial to design.

If you'd like, I can also suggest an easier semver check to implement, where the schema and query are a bit more straightforward, so you don't have to jump right into the deep end.

Finally, here's a query playground where you can run some Trustfall queries against the rustdoc of the top ~100 crates on crates.io. It can't do comparisons between versions (no CrateDiff vertex type) but might still be useful for getting a hang of the query language: https://play.predr.ag/rustdoc

[DavidArchibald]

Is there a messaging platform outside of Github issues that we can communicate through more quickly? I would love to send back and forth some messages that don't fit into the niche buckets of what's in an issue. I decided to pivot towards implementing must_use as it seems to have been much easier, though I'm struggling to add any new property, I keep getting stuff like CompilationError(ValidationError(NonExistentPath(["CrateDiff", "baseline", "item", "attribute", "path"]))) despite my best efforts (it's in the graphql file and rust file etc. but I'm obviously missing something). I may attempt to pick this up later once I've done more.

There's a couple of things I would like to ask, for example what operations for @filter have been implemented. I found here a list of operations but the only one I've verified as working is has_substring and not_has_substring. But I have many other small questions like whether you'd like to implement a monorepo style you can always do what Rust does, a git subtree, to make PRs across what would otherwise be multiple repos easier etc. See this old issue with git subtree, as well as this article which touches on it.

[obi1kenobi]

Here's an example of the json given: {"trait_bound":{"trait":{"name":"Into","id":"2:3190:150","args":{"angle_bracketed":{"args":[{"type":{"kind":"primitive","inner":"u8"}}],"bindings":[]}}}}}. Yes, some of these are union types in Rust code and should reflect that way in GraphQL more than likely.

A hybrid approach between this and the bound_str approach might be best: have an interface type that all the "union" variants implement, and have the interface include the bound_str property while the types that implement the interface have the more detailed structure.

Trustfall isn't quite GraphQL (just syntactically equivalent so we get moder editor conveniences for free) and doesn't currently support unions. It's probably not that difficult add support for them, and I'm happy to mentor for that as well, but I'm not sure how many repos you're okay with touching with your first contribution here 😅

This has been my first go at implementing it in trustfall-rust-adapter on the graphql side (sent as a txt file for brevity): Generic.txt. It's quite likely there's invalid syntax or the like here but I think this is how it'd have to be implemented. While there's a lot of missing variants in these enums (| {} is used to allow an unimplemented variant) if the mapping is going to be 1:1 this is how it should look like.

It's quite difficult to review schemas in isolation like this — it's easy for a schema to look "completely fine" but then end up not supporting some key query. I'd recommend writing down some key queries together with the new schema and seeing how they might pan out.

*I'm not actually sure what an edge actually is contextually, it seems to be used for anything that isn't simple like a boolean though.

It's ultimately up to the person designing the schema to decide what makes sense to be an edge. Pretty much the only thing that must be an edge is if you have a one-to-many relationship between objects that aren't simple scalars or (nested) lists of such, like "items in a crate" i.e. the Crate -[item]-> Item edge. If you have a one-to-one relationship, you can choose to make it an edge or you can choose to "inline" one type into the other — this is purely a schema ergonomics decision.

If you have Twitter, ping me there and we can find a platform that works better? If not, shoot me an email at the email address in my commits?

[obi1kenobi]

Is there a messaging platform outside of Github issues that we can communicate through more quickly? I would love to send back and forth some messages that don't fit into the niche buckets of what's in an issue. I decided to pivot towards implementing must_use as it seems to have been much easier, though I'm struggling to add any new property, I keep getting stuff like CompilationError(ValidationError(NonExistentPath(["CrateDiff", "baseline", "item", "attribute", "path"]))) despite my best efforts (it's in the graphql file and rust file etc. but I'm obviously missing something).

I'm happy to debug this together. The error message sounds like you are trying to look up a path property on Attribute (CrateDiff -[baseline]-> Crate -[item]-> Item -[attribute]-> Attribute) but the schema says value is the only field on that type. Here's a playground link.

There's a couple of things I would like to ask, for example what operations for @filter have been implemented. I found here a list of operations but the only one I've verified as working is has_substring and not_has_substring.

Nice! The link you found is the predecessor project to Trustfall, which is similar in many ways. I'm sorry that Trustfall documentation is badly lacking at the moment — it's something I intend to spend significant time on in the rest of the year. A list of questions would be extremely helpful for me to know what docs to write first, so please send it over or just ask me and I'll write them down.

These are the supported operations and their names currently:
https://github.com/obi1kenobi/trustfall/blob/86b74b8336b4c1301d7a02d1309d684df58cf437/trustfall_core/src/graphql_query/directives.rs#L116-L139

But I have many other small questions like whether you'd like to implement a monorepo style you can always do what Rust does, a git subtree, to make PRs across what would otherwise be multiple repos easier etc.

On changing the repo style and similar broad questions, I defer to @epage. The top priority is to make #61 happen as quickly as possible and with no speed bumps, and everything else is in service of that.