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

Deref and DerefMut cause ambiguity issues when resolving #3032

Closed
CohenArthur opened this issue May 22, 2024 · 4 comments · Fixed by #3214
Closed

Deref and DerefMut cause ambiguity issues when resolving #3032

CohenArthur opened this issue May 22, 2024 · 4 comments · Fixed by #3214

Comments

@CohenArthur
Copy link
Member

This code is the base implementation of Deref and DerefMut:

#[lang = "sized"]
trait Sized {}

#[lang = "deref"]
pub trait Deref {
    /// The resulting type after dereferencing.
    #[stable(feature = "rust1", since = "1.0.0")]
    // #[rustc_diagnostic_item = "deref_target"]
    type Target: ?Sized;

    /// Dereferences the value.
    #[must_use]
    #[stable(feature = "rust1", since = "1.0.0")]
    // #[rustc_diagnostic_item = "deref_method"]
    fn deref(&self) -> &Self::Target;
}

impl<T: ?Sized> Deref for &T {
    type Target = T;

    fn deref(&self) -> &T {
        *self
    }
}

// this is added because of #3030 
extern "C" {
    fn never() -> !;
}

impl<T: ?Sized> !DerefMut for &T {
    fn deref_mut(&mut self) -> &mut T {
        unsafe { never() }
    }
}

impl<T: ?Sized> Deref for &mut T {
    type Target = T;

    fn deref(&self) -> &T {
        *self
    }
}

#[lang = "deref_mut"]
pub trait DerefMut: Deref {
    /// Mutably dereferences the value.
    #[stable(feature = "rust1", since = "1.0.0")]
    fn deref_mut(&mut self) -> &mut Self::Target;
}

impl<T: ?Sized> DerefMut for &mut T {
    fn deref_mut(&mut self) -> &mut T {
        *self
    }
}

If we remove the #[lang = "deref{_mut}"] attributes, then the code works, but otherwise it errors out:

arthur@platypus ~/G/r/gccrs (master)> build/gcc/crab1 test.rs
test.rs:5:5: error: ambiguous type bound for trait Deref and type &mut T
    5 | pub trait Deref {
      |     ^~~~~
......
   51 | impl<T: ?Sized> DerefMut for &mut T {
      |                              ~
test.rs:52:39: error: mismatched types, expected ‘&mut T’ but got ‘<placeholder:>’ [E0308]
   52 |     fn deref_mut(&mut self) -> &mut T {
      |     ~~                         ~      ^

the reason is that there is an ambiguity and that the compiler finds two associated trait impls: Deref for &mut T and Deref for &T

@CohenArthur
Copy link
Member Author

the issue only arises upon adding this implementation:

impl<T: ?Sized> DerefMut for &mut T {
    fn deref_mut(&mut self) -> &mut T {
        *self
    }
}

which is part of the core library but isn't present in our existing testcases, as they focused on Deref and its behavior

@CohenArthur
Copy link
Member Author

we're also running into this with this code example - I suspect the fix would be the same and there is something wrong when it comes to references/pointers and mutability:

#![feature(intrinsics)]

#[lang = "sized"]
trait Sized {}

pub struct VaListImpl<'f>;

mod sealed_trait {
    pub trait VaArgSafe {}
}

impl<T> sealed_trait::VaArgSafe for *mut T {}
impl<T> sealed_trait::VaArgSafe for *const T {}

impl<'f> VaListImpl<'f> {
    /// Advance to the next arg.
    #[inline]
    pub unsafe fn arg<T: sealed_trait::VaArgSafe>(&mut self) {
        va_arg2(self);
    }
}

fn va_arg2<T: sealed_trait::VaArgSafe>(ap: &mut VaListImpl<'_>) {}

@CohenArthur
Copy link
Member Author

I don't even understand how it makes sense for the last example - the type bound is unused, so it shouldn't affect anything

@philberty philberty self-assigned this Sep 26, 2024
philberty added a commit that referenced this issue Oct 24, 2024
When we are typechecking the impl block for DerefMut for &mut T
the implementation follows the usual operator overload check
but this ended up just resolving directly to the Trait definition
which ends up being recursive which we usually handle. The issue
we had is that a dereference call can be for either the DEREF
or DEREF_MUT lang item here it was looking for a recurisve call
to the DEREF lang item but we were in the DEREF_MUT lang item
so this case was not accounted for.

Fixes #3032

gcc/rust/ChangeLog:

	* typecheck/rust-hir-trait-reference.h: new get locus helper
	* typecheck/rust-hir-trait-resolve.cc (AssociatedImplTrait::get_locus): implemention
	* typecheck/rust-hir-type-check-expr.cc (TypeCheckExpr::resolve_operator_overload): fix overload

gcc/testsuite/ChangeLog:

	* rust/compile/nr2/exclude:
	* rust/compile/issue-3032-1.rs: New test.
	* rust/compile/issue-3032-2.rs: New test.

Signed-off-by: Philip Herron <[email protected]>
@philberty
Copy link
Member

philberty commented Oct 24, 2024

we're also running into this with this code example - I suspect the fix would be the same and there is something wrong when it comes to references/pointers and mutability:

#![feature(intrinsics)]

#[lang = "sized"]
trait Sized {}

pub struct VaListImpl<'f>;

mod sealed_trait {
    pub trait VaArgSafe {}
}

impl<T> sealed_trait::VaArgSafe for *mut T {}
impl<T> sealed_trait::VaArgSafe for *const T {}

impl<'f> VaListImpl<'f> {
    /// Advance to the next arg.
    #[inline]
    pub unsafe fn arg<T: sealed_trait::VaArgSafe>(&mut self) {
        va_arg2(self);
    }
}

fn va_arg2<T: sealed_trait::VaArgSafe>(ap: &mut VaListImpl<'_>) {}

This is a seperate issue i am going to open this into a seperate issue as the original issue is different and fix is up now

philberty added a commit that referenced this issue Oct 24, 2024
When we are typechecking the impl block for DerefMut for &mut T
the implementation follows the usual operator overload check
but this ended up just resolving directly to the Trait definition
which ends up being recursive which we usually handle. The issue
we had is that a dereference call can be for either the DEREF
or DEREF_MUT lang item here it was looking for a recurisve call
to the DEREF lang item but we were in the DEREF_MUT lang item
so this case was not accounted for.

Fixes #3032

gcc/rust/ChangeLog:

	* typecheck/rust-hir-trait-reference.h: new get locus helper
	* typecheck/rust-hir-trait-resolve.cc (AssociatedImplTrait::get_locus): implemention
	* typecheck/rust-hir-type-check-expr.cc (TypeCheckExpr::resolve_operator_overload): fix overload

gcc/testsuite/ChangeLog:

	* rust/compile/nr2/exclude: nr2 cant handle this
	* rust/compile/issue-3032-1.rs: New test.
	* rust/compile/issue-3032-2.rs: New test.

Signed-off-by: Philip Herron <[email protected]>
philberty added a commit that referenced this issue Oct 24, 2024
When we are typechecking the impl block for DerefMut for &mut T
the implementation follows the usual operator overload check
but this ended up just resolving directly to the Trait definition
which ends up being recursive which we usually handle. The issue
we had is that a dereference call can be for either the DEREF
or DEREF_MUT lang item here it was looking for a recurisve call
to the DEREF lang item but we were in the DEREF_MUT lang item
so this case was not accounted for.

Fixes #3032

gcc/rust/ChangeLog:

	* typecheck/rust-hir-trait-reference.h: new get locus helper
	* typecheck/rust-hir-trait-resolve.cc (AssociatedImplTrait::get_locus): implemention
	* typecheck/rust-hir-type-check-expr.cc (TypeCheckExpr::resolve_operator_overload): fix overload

gcc/testsuite/ChangeLog:

	* rust/compile/nr2/exclude: nr2 cant handle this
	* rust/compile/issue-3032-1.rs: New test.
	* rust/compile/issue-3032-2.rs: New test.

Signed-off-by: Philip Herron <[email protected]>
philberty added a commit that referenced this issue Oct 24, 2024
When we are typechecking the impl block for DerefMut for &mut T
the implementation follows the usual operator overload check
but this ended up just resolving directly to the Trait definition
which ends up being recursive which we usually handle. The issue
we had is that a dereference call can be for either the DEREF
or DEREF_MUT lang item here it was looking for a recurisve call
to the DEREF lang item but we were in the DEREF_MUT lang item
so this case was not accounted for.

Fixes #3032

gcc/rust/ChangeLog:

	* typecheck/rust-hir-trait-reference.h: new get locus helper
	* typecheck/rust-hir-trait-resolve.cc (AssociatedImplTrait::get_locus): implemention
	* typecheck/rust-hir-type-check-expr.cc (TypeCheckExpr::resolve_operator_overload):
	fix overload

gcc/testsuite/ChangeLog:

	* rust/compile/nr2/exclude: nr2 cant handle this
	* rust/compile/issue-3032-1.rs: New test.
	* rust/compile/issue-3032-2.rs: New test.

Signed-off-by: Philip Herron <[email protected]>
github-merge-queue bot pushed a commit that referenced this issue Oct 25, 2024
When we are typechecking the impl block for DerefMut for &mut T
the implementation follows the usual operator overload check
but this ended up just resolving directly to the Trait definition
which ends up being recursive which we usually handle. The issue
we had is that a dereference call can be for either the DEREF
or DEREF_MUT lang item here it was looking for a recurisve call
to the DEREF lang item but we were in the DEREF_MUT lang item
so this case was not accounted for.

Fixes #3032

gcc/rust/ChangeLog:

	* typecheck/rust-hir-trait-reference.h: new get locus helper
	* typecheck/rust-hir-trait-resolve.cc (AssociatedImplTrait::get_locus): implemention
	* typecheck/rust-hir-type-check-expr.cc (TypeCheckExpr::resolve_operator_overload):
	fix overload

gcc/testsuite/ChangeLog:

	* rust/compile/nr2/exclude: nr2 cant handle this
	* rust/compile/issue-3032-1.rs: New test.
	* rust/compile/issue-3032-2.rs: New test.

Signed-off-by: Philip Herron <[email protected]>
@github-project-automation github-project-automation bot moved this from Todo to Done in libcore 1.49 Oct 25, 2024
tschwinge pushed a commit that referenced this issue Dec 4, 2024
When we are typechecking the impl block for DerefMut for &mut T
the implementation follows the usual operator overload check
but this ended up just resolving directly to the Trait definition
which ends up being recursive which we usually handle. The issue
we had is that a dereference call can be for either the DEREF
or DEREF_MUT lang item here it was looking for a recurisve call
to the DEREF lang item but we were in the DEREF_MUT lang item
so this case was not accounted for.

Fixes #3032

gcc/rust/ChangeLog:

	* typecheck/rust-hir-trait-reference.h: new get locus helper
	* typecheck/rust-hir-trait-resolve.cc (AssociatedImplTrait::get_locus): implemention
	* typecheck/rust-hir-type-check-expr.cc (TypeCheckExpr::resolve_operator_overload):
	fix overload

gcc/testsuite/ChangeLog:

	* rust/compile/nr2/exclude: nr2 cant handle this
	* rust/compile/issue-3032-1.rs: New test.
	* rust/compile/issue-3032-2.rs: New test.

Signed-off-by: Philip Herron <[email protected]>
Kamiinarii78 pushed a commit to Kamiinarii78/gccrs that referenced this issue Dec 12, 2024
When we are typechecking the impl block for DerefMut for &mut T
the implementation follows the usual operator overload check
but this ended up just resolving directly to the Trait definition
which ends up being recursive which we usually handle. The issue
we had is that a dereference call can be for either the DEREF
or DEREF_MUT lang item here it was looking for a recurisve call
to the DEREF lang item but we were in the DEREF_MUT lang item
so this case was not accounted for.

Fixes Rust-GCC#3032

gcc/rust/ChangeLog:

	* typecheck/rust-hir-trait-reference.h: new get locus helper
	* typecheck/rust-hir-trait-resolve.cc (AssociatedImplTrait::get_locus): implemention
	* typecheck/rust-hir-type-check-expr.cc (TypeCheckExpr::resolve_operator_overload):
	fix overload

gcc/testsuite/ChangeLog:

	* rust/compile/nr2/exclude: nr2 cant handle this
	* rust/compile/issue-3032-1.rs: New test.
	* rust/compile/issue-3032-2.rs: New test.

Signed-off-by: Philip Herron <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

2 participants