-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
Implement #[rustc_must_implement_one_of]
attribute
#92164
Changes from 8 commits
5ab40c8
e6bc0ac
268ae9a
96b2f8a
f64daff
4ccfa97
c30ec5a
f9174e1
28edd7a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1198,9 +1198,11 @@ fn super_predicates_that_define_assoc_type( | |
fn trait_def(tcx: TyCtxt<'_>, def_id: DefId) -> ty::TraitDef { | ||
let item = tcx.hir().expect_item(def_id.expect_local()); | ||
|
||
let (is_auto, unsafety) = match item.kind { | ||
hir::ItemKind::Trait(is_auto, unsafety, ..) => (is_auto == hir::IsAuto::Yes, unsafety), | ||
hir::ItemKind::TraitAlias(..) => (false, hir::Unsafety::Normal), | ||
let (is_auto, unsafety, items) = match item.kind { | ||
hir::ItemKind::Trait(is_auto, unsafety, .., items) => { | ||
(is_auto == hir::IsAuto::Yes, unsafety, items) | ||
} | ||
hir::ItemKind::TraitAlias(..) => (false, hir::Unsafety::Normal, &[][..]), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this can just be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Very disappointing; minimal repro: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=875b8d707eb70e293fc06d51eb6f26e0 — a type annotation at line 1201 does fix it, but without that a "wide pointer" field on one branch is not sufficient for a coercion to happen on other branches. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually, even a one element tuple is enough to demonstrate this: (play) |
||
_ => span_bug!(item.span, "trait_def_of_item invoked on non-trait"), | ||
}; | ||
|
||
|
@@ -1227,6 +1229,108 @@ fn trait_def(tcx: TyCtxt<'_>, def_id: DefId) -> ty::TraitDef { | |
ty::trait_def::TraitSpecializationKind::None | ||
}; | ||
let def_path_hash = tcx.def_path_hash(def_id); | ||
|
||
let must_implement_one_of = tcx | ||
.get_attrs(def_id) | ||
.iter() | ||
.find(|attr| attr.has_name(sym::rustc_must_implement_one_of)) | ||
// Check that there are at least 2 arguments of `#[rustc_must_implement_one_of]` | ||
// and that they are all identifiers | ||
.and_then(|attr| match attr.meta_item_list() { | ||
Some(items) if items.len() < 2 => { | ||
tcx.sess | ||
.struct_span_err( | ||
attr.span, | ||
"the `#[rustc_must_implement_one_of]` attribute must be \ | ||
used with at least 2 args", | ||
) | ||
.emit(); | ||
|
||
None | ||
} | ||
Some(items) => items | ||
.into_iter() | ||
.map(|item| item.ident().ok_or(item.span())) | ||
.collect::<Result<Box<[_]>, _>>() | ||
.map_err(|span| { | ||
tcx.sess | ||
.struct_span_err(span, "must be a name of an associated function") | ||
.emit(); | ||
}) | ||
.ok() | ||
.zip(Some(attr.span)), | ||
// Error is reported by `rustc_attr!` | ||
None => None, | ||
}) | ||
// Check that all arguments of `#[rustc_must_implement_one_of]` reference | ||
// functions in the trait with default implementations | ||
.and_then(|(list, attr_span)| { | ||
let errors = list.iter().filter_map(|ident| { | ||
let item = items.iter().find(|item| item.ident == *ident); | ||
|
||
match item { | ||
Some(item) if matches!(item.kind, hir::AssocItemKind::Fn { .. }) => { | ||
if !item.defaultness.has_value() { | ||
tcx.sess | ||
.struct_span_err( | ||
item.span, | ||
"This function doesn't have a default implementation", | ||
) | ||
.span_note(attr_span, "required by this annotation") | ||
.emit(); | ||
|
||
return Some(()); | ||
} | ||
|
||
return None; | ||
} | ||
WaffleLapkin marked this conversation as resolved.
Show resolved
Hide resolved
|
||
Some(item) => tcx | ||
.sess | ||
.struct_span_err(item.span, "Not a function") | ||
.span_note(attr_span, "required by this annotation") | ||
.note( | ||
"All `#[rustc_must_implement_one_of]` arguments \ | ||
must be associated function names", | ||
) | ||
.emit(), | ||
None => tcx | ||
.sess | ||
.struct_span_err(ident.span, "Function not found in this trait") | ||
.emit(), | ||
} | ||
|
||
Some(()) | ||
}); | ||
|
||
(errors.count() == 0).then_some(list) | ||
}) | ||
// Check for duplicates | ||
.and_then(|list| { | ||
let mut set: FxHashSet<&Ident> = FxHashSet::default(); | ||
let mut no_dups = true; | ||
|
||
for ident in &*list { | ||
if let Some(dup) = set.replace(ident) { | ||
WaffleLapkin marked this conversation as resolved.
Show resolved
Hide resolved
|
||
let dup2 = set.get(&dup).copied().unwrap(); // We've just inserted it | ||
|
||
tcx.sess | ||
.struct_span_err( | ||
vec![dup.span, dup2.span], | ||
WaffleLapkin marked this conversation as resolved.
Show resolved
Hide resolved
|
||
"Functions names are duplicated", | ||
) | ||
.note( | ||
"All `#[rustc_must_implement_one_of]` arguments \ | ||
must be unique", | ||
) | ||
.emit(); | ||
|
||
no_dups = false; | ||
} | ||
} | ||
|
||
no_dups.then_some(list) | ||
}); | ||
|
||
ty::TraitDef::new( | ||
def_id, | ||
unsafety, | ||
|
@@ -1236,6 +1340,7 @@ fn trait_def(tcx: TyCtxt<'_>, def_id: DefId) -> ty::TraitDef { | |
skip_array_during_method_dispatch, | ||
spec_kind, | ||
def_path_hash, | ||
must_implement_one_of, | ||
) | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,44 @@ | ||
#![feature(rustc_attrs)] | ||
|
||
#[rustc_must_implement_one_of(eq, neq)] | ||
trait Equal { | ||
fn eq(&self, other: &Self) -> bool { | ||
!self.neq(other) | ||
} | ||
|
||
fn neq(&self, other: &Self) -> bool { | ||
!self.eq(other) | ||
} | ||
} | ||
|
||
struct T0; | ||
struct T1; | ||
struct T2; | ||
struct T3; | ||
|
||
impl Equal for T0 { | ||
fn eq(&self, _other: &Self) -> bool { | ||
true | ||
} | ||
} | ||
|
||
impl Equal for T1 { | ||
fn neq(&self, _other: &Self) -> bool { | ||
false | ||
} | ||
} | ||
|
||
impl Equal for T2 { | ||
fn eq(&self, _other: &Self) -> bool { | ||
true | ||
} | ||
|
||
fn neq(&self, _other: &Self) -> bool { | ||
false | ||
} | ||
} | ||
|
||
impl Equal for T3 {} | ||
//~^ not all trait items implemented, missing one of: `eq`, `neq` | ||
|
||
fn main() {} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,15 @@ | ||
error[E0046]: not all trait items implemented, missing one of: `eq`, `neq` | ||
--> $DIR/rustc_must_implement_one_of.rs:41:1 | ||
| | ||
LL | impl Equal for T3 {} | ||
| ^^^^^^^^^^^^^^^^^ missing one of `eq`, `neq` in implementation | ||
| | ||
note: required because of this annotation | ||
--> $DIR/rustc_must_implement_one_of.rs:3:1 | ||
| | ||
LL | #[rustc_must_implement_one_of(eq, neq)] | ||
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | ||
|
||
error: aborting due to previous error | ||
|
||
For more information about this error, try `rustc --explain E0046`. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,19 @@ | ||
#![feature(rustc_attrs)] | ||
|
||
#[rustc_must_implement_one_of(a, a)] | ||
//~^ Functions names are duplicated | ||
trait Trait { | ||
fn a() {} | ||
} | ||
|
||
#[rustc_must_implement_one_of(b, a, a, c, b, c)] | ||
//~^ Functions names are duplicated | ||
//~| Functions names are duplicated | ||
//~| Functions names are duplicated | ||
trait Trait1 { | ||
fn a() {} | ||
fn b() {} | ||
fn c() {} | ||
} | ||
|
||
fn main() {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Speaking of which: how does this interact with specialization / partial impls?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This example doesn't compile even without
rustc_must_implement_one_of
though...There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Argh, I hate
default impl
not meaningpartial impl
butpartial impl { default members }
(not only is it redundant w.r.t. todefault fn
being usable inside adefault impl
, examples such as this one showcase that's it's also too restrictive, and thus, unnecessarily restrictive).Okay, replace
default impl Trait for ()
withdefault impl<T> Trait for T { fn foo() {} }
.And let's also test with
default impl Trait for T {}
🙏There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So
default impl<T> Trait for T { fn foo() {} }
&impl Trait for () {}
compilesdefault impl<T> Trait for T {}
errorsI'm honestly very confused about what
default
impls mean, so not sure if this is correct 😅Snippets
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for having run the tests (I don't have access to a computer with a
rust/
repo setup today)My view on the matter
So,
default impl … { items… }
meanspartial impl … { default items… }
, as in one is then writing part of a trait (from 0 to all associated items), but for the trait itself. And I personally seepartial impl
, alone, as sugar for super-traits.Let's take
Any
upcasting as an example:Problem: implementors of
SubAny
would need to feature an impl ofas_any
.Problem:
Self : Sized
does not hold.Problem:
&dyn SubAny
can't useas_any
.This finally works, but is cumbersome, plus the
AsAny
trait does not read as well in the docs to let the user know of a.as_any()
method available to it.With
partial impl
s, however, we can both feature a method with no trait-def impl / body (where we couldn't because of lack ofSized
) and yet not force implementors to provide, each time, the same body again and again:Now, from here, your challenge would be to somehow make it if you are inside a
default impl
, not error there (since even if some methods are implemented, the trait itself isn't and thus, its methods aren't callable, so there is no risk of infinite recursion of, e.g.,eq
callingne
callingeq
, etc.).Given the body of the function, it may be as simple as disabling your check if
tcx.impl_defaultness(impl_id).is_final()
does not hold, but take that suggestion with a big grain of salt 😅There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally, I think it's fine to address any issues with
default
in a follow-up PR. Specialization is a long ways from stabilization, so I don't think it needs to block this PR