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

New lint: trait removed supertrait #232

Closed
Tracked by #5
obi1kenobi opened this issue Dec 19, 2022 · 9 comments · Fixed by #470
Closed
Tracked by #5

New lint: trait removed supertrait #232

obi1kenobi opened this issue Dec 19, 2022 · 9 comments · Fixed by #470
Labels
A-lint Area: new or existing lint E-help-wanted Call for participation: Help is requested to fix this issue. E-mentor Call for participation: Mentorship is available for this issue.

Comments

@obi1kenobi
Copy link
Owner

obi1kenobi commented Dec 19, 2022

Example of why this is breaking: say we removed the DerefMut: Deref supertrait relationship. Then, this code would break:

fn foo(x: impl Deref) {}

fn bar(x: impl DerefMut) {
    foo(x)
}

h/t https://twitter.com/i2talics/status/1559607755975057408

@obi1kenobi obi1kenobi changed the title trait removed supertrait New lint: trait removed supertrait Dec 19, 2022
@obi1kenobi obi1kenobi added A-lint Area: new or existing lint E-help-wanted Call for participation: Help is requested to fix this issue. E-mentor Call for participation: Mentorship is available for this issue. labels Dec 19, 2022
@era
Copy link
Contributor

era commented Jun 1, 2023

@obi1kenobi I can take this one if you don't mind me sitting on it for a couple of days, I'm still trying to understand exactly how the lint rules work (more specifically the trustfall queries).

edit: I think "sitting on it" is not the right expression, "if you don't mind me taking a couple of days trying out how to make this work"

@obi1kenobi
Copy link
Owner Author

No worries at all, take all the time you need and feel free to ask questions / open draft PRs and ask for feedback / etc. I'm happy to support you in whatever way you need.

You can play with the query language over rustdoc here: https://play.predr.ag/rustdoc

It can't compare crates -- it only queries one crate at a time -- and uses a slightly older plugin version so not all the bits of the schema cargo-semver-checks has are there (this is what the playground has instead). But it's a quick and easy way to write some queries, see what they do, and get used to the whole thing. There are example queries in the drop-down but you can also write your own as well.

@era
Copy link
Contributor

era commented Jun 4, 2023

I do have a question, I'm using the playground to understand better the information available on Trait. But I cannot get any data from it, for example if I try:

query {
  Crate {
    item {
      ... on Trait {
        name @output
        docs @output
        crate_id @output

        span {
          filename @output
          first_line: begin_line @output
        }
      }
    }
  }
}

My output is []. But from what I can see, all the attributes I'm looking for are present at the Schema. Also, if I change Trait to Struct I do get some data. I tried the query against this crate: https://docs.rs/http/0.2.8/http/all.html, which has two traits.

Do you know what I'm doing wrong?

@obi1kenobi
Copy link
Owner Author

obi1kenobi commented Jun 4, 2023 via email

@obi1kenobi
Copy link
Owner Author

Confirmed. Just a severely outdated and incomplete implementation. I'll see if I can upgrade it to something more feature-complete and modern in the next few days :)

@era
Copy link
Contributor

era commented Jun 6, 2023

Thank you for checking I was not crazy about the queries. I tried a bit locally and I think I understand how things are working on that side. I started looking at the code for the Adapter.

For exposing the information about the supertrait. If I understood it correctly this information is on GenericBounds (docs). This is an example of the rustdoc json:

"0:4:1601": {
      "id": "0:4:1601",
      "crate_id": 0,
   
        "bounds": [
          {
            "trait_bound": {
              "trait": {
                "name": "supertrait",
                "id": "0:3:1600",
                "args": {
                  "angle_bracketed": {
                    "args": [],
                    "bindings": []
                  }
                }
              },
              "generic_params": [],
              "modifier": "none"
            }
          }
        ],
        "implementations": []
      }
    },

for something like:

pub trait supertrait {}

pub trait base_trait : supertrait {}

So I think the schema outcome would be:

type Trait implements Item & Importable {
 # [...]
 # new field
  bounds: [GenericBound]
}

"""
https://docs.rs/rustdoc-types/latest/rustdoc_types/enum.GenericBound.html
"""
type GenericBound { 
  # I feel bad about having trait and outlives together here,
  # because GenericBound is either a Trait Bound or an "Outlives" bound.
  trait: TraitBound
  outlives: String!
}

type TraitBound {
   trait: String!
   # Omitting the other fields for now
}

(Please tell me any modification you want on the structure)

Based on that, I believe I need to modify a couple of things in the adapter:

  1. Because I'm modifying the Trait edge, I need to add my bounds property at resolve_trait_edge at edge.rs.
  2. I think I may need a resolve_generic_bounds similar to resolve_trait_edge because generic bounds are an edge type (correct me if I'm wrong).

My main question is how to "iterate" on the bounds vector. I think I have to implement as_generic_bound on vertex.rs, but I'm only "pattern matching" the code and not really understanding everything that is going on. Do you have a commit on the adapter code that exemplifies a little bit of the work I need to do there? :)

@obi1kenobi
Copy link
Owner Author

Sorry for the delay.

I suggested trying this issue precisely because I was hoping it would create exactly this kind of learning opportunity. Trustfall is unusual in the amount of flexibility it provides us to build a more pleasant representation than what the underlying format provides. Check this out!

Rustdoc JSON uses a GenericBound type, but that doesn't mean we have to use it too in our schema. We can design a schema that is more ergonomic (and decoupled from rustdoc implementation details which may change!), and the adapter can handle the translation.

Let's ignore rustdoc JSON's structure for a second, and just think about Rust. The following is valid Rust:

pub trait Supertrait<T> { ... }

pub trait MyTrait : Supertrait<i64> { ... }

How might we represent this in a Trustfall schema?

As you've already observed, our type Trait implements Item & Importable represents the pub trait MyTrait and pub trait Supertrait<T> items.

The Supertrait<i64> bit looks like our ImplementedTrait type: https://github.com/obi1kenobi/trustfall-rustdoc-adapter/blob/rustdoc-v26/src/rustdoc_schema.graphql#L623-L636

And MyTrait : Supertrait<i64> looks like an edge: Trait -[supertrait]-> ImplementedTrait.

What if GenericBound is an "outlives" lifetime parameter instead? We'd just make that another edge from Trait. This way we won't run into the problem of "GenericBound is either a supertrait or an 'outlives' lifetime" you pointed out.

Even if we try something really complex, like:

pub trait Bar<'a> {}

pub trait Foo<'a> : for<'b> Bar<'b> + 'a {}

this design still holds up: pub trait Foo<'a> and pub trait Bar<'a> are Trait vertices, for<'b> Bar<'b> is an ImplementedTrait vertex connected by the new supertrait edge, and + 'a would be a new edge that we haven't created yet to a new vertex type we also haven't created yet.

So with this design, all we're adding is a new supertrait: [ImplementedTrait!] edge inside type Trait implements Item & Importable, which shouldn't be too much work.

Does this make sense?

Last night I also updated the playground to use the same adapter code as cargo-semver-checks, so you can try querying crate rustdocs here: https://play.predr.ag/rustdoc

I didn't quite understand the question about iterating on bounds but perhaps playing around with some queries in the playground can help you find the answer? Or if you wouldn't mind giving me a bit more detail about it, that could work too.

@obi1kenobi
Copy link
Owner Author

Re-reading this, I feel like I did an B- job of explaining. It's hard! 😅

Let me try to find you some code from a prior schema addition, perhaps that will be more helpful. Try this one, it adds a field edge on Variant: https://github.com/obi1kenobi/trustfall-rustdoc-adapter/pull/25/files

I've since refactored the adapter code a bit, so the code from that PR ended up here now: https://github.com/obi1kenobi/trustfall-rustdoc-adapter/blob/rustdoc-v26/src/adapter/edges.rs#L199-L230

@era
Copy link
Contributor

era commented Jun 9, 2023

The explanation was perfect! I was actually expecting the changes to be more complex, so I thought I was missing something, but using ImplementedTrait helped me to better understand the whole flow.

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-lint Area: new or existing lint E-help-wanted Call for participation: Help is requested to fix this issue. E-mentor Call for participation: Mentorship is available for this issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants