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
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 13 additions & 1 deletion marker_adapter/src/context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,7 @@ impl<'ast> MarkerContextWrapper<'ast> {
emit_diag,
resolve_ty_ids,
expr_ty,
ty_implements_trait,
span,
span_snippet,
span_source,
Expand All @@ -145,6 +146,7 @@ pub trait MarkerContextDriver<'ast> {
fn resolve_ty_ids(&'ast self, path: &str) -> &'ast [TyDefId];

fn expr_ty(&'ast self, expr: ExprId) -> marker_api::sem::TyKind<'ast>;
fn ty_implements_trait(&'ast self, ty: sem::TyKind<'ast>, trait_ref: &sem::FfiTestTraitRef<'_>) -> bool;
fn span(&'ast self, owner: SpanId) -> &'ast Span<'ast>;
fn span_snippet(&'ast self, span: &Span<'_>) -> Option<&'ast str>;
fn span_source(&'ast self, span: &Span<'_>) -> SpanSource<'ast>;
Expand All @@ -165,12 +167,22 @@ extern "C" fn resolve_ty_ids<'ast>(
unsafe { as_driver(data) }.resolve_ty_ids((&path).into()).into()
}

// False positive because `SemTyKind` is non-exhaustive
// False positive because `TyKind` is non-exhaustive
#[allow(improper_ctypes_definitions)]
extern "C" fn expr_ty<'ast>(data: &'ast MarkerContextData, expr: ExprId) -> marker_api::sem::TyKind<'ast> {
unsafe { as_driver(data) }.expr_ty(expr)
}

// False positive because `TyKind` is non-exhaustive
#[allow(improper_ctypes_definitions)]
extern "C" fn ty_implements_trait<'ast>(
data: &'ast MarkerContextData,
ty: sem::TyKind<'ast>,
trait_ref: &sem::FfiTestTraitRef<'_>,
) -> bool {
unsafe { as_driver(data) }.ty_implements_trait(ty, trait_ref)
}

extern "C" fn span<'ast>(data: &'ast MarkerContextData, span_id: SpanId) -> &'ast Span<'ast> {
unsafe { as_driver(data) }.span(span_id)
}
Expand Down
2 changes: 1 addition & 1 deletion marker_api/src/common/id.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.

}

new_id! {
Expand Down
7 changes: 6 additions & 1 deletion marker_api/src/context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ use crate::{
common::{ExpnId, ExprId, ItemId, Level, MacroReport, SpanId, SymbolId, TyDefId},
diagnostic::{Diagnostic, DiagnosticBuilder, EmissionNode},
ffi,
sem::TyKind,
sem::{self, TyKind},
span::{ExpnInfo, FileInfo, FilePos, Span, SpanPos, SpanSource},
Lint,
};
Expand Down Expand Up @@ -259,6 +259,9 @@ impl<'ast> MarkerContext<'ast> {
pub(crate) fn expr_ty(&self, expr: ExprId) -> TyKind<'ast> {
self.callbacks.call_expr_ty(expr)
}
pub(crate) fn ty_implements_trait(&self, ty: sem::TyKind<'ast>, trait_ref: &sem::FfiTestTraitRef<'_>) -> bool {
(self.callbacks.ty_implements_trait)(self.callbacks.data, ty, trait_ref)
}

// FIXME: This function should probably be removed in favor of a better
// system to deal with spans. See rust-marker/marker#175
Expand Down Expand Up @@ -322,6 +325,8 @@ struct MarkerContextCallbacks<'ast> {

// Internal utility
pub expr_ty: extern "C" fn(&'ast MarkerContextData, ExprId) -> TyKind<'ast>,
pub ty_implements_trait:
extern "C" fn(&'ast MarkerContextData, sem::TyKind<'ast>, &sem::FfiTestTraitRef<'_>) -> bool,
pub span: extern "C" fn(&'ast MarkerContextData, SpanId) -> &'ast Span<'ast>,
pub span_snippet: extern "C" fn(&'ast MarkerContextData, &Span<'ast>) -> ffi::FfiOption<ffi::FfiStr<'ast>>,
pub span_source: extern "C" fn(&'ast MarkerContextData, &Span<'_>) -> SpanSource<'ast>,
Expand Down
2 changes: 1 addition & 1 deletion marker_api/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
#![allow(clippy::unused_self)] // `self` is needed to change the behavior later
#![allow(clippy::missing_panics_doc)] // Temporary allow for `todo!`s
#![allow(clippy::new_without_default)] // Not very helpful as `new` is almost always cfged
#![cfg_attr(not(feature = "driver-api"), allow(dead_code))]
#![cfg_attr(not(feature = "driver-api"), allow(dead_code, unused_macros, unused_imports))]
#![cfg_attr(marker, warn(marker::marker_lints::not_using_has_span_trait))]

pub static MARKER_API_VERSION: &str = env!("CARGO_PKG_VERSION");
Expand Down
233 changes: 231 additions & 2 deletions marker_api/src/sem/ty.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,11 @@ pub use sequence_ty::*;
pub use trait_ty::*;
pub use user_ty::*;

use crate::common::DriverTyId;
use crate::{
common::{DriverTyId, TyDefId},
context::with_cx,
ffi::FfiSlice,
};
use std::{fmt::Debug, marker::PhantomData};

/// The semantic representation of a type.
Expand Down Expand Up @@ -113,7 +117,52 @@ impl<'ast> TyKind<'ast> {
}
ty
}

/// This function tests if a given type implements a trait, specified by
/// the given [`TestTraitRef`]. A [`TraitTestMode`] can be specified as
/// part of the trait reference.
#[must_use]
pub fn implements_trait(self, trait_ref: &TestTraitRef) -> bool {
let check_with_ids = |ids: &[TyDefId]| {
let ffi = FfiTestTraitRef {
trait_ids: ids.into(),
mode: trait_ref.mode,
};
with_cx(&self, |cx| cx.ty_implements_trait(self, &ffi))
};

match &trait_ref.trait_ref {
TyDefIdSource::Path(path) => check_with_ids(with_cx(&self, |cx| cx.resolve_ty_ids(path))),
TyDefIdSource::Id(id) => check_with_ids(&[*id]),
}
}
}

#[cfg(feature = "driver-api")]
impl<'ast> TyKind<'ast> {
impl_ty_kind_fn!(data() -> &CommonTyData<'ast>);
}

macro_rules! impl_ty_kind_fn {
($method:ident () -> $return_ty:ty) => {
impl_ty_kind_fn!($method() -> $return_ty,
Bool, Num, Text, Never,
Tuple, Array, Slice,
Fn, Closure,
Ref, RawPtr, FnPtr,
TraitObj, Adt, Generic, Alias,
Unstable
);
};
($method:ident () -> $return_ty:ty $(, $item:ident)+) => {
pub fn $method(&self) -> $return_ty {
match self {
$(TyKind::$item(data) => data.$method(),)*
}
}
};
}
use impl_ty_kind_fn;

#[repr(C)]
#[cfg_attr(feature = "driver-api", visibility::make(pub))]
Expand All @@ -139,7 +188,7 @@ impl<'ast> CommonTyData<'ast> {

macro_rules! impl_ty_data {
($self_ty:ty, $enum_name:ident) => {
#[cfg(feature = "driver_api")]
#[cfg(feature = "driver-api")]
impl<'ast> $self_ty {
pub fn data(&self) -> &$crate::sem::ty::CommonTyData<'ast> {
&self.data
Expand All @@ -154,3 +203,183 @@ macro_rules! impl_ty_data {
};
}
use impl_ty_data;

/// This struct describes a trait and its generic arguments. It is used to check
/// if a semantic type implements a trait.
///
/// See [`TyKind::implements_trait`].
#[derive(Debug)]
pub struct TestTraitRef {
trait_ref: TyDefIdSource,
// TODO generics
mode: TraitTestMode,
}

impl TestTraitRef {
pub fn builder() -> TestTraitRefBuilder {
TestTraitRefBuilder::new()
}
}

/// A builder used to construct a [`TestTraitRef`] instance.
#[derive(Debug)]
pub struct TestTraitRefBuilder {
trait_ref: Option<TyDefIdSource>,
mode: TraitTestMode,
}

impl TestTraitRefBuilder {
fn new() -> Self {
Self {
trait_ref: None,
mode: TraitTestMode::default(),
}
}

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

self.trait_ref = Some(TyDefIdSource::Path(path.into()));
self
}

pub fn trait_from_id(&mut self, id: impl Into<TyDefId>) -> &mut Self {
self.trait_ref = Some(TyDefIdSource::Id(id.into()));
self
}

pub fn mode(&mut self, mode: TraitTestMode) -> &mut Self {
self.mode = mode;
self
}

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

mode: self.mode,
}
}
}

#[derive(Debug)]
#[cfg_attr(feature = "driver-api", visibility::make(pub))]
pub(crate) struct FfiTestTraitRef<'a> {
trait_ids: FfiSlice<'a, TyDefId>,
// TODO generics
mode: TraitTestMode,
}

#[cfg(feature = "driver-api")]
impl<'a> FfiTestTraitRef<'a> {
pub fn trait_ids(&self) -> &'a [TyDefId] {
self.trait_ids.get()
}

pub fn mode(&self) -> TraitTestMode {
self.mode
}
}

#[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?

Id(TyDefId),
// TODO: Handle the path properly, since `Str` is not ABI safe
Path(String),
}

impl From<TyDefId> for TyDefIdSource {
fn from(value: TyDefId) -> Self {
TyDefIdSource::Id(value)
}
}

impl From<String> for TyDefIdSource {
fn from(value: String) -> Self {
TyDefIdSource::Path(value)
}
}

/// This enum defines how strict the [`TyKind::implements_trait`] test should be.
/// The difference is probably best illustrated by examples. For that, let's
/// take the following traits:
///
/// ```
/// // 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

/// ```
///
/// 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

///
/// ```
/// # trait SimpleTrait { /* ... */ }
/// # trait GenericTrait<T> { /* ... */ }
/// # trait TwoGenericTrait<T, U=u8> { /* ... */ }
/// struct SimpleType;
///
/// impl SimpleTrait for SimpleType {}
/// impl GenericTrait<i16> for SimpleType {}
/// // The second generic argument, defaults to `u8`.
/// impl TwoGenericTrait<u16> for SimpleType {}
/// ```
///
/// | Semantic type and checked trait | Lossy | Strict |
/// | --------------------------------------------------------- | ----- | ------ |
/// | `SimpleType` implements `SimpleTrait` | ✅ | ✅ |
/// | `SimpleType` implements `GenericTrait` | ✅ | ❌ |
/// | `SimpleType` implements `GenericTrait<!>` | ❌ | ❌ |
/// | `SimpleType` implements `TwoGenericTrait<u16>` | ✅ | ❌ |
/// | `SimpleType` implements `TwoGenericTrait<u16, u8>` | ✅ | ✅ |
/// | `SimpleType` implements `TwoGenericTrait<u16, !>` | ❌ | ❌ |
///
/// We can also check traits for types with generic parameter:
///
/// ```
/// # trait SimpleTrait { /* ... */ }
/// # trait GenericTrait<T> { /* ... */ }
/// # trait TwoGenericTrait<T, U=u8> { /* ... */ }
/// struct GenericType<T>(T);
///
/// impl<T> SimpleTrait for GenericType<T> {}
/// impl GenericTrait<u32> for GenericType<u32> {}
/// impl TwoGenericTrait<i32, i32> for GenericType<u32> {}
/// ```
/// | Semantic type and checked trait | Lossy | Strict |
/// | --------------------------------------------------------- | ----- | ------ |
/// | `GenericType<()>` implements `SimpleTrait` | ✅ | ✅ |
/// | `GenericType<!>` implements `GenericTrait<u32>` | ❌ | ❌ |
/// | `GenericType<u32>` implements `GenericTrait<u32>` | ✅ | ✅ |
/// | `GenericType<u32>` implements `TwoGenericTrait` | ✅ | ❌ |
/// | `GenericType<u32>` implements `TwoGenericTrait<i32>` | ✅ | ❌ |
/// | `GenericType<u32>` implements `TwoGenericTrait<i32, i32>` | ✅ | ✅ |
#[non_exhaustive]
#[derive(Debug, Copy, Clone, Default)]
pub enum TraitTestMode {
/// The comparison will enforce the given generic arguments and allow any
/// additional ones to be freely matched. This is useful for traits with
/// default types for generic parameters.
///
/// See the documentation of [`TraitTestMode`] for a comparison of the
/// different test modes.
#[default]
Lossy,
/// The comparison will check that the correct number of generics has been
/// provided and will check that all of them match.
///
/// See the documentation of [`TraitTestMode`] for a comparison of the
/// different test modes.
Strict,
}

pub struct UserDefinedGeneric<'a> {
_lifetime: PhantomData<&'a ()>,
}

#[derive(Debug)]
#[non_exhaustive]
#[cfg_attr(feature = "driver-api", visibility::make(pub))]
enum UserDefinedGenericInner<'a> {
ApiType(TyKind<'a>),
// TODO: Handle the path properly, since `Str` is not ABI safe
Path(String),
}
Loading