Skip to content

Commit 6ddccc9

Browse files
VeykrilVeykril
committed
Diagnose some orphan trait impl cases
1 parent b740155 commit 6ddccc9

File tree

8 files changed

+205
-15
lines changed

8 files changed

+205
-15
lines changed

‎crates/hir-ty/src/diagnostics.rs‎

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -11,9 +11,3 @@ pub use crate::diagnostics::{
1111
},
1212
unsafe_check::{missing_unsafe, unsafe_expressions, UnsafeExpr},
1313
};
14-
15-
#[derive(Debug, PartialEq, Eq)]
16-
pub struct IncoherentImpl {
17-
pub file_id: hir_expand::HirFileId,
18-
pub impl_: syntax::AstPtr<syntax::ast::Impl>,
19-
}

‎crates/hir-ty/src/lib.rs‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -80,6 +80,7 @@ pub use mapping::{
8080
lt_from_placeholder_idx, to_assoc_type_id, to_chalk_trait_id, to_foreign_def_id,
8181
to_placeholder_idx,
8282
};
83+
pub use method_resolution::check_orphan_rules;
8384
pub use traits::TraitEnvironment;
8485
pub use utils::{all_super_traits, is_fn_unsafe_to_call};
8586

‎crates/hir-ty/src/method_resolution.rs‎

Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -862,6 +862,62 @@ fn is_inherent_impl_coherent(
862862
}
863863
}
864864

865+
/// Checks whether the impl satisfies the orphan rules.
866+
///
867+
/// Given `impl<P1..=Pn> Trait<T1..=Tn> for T0`, an `impl`` is valid only if at least one of the following is true:
868+
/// - Trait is a local trait
869+
/// - All of
870+
/// - At least one of the types `T0..=Tn`` must be a local type. Let `Ti`` be the first such type.
871+
/// - No uncovered type parameters `P1..=Pn` may appear in `T0..Ti`` (excluding `Ti`)
872+
pub fn check_orphan_rules(db: &dyn HirDatabase, impl_: ImplId) -> bool {
873+
let substs = TyBuilder::placeholder_subst(db, impl_);
874+
let Some(impl_trait) = db.impl_trait(impl_) else {
875+
// not a trait impl
876+
return true;
877+
};
878+
879+
let local_crate = impl_.lookup(db.upcast()).container.krate();
880+
let is_local = |tgt_crate| tgt_crate == local_crate;
881+
882+
let trait_ref = impl_trait.substitute(Interner, &substs);
883+
let trait_id = from_chalk_trait_id(trait_ref.trait_id);
884+
if is_local(trait_id.module(db.upcast()).krate()) {
885+
// trait to be implemented is local
886+
return true;
887+
}
888+
889+
let unwrap_fundamental = |ty: Ty| match ty.kind(Interner) {
890+
TyKind::Ref(_, _, referenced) => referenced.clone(),
891+
&TyKind::Adt(AdtId(hir_def::AdtId::StructId(s)), ref subs) => {
892+
let struct_data = db.struct_data(s);
893+
if struct_data.flags.contains(StructFlags::IS_FUNDAMENTAL) {
894+
let next = subs.type_parameters(Interner).next();
895+
match next {
896+
Some(ty) => ty,
897+
None => ty,
898+
}
899+
} else {
900+
ty
901+
}
902+
}
903+
_ => ty,
904+
};
905+
// - At least one of the types `T0..=Tn`` must be a local type. Let `Ti`` be the first such type.
906+
let is_not_orphan = trait_ref.substitution.type_parameters(Interner).any(|ty| {
907+
match unwrap_fundamental(ty).kind(Interner) {
908+
&TyKind::Adt(AdtId(id), _) => is_local(id.module(db.upcast()).krate()),
909+
TyKind::Error => true,
910+
TyKind::Dyn(it) => it.principal().map_or(false, |trait_ref| {
911+
is_local(from_chalk_trait_id(trait_ref.trait_id).module(db.upcast()).krate())
912+
}),
913+
_ => false,
914+
}
915+
});
916+
// FIXME: param coverage
917+
// - No uncovered type parameters `P1..=Pn` may appear in `T0..Ti`` (excluding `Ti`)
918+
is_not_orphan
919+
}
920+
865921
pub fn iterate_path_candidates(
866922
ty: &Canonical<Ty>,
867923
db: &dyn HirDatabase,

‎crates/hir/src/diagnostics.rs‎

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@
33
//!
44
//! This probably isn't the best way to do this -- ideally, diagnostics should
55
//! be expressed in terms of hir types themselves.
6-
pub use hir_ty::diagnostics::{CaseType, IncoherentImpl, IncorrectCase};
6+
pub use hir_ty::diagnostics::{CaseType, IncorrectCase};
77

88
use base_db::CrateId;
99
use cfg::{CfgExpr, CfgOptions};
@@ -38,6 +38,7 @@ diagnostics![
3838
IncorrectCase,
3939
InvalidDeriveTarget,
4040
IncoherentImpl,
41+
TraitImplOrphan,
4142
MacroDefError,
4243
MacroError,
4344
MacroExpansionParseError,
@@ -280,3 +281,15 @@ pub struct MovedOutOfRef {
280281
pub ty: Type,
281282
pub span: InFile<SyntaxNodePtr>,
282283
}
284+
285+
#[derive(Debug, PartialEq, Eq)]
286+
pub struct IncoherentImpl {
287+
pub file_id: HirFileId,
288+
pub impl_: AstPtr<ast::Impl>,
289+
}
290+
291+
#[derive(Debug, PartialEq, Eq)]
292+
pub struct TraitImplOrphan {
293+
pub file_id: HirFileId,
294+
pub impl_: AstPtr<ast::Impl>,
295+
}

‎crates/hir/src/lib.rs‎

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -60,7 +60,7 @@ use hir_def::{
6060
};
6161
use hir_expand::{name::name, MacroCallKind};
6262
use hir_ty::{
63-
all_super_traits, autoderef,
63+
all_super_traits, autoderef, check_orphan_rules,
6464
consteval::{try_const_usize, unknown_const_as_generic, ConstEvalError, ConstExt},
6565
diagnostics::BodyValidationDiagnostic,
6666
known_const_to_ast,
@@ -95,7 +95,7 @@ pub use crate::{
9595
MacroExpansionParseError, MalformedDerive, MismatchedArgCount,
9696
MismatchedTupleStructPatArgCount, MissingFields, MissingMatchArms, MissingUnsafe,
9797
MovedOutOfRef, NeedMut, NoSuchField, PrivateAssocItem, PrivateField,
98-
ReplaceFilterMapNextWithFindMap, TypeMismatch, TypedHole, UndeclaredLabel,
98+
ReplaceFilterMapNextWithFindMap, TraitImplOrphan, TypeMismatch, TypedHole, UndeclaredLabel,
9999
UnimplementedBuiltinMacro, UnreachableLabel, UnresolvedExternCrate, UnresolvedField,
100100
UnresolvedImport, UnresolvedMacroCall, UnresolvedMethodCall, UnresolvedModule,
101101
UnresolvedProcMacro, UnusedMut, UnusedVariable,
@@ -624,6 +624,11 @@ impl Module {
624624
acc.push(IncoherentImpl { impl_: ast_id_map.get(node.ast_id()), file_id }.into())
625625
}
626626

627+
if !impl_def.check_orphan_rules(db) {
628+
let ast_id_map = db.ast_id_map(file_id);
629+
acc.push(TraitImplOrphan { impl_: ast_id_map.get(node.ast_id()), file_id }.into())
630+
}
631+
627632
for item in impl_def.items(db) {
628633
let def: DefWithBody = match item {
629634
AssocItem::Function(it) => it.into(),
@@ -3406,6 +3411,10 @@ impl Impl {
34063411
let src = self.source(db)?;
34073412
src.file_id.as_builtin_derive_attr_node(db.upcast())
34083413
}
3414+
3415+
pub fn check_orphan_rules(self, db: &dyn HirDatabase) -> bool {
3416+
check_orphan_rules(db, self.id)
3417+
}
34093418
}
34103419

34113420
#[derive(Clone, PartialEq, Eq, Debug, Hash)]

‎crates/ide-diagnostics/src/handlers/trait_impl_orphan.rs‎

Lines changed: 106 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,106 @@
1+
use hir::InFile;
2+
3+
use crate::{Diagnostic, DiagnosticCode, DiagnosticsContext};
4+
5+
// Diagnostic: trait-impl-orphan
6+
//
7+
// Only traits defined in the current crate can be implemented for arbitrary types
8+
pub(crate) fn trait_impl_orphan(
9+
ctx: &DiagnosticsContext<'_>,
10+
d: &hir::TraitImplOrphan,
11+
) -> Diagnostic {
12+
Diagnostic::new_with_syntax_node_ptr(
13+
ctx,
14+
DiagnosticCode::RustcHardError("E0117"),
15+
format!("only traits defined in the current crate can be implemented for arbitrary types"),
16+
InFile::new(d.file_id, d.impl_.clone().into()),
17+
)
18+
// Not yet checked for false positives
19+
.experimental()
20+
}
21+
22+
#[cfg(test)]
23+
mod tests {
24+
use crate::tests::check_diagnostics;
25+
26+
#[test]
27+
fn simple() {
28+
check_diagnostics(
29+
r#"
30+
//- /foo.rs crate:foo
31+
pub trait Foo {}
32+
//- /bar.rs crate:bar
33+
pub struct Bar;
34+
//- /main.rs crate:main deps:foo,bar
35+
struct LocalType;
36+
trait LocalTrait {}
37+
impl foo::Foo for bar::Bar {}
38+
//^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ error: only traits defined in the current crate can be implemented for arbitrary types
39+
impl foo::Foo for LocalType {}
40+
impl LocalTrait for bar::Bar {}
41+
"#,
42+
);
43+
}
44+
45+
#[test]
46+
fn generics() {
47+
check_diagnostics(
48+
r#"
49+
//- /foo.rs crate:foo
50+
pub trait Foo<T> {}
51+
//- /bar.rs crate:bar
52+
pub struct Bar<T>(T);
53+
//- /main.rs crate:main deps:foo,bar
54+
struct LocalType<T>;
55+
trait LocalTrait<T> {}
56+
impl<T> foo::Foo<T> for bar::Bar<T> {}
57+
//^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ error: only traits defined in the current crate can be implemented for arbitrary types
58+
59+
impl<T> foo::Foo<T> for bar::Bar<LocalType<T>> {}
60+
//^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ error: only traits defined in the current crate can be implemented for arbitrary types
61+
62+
impl<T> foo::Foo<LocalType<T>> for bar::Bar<T> {}
63+
64+
impl<T> foo::Foo<bar::Bar<LocalType<T>>> for bar::Bar<LocalType<T>> {}
65+
//^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ error: only traits defined in the current crate can be implemented for arbitrary types
66+
"#,
67+
);
68+
}
69+
70+
#[test]
71+
fn fundamental() {
72+
check_diagnostics(
73+
r#"
74+
//- /foo.rs crate:foo
75+
pub trait Foo<T> {}
76+
//- /bar.rs crate:bar
77+
pub struct Bar<T>(T);
78+
#[lang = "owned_box"]
79+
#[fundamental]
80+
pub struct Box<T>(T);
81+
//- /main.rs crate:main deps:foo,bar
82+
struct LocalType;
83+
impl<T> foo::Foo<T> for bar::Box<T> {}
84+
//^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ error: only traits defined in the current crate can be implemented for arbitrary types
85+
impl<T> foo::Foo<T> for &LocalType {}
86+
impl<T> foo::Foo<T> for bar::Box<LocalType> {}
87+
"#,
88+
);
89+
}
90+
91+
#[test]
92+
fn dyn_object() {
93+
check_diagnostics(
94+
r#"
95+
//- /foo.rs crate:foo
96+
pub trait Foo<T> {}
97+
//- /bar.rs crate:bar
98+
pub struct Bar;
99+
//- /main.rs crate:main deps:foo,bar
100+
trait LocalTrait {}
101+
impl<T> foo::Foo<T> for dyn LocalTrait {}
102+
impl<T> foo::Foo<dyn LocalTrait> for Bar {}
103+
"#,
104+
);
105+
}
106+
}

‎crates/ide-diagnostics/src/lib.rs‎

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,7 @@ mod handlers {
4444
pub(crate) mod private_assoc_item;
4545
pub(crate) mod private_field;
4646
pub(crate) mod replace_filter_map_next_with_find_map;
47+
pub(crate) mod trait_impl_orphan;
4748
pub(crate) mod typed_hole;
4849
pub(crate) mod type_mismatch;
4950
pub(crate) mod unimplemented_builtin_macro;
@@ -301,9 +302,9 @@ pub fn diagnostics(
301302
)
302303
}));
303304

304-
let parse = parse.syntax_node();
305+
let parse = sema.parse(file_id);
305306

306-
for node in parse.descendants() {
307+
for node in parse.syntax().descendants() {
307308
handlers::useless_braces::useless_braces(&mut res, file_id, &node);
308309
handlers::field_shorthand::field_shorthand(&mut res, file_id, &node);
309310
handlers::json_is_not_rust::json_in_items(&sema, &mut res, file_id, &node, config);
@@ -358,6 +359,7 @@ pub fn diagnostics(
358359
AnyDiagnostic::PrivateAssocItem(d) => handlers::private_assoc_item::private_assoc_item(&ctx, &d),
359360
AnyDiagnostic::PrivateField(d) => handlers::private_field::private_field(&ctx, &d),
360361
AnyDiagnostic::ReplaceFilterMapNextWithFindMap(d) => handlers::replace_filter_map_next_with_find_map::replace_filter_map_next_with_find_map(&ctx, &d),
362+
AnyDiagnostic::TraitImplOrphan(d) => handlers::trait_impl_orphan::trait_impl_orphan(&ctx, &d),
361363
AnyDiagnostic::TypedHole(d) => handlers::typed_hole::typed_hole(&ctx, &d),
362364
AnyDiagnostic::TypeMismatch(d) => handlers::type_mismatch::type_mismatch(&ctx, &d),
363365
AnyDiagnostic::UndeclaredLabel(d) => handlers::undeclared_label::undeclared_label(&ctx, &d),
@@ -386,7 +388,7 @@ pub fn diagnostics(
386388

387389
handle_lint_attributes(
388390
&ctx.sema,
389-
&parse,
391+
parse.syntax(),
390392
&mut rustc_stack,
391393
&mut clippy_stack,
392394
&mut diagnostics_of_range,

‎crates/ide-diagnostics/src/tests.rs‎

Lines changed: 12 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@ use expect_test::Expect;
55
use ide_db::{
66
assists::AssistResolveStrategy,
77
base_db::{fixture::WithFixture, SourceDatabaseExt},
8-
RootDatabase,
8+
LineIndexDatabase, RootDatabase,
99
};
1010
use stdx::trim_indent;
1111
use test_utils::{assert_eq_text, extract_annotations, MiniCore};
@@ -103,6 +103,7 @@ pub(crate) fn check_diagnostics(ra_fixture: &str) {
103103
pub(crate) fn check_diagnostics_with_config(config: DiagnosticsConfig, ra_fixture: &str) {
104104
let (db, files) = RootDatabase::with_many_files(ra_fixture);
105105
for file_id in files {
106+
let line_index = db.line_index(file_id);
106107
let diagnostics = super::diagnostics(&db, &config, &AssistResolveStrategy::All, file_id);
107108

108109
let expected = extract_annotations(&db.file_text(file_id));
@@ -136,8 +137,16 @@ pub(crate) fn check_diagnostics_with_config(config: DiagnosticsConfig, ra_fixtur
136137
}
137138
}
138139
if expected != actual {
139-
let fneg = expected.iter().filter(|x| !actual.contains(x)).collect::<Vec<_>>();
140-
let fpos = actual.iter().filter(|x| !expected.contains(x)).collect::<Vec<_>>();
140+
let fneg = expected
141+
.iter()
142+
.filter(|x| !actual.contains(x))
143+
.map(|(range, s)| (line_index.line_col(range.start()), range, s))
144+
.collect::<Vec<_>>();
145+
let fpos = actual
146+
.iter()
147+
.filter(|x| !expected.contains(x))
148+
.map(|(range, s)| (line_index.line_col(range.start()), range, s))
149+
.collect::<Vec<_>>();
141150

142151
panic!("Diagnostic test failed.\nFalse negatives: {fneg:?}\nFalse positives: {fpos:?}");
143152
}

0 commit comments

Comments
 (0)