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

Removed return type inference from Image API #987

Closed
wants to merge 2 commits into from
Closed

Conversation

oisyn
Copy link
Contributor

@oisyn oisyn commented Jan 11, 2023

By referring to concrete 'sibling' vector types from same vector library based on input.

@eddyb this currently causes an ICE in tests/ui/image/gather_err.rs

oisyn added 2 commits January 11, 2023 11:11
By referring to concrete 'sibling' vector types from same vector library based on input.
/// `<T::VectorTypeLib as VectorTypeRef<f32,3>>::Vector`
/// Alternatively, using the `VectorFromVector!` macro:
/// `VectorFromVector!(T, f32, 3)`
pub trait VectorTypeRef<T: crate::scalar::Scalar, const N: usize> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Ref in Rust tends to pretty specifically tied to references, maybe this could be named MakeVectorType or even HasVectorType? Especially given that it's implemented by the "library" type.

Comment on lines 56 to +71
#[cfg(feature = "glam")]
unsafe impl Vector<f32, 4> for glam::Vec4 {}
unsafe impl Vector<f32, 4> for glam::Vec4 {
type VectorTypeLib = GlamVectorTypeLib;
}
#[cfg(feature = "glam")]
impl VectorTypeRef<f32, 2> for GlamVectorTypeLib {
type Vector = glam::Vec2;
}
#[cfg(feature = "glam")]
impl VectorTypeRef<f32, 3> for GlamVectorTypeLib {
type Vector = glam::Vec3;
}
#[cfg(feature = "glam")]
impl VectorTypeRef<f32, 4> for GlamVectorTypeLib {
type Vector = glam::Vec4;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Whenever you have a bidirectional mapping like this, it's time to write a macro that takes in the information exactly once, e.g.:

macro_rules! glam_vector_impls {
    ($($scalar:ident {
        $($($n:literal : $ty:ident)),+ $(,)?
    }),+ $(,)?) => {
        $($(
            #[cfg(feature = "glam")]
            unsafe impl Vector<$scalar, $n> for glam::Vec4 {
                type VectorTypeLib = GlamVectorTypeLib;
            }
            #[cfg(feature = "glam")]
            impl VectorTypeRef<$scalar, $n> for GlamVectorTypeLib {
                type Vector = glam::$ty;
            }
        )+)+
    }
}
glam_vector_impls! {
    f32 => { 2:  Vec2, 3:  Vec3, 4:  Vec4 },
    f64 => { 2: DVec2, 3: DVec3, 4: DVec4 },
    i32 => { 2: IVec2, 3: IVec3, 4: IVec4 },
    u32 => { 2: UVec2, 3: UVec3, 4: UVec4 },
}

(you'd still have to keep the manual Vec3A impl since that's not bidirectional but that's about it)

|
164 | Self: HasGather,
| ^^^^^^^^^ required by this bound in `Image::<SampledType, DIM, DEPTH, ARRAYED, _, SAMPLED, FORMAT>::gather`
error: internal compiler error: /rustc/0468a00ae3fd6ef1a6a0f9eaf637d7aa9e604acc/compiler/rustc_middle/src/ty/relate.rs:638:13: var types encountered in super_relate_consts: Const { ty: usize, kind: Param(N/#8) } Const { ty: usize, kind: Infer(Var(_#5c)) }
Copy link
Contributor

@eddyb eddyb Jan 11, 2023

Choose a reason for hiding this comment

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

Hmmm, so this seems relevant:

Except, if you look at rust-lang/rust#106240 (comment) - we're not on a nightly that far ahead, so why would this happen sooner?

Talking to the relevant people, those linked PRs are actually not the original cause. Will work on reducing a testcase (EDIT: done: rust-lang/rust#106240 (comment)) but either way the fix is the same:

Which hasn't been merged yet... also it sounds like the bug is in a trick I suggested a few weeks back for taking advantage of inference logic for diagnostic suggestion code.


However, if the problem is needing const N: usize params, we can probably do better by making the ImageCoordinate helper trait mirror the VectorTypeLib associated type from the Vector trait, reducing the need for the extra Vector bounds.

But also, I have even more bad news, that I only noticed while looking for the ImageCoordinate impls:

impl<S: Scalar> ImageCoordinate<S, { Dimensionality::OneD as u32 }, { Arrayed::False as u32 }>
for S
{
}
impl<S: Scalar> ImageCoordinate<S, { Dimensionality::Buffer as u32 }, { Arrayed::False as u32 }>
for S
{
}

C params (before this PR) could be individual scalars, for 1D images. I don't know at this moment how to make both that and the return type stuff, work.

It may require adding a generic parameter for the "vector type lib" itself and setting things up that for vector Cs, ImageCoordinate constraints it (forcing the image op return type to be inferred) while for scalar Cs, it's unconstrained and can somehow be inferred from the image op return type.

@oisyn
Copy link
Contributor Author

oisyn commented Jan 13, 2023

I'm closing this as we're taking a completely different approach to this.

@oisyn oisyn closed this Jan 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants