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

API: Add sem::TyKind::implements_trait (Draft) #340

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

xFrednet
Copy link
Member

This PR allows users to check if a given semantic type implements a specific trait. The implementation is inspired by Clippy's implementation of clippy_utils::ty::implements_trait. It already works for simple traits, without generics.


I'm stuck on how to allow users to specify generic arguments for the trait. My idea for Marker's function was that users can construct a TestTraitRef, which specifies a trait with its potential generics. I think this would make it a bit cleaner than Clippy's approach of taking a trait_id: DefId and args: &[GenericArg<'tcx>]. It also makes the interface more extendable. The problem is, that I can't figure out how to let the user specify generics in this way...

First some context. Generic arguments for the type check can be constructed from rustc's semantic types. Clippy constructs new semantic types for the type check, if needed. This is something which we'll also have to do in Marker, to support generics in types. Clippy doesn't distinguish between real types from the compiler and fake types constructed for this check. For Marker, I don't think this is a real option. If we allow users to construct sem::TyKinds I see numerous problems coming our way. Instead, I considered having a simplified type representation, that users can use to construct types for testing, something like this:

#[derive(Debug)]
enum TestType<'a> {
    Ref(&'a Test<'a>),
    Tuple(&'a [&'a Test<'a>]),
    Type(u32)
}

Then we could construct semantic types in the backend for the type check. And this is where the issue begins. I can't figure out a way to deal with the references in a good way, since I would like to store the thing inside the TestTraitRef. For that, we would have to allocate memory somewhere, as 'a wouldn't live long enough otherwise.

Does anyone have a suggestion on how to best do this?


I can thing of three ways, which all feel a bit complicated:

  1. Simply use an allocator in marker_api to extend the lifetime. This would add a dependency, but allow us to use these kinds of references.
  2. Have a Vec<TestType> in the TestTraitRef that stores the nodes, extending their lifetimes to the life of TestTraitRef. This feels hacky and also doesn't solve all problems, as slices etc would need to be handled separately.
  3. Use Box<> instead of references. Here I fear that we lock us in into a design, which can't be changed later. We could hide the usage of Boxes, by denying the user access to the TestType enum with its variants directly, and instead require them to use a builder of some kind. Using a builder is probably smart in either case. It still feels weird to me.

@Veetaha, do you maybe have an idea, how could this be solved? If you have the time, I'd appreciate it, if you could also take a look at the interface I currently have. Maybe you have some early feedback, which I can incorporate.


cc: #141

@xFrednet xFrednet added C-enhancement Category: New feature or request A-api Area: Stable API labels Dec 18, 2023
@xFrednet xFrednet added this to the v0.5.0 milestone Dec 18, 2023
@@ -148,7 +148,7 @@ new_id! {
/// This id is used by the driver to lint the semantic type representation, back to the
/// driver type representation, if needed.
#[cfg_attr(feature = "driver-api", visibility::make(pub))]
pub(crate) DriverTyId: u64
pub(crate) DriverTyId: u128
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder why this changed. I'd expect such breaking change only during the nightly toochain version update.

}
}

pub fn trait_from_path(&mut self, path: impl Into<String>) -> &mut Self {
Copy link
Contributor

@Veetaha Veetaha Dec 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think in general we should avoid allocations especially in read-only APIs which is basically what all APIs are in marker_api. 99% of the time the trait name will be a &'static str. So maybe this could be

impl Into<Cow<('static|'_), str>>

Although sqlx isn't performance sensitive, but they did it in their method for loading the SQLite extensions here


pub fn build(&mut self) -> TestTraitRef {
TestTraitRef {
trait_ref: self.trait_ref.take().expect("the trait was never set"),
Copy link
Contributor

@Veetaha Veetaha Dec 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think buildstructor would be handy in this case since it validates such errors at compile time. Anyway, at some point we'll have to have a proc macro in marker_api.

With buildstructor you could even achieve the API like this:

ctx.test_trait()
   .arg_foo(...)
   .arg_bar(...)
   .call()

In this case you won't even need any owned value as an arg like a String. Although with this approach it's harder to save the args into a variable and basically impossible to put them into some struct or enum because the builder type buildstructor generates is something very complicated and potentially breakable, so treat it as an anonymous type. However, I think this would be good enough


#[derive(Debug)]
#[non_exhaustive]
pub enum TyDefIdSource {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this need to be pub?

/// // Definitions
/// trait SimpleTrait { /* ... */ }
/// trait GenericTrait<T> { /* ... */ }
/// trait TwoGenericTrait<T, U=u8> { /* ... */ }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is also a case for const generics in traits, although I that's a rare case and we can implement support for that later

/// trait TwoGenericTrait<T, U=u8> { /* ... */ }
/// ```
///
/// Now we have a `SimpleType` which implements a few traits:
Copy link
Contributor

@Veetaha Veetaha Dec 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe just squash this into one code snippet, and make this a line comment to DRY-up code snippet a bit

Comment on lines +357 to +361
// Let the record show: I'm not proud of this!!!

infcx
.evaluate_obligation(&obligation)
.is_ok_and(EvaluationResult::must_apply_modulo_regions)
Copy link
Contributor

@Veetaha Veetaha Dec 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if there is API in rustc that just returns the list of all traits that the type implements. For example, rustdoc can somehow display all the traits that the type implements both directly and via the blanket types. Looking at their impl of this will reveal how it works.

If we just had a method that returned the full list (slice or set?) then querying the trait implementations wouldn't require any special TestTraitRef DSL and other hussle (especially with const generics matching). The users would be able to just iterate over the trait implementations manually. Ideally use iterators for that.

I think this would be my ultimate suggestion for:

@Veetaha, do you maybe have an idea, how could this be solved?

Just not have "query" API, and instead have "full list" API, and offload querying to iterators.

@xFrednet xFrednet modified the milestones: v0.5.0, v0.6.0 Dec 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-api Area: Stable API C-enhancement Category: New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants