Diagnose some orphan trait impl cases

Author: Veykril

crates/hir-ty/src/diagnostics.rs

@@ -11,9 +11,3 @@ pub use crate::diagnostics::{
     },
     unsafe_check::{missing_unsafe, unsafe_expressions, UnsafeExpr},
 };
-
-#[derive(Debug, PartialEq, Eq)]
-pub struct IncoherentImpl {
-    pub file_id: hir_expand::HirFileId,
-    pub impl_: syntax::AstPtr<syntax::ast::Impl>,
-}

crates/hir-ty/src/lib.rs

@@ -80,6 +80,7 @@ pub use mapping::{
     lt_from_placeholder_idx, to_assoc_type_id, to_chalk_trait_id, to_foreign_def_id,
     to_placeholder_idx,
 };
+pub use method_resolution::check_orphan_rules;
 pub use traits::TraitEnvironment;
 pub use utils::{all_super_traits, is_fn_unsafe_to_call};
 

crates/hir-ty/src/method_resolution.rs

@@ -862,6 +862,62 @@ fn is_inherent_impl_coherent(
     }
 }
 
+/// Checks whether the impl satisfies the orphan rules.
+///
+/// Given `impl<P1..=Pn> Trait<T1..=Tn> for T0`, an `impl`` is valid only if at least one of the following is true:
+/// - Trait is a local trait
+/// - All of
+///   - At least one of the types `T0..=Tn`` must be a local type. Let `Ti`` be the first such type.
+///   - No uncovered type parameters `P1..=Pn` may appear in `T0..Ti`` (excluding `Ti`)
+pub fn check_orphan_rules(db: &dyn HirDatabase, impl_: ImplId) -> bool {
+    let substs = TyBuilder::placeholder_subst(db, impl_);
+    let Some(impl_trait) = db.impl_trait(impl_) else {
+        // not a trait impl
+        return true;
+    };
+
+    let local_crate = impl_.lookup(db.upcast()).container.krate();
+    let is_local = |tgt_crate| tgt_crate == local_crate;
+
+    let trait_ref = impl_trait.substitute(Interner, &substs);
+    let trait_id = from_chalk_trait_id(trait_ref.trait_id);
+    if is_local(trait_id.module(db.upcast()).krate()) {
+        // trait to be implemented is local
+        return true;
+    }
+
+    let unwrap_fundamental = |ty: Ty| match ty.kind(Interner) {
+        TyKind::Ref(_, _, referenced) => referenced.clone(),
+        &TyKind::Adt(AdtId(hir_def::AdtId::StructId(s)), ref subs) => {
+            let struct_data = db.struct_data(s);
+            if struct_data.flags.contains(StructFlags::IS_FUNDAMENTAL) {
+                let next = subs.type_parameters(Interner).next();
+                match next {
+                    Some(ty) => ty,
+                    None => ty,
+                }
+            } else {
+                ty
+            }
+        }
+        _ => ty,
+    };
+    //   - At least one of the types `T0..=Tn`` must be a local type. Let `Ti`` be the first such type.
+    let is_not_orphan = trait_ref.substitution.type_parameters(Interner).any(|ty| {
+        match unwrap_fundamental(ty).kind(Interner) {
+            &TyKind::Adt(AdtId(id), _) => is_local(id.module(db.upcast()).krate()),
+            TyKind::Error => true,
+            TyKind::Dyn(it) => it.principal().map_or(false, |trait_ref| {
+                is_local(from_chalk_trait_id(trait_ref.trait_id).module(db.upcast()).krate())
+            }),
+            _ => false,
+        }
+    });
+    // FIXME: param coverage
+    //   - No uncovered type parameters `P1..=Pn` may appear in `T0..Ti`` (excluding `Ti`)
+    is_not_orphan
+}
+
 pub fn iterate_path_candidates(
     ty: &Canonical<Ty>,
     db: &dyn HirDatabase,

crates/hir/src/diagnostics.rs

@@ -3,7 +3,7 @@
 //!
 //! This probably isn't the best way to do this -- ideally, diagnostics should
 //! be expressed in terms of hir types themselves.
-pub use hir_ty::diagnostics::{CaseType, IncoherentImpl, IncorrectCase};
+pub use hir_ty::diagnostics::{CaseType, IncorrectCase};
 
 use base_db::CrateId;
 use cfg::{CfgExpr, CfgOptions};
@@ -38,6 +38,7 @@ diagnostics![
     IncorrectCase,
     InvalidDeriveTarget,
     IncoherentImpl,
+    TraitImplOrphan,
     MacroDefError,
     MacroError,
     MacroExpansionParseError,
@@ -280,3 +281,15 @@ pub struct MovedOutOfRef {
     pub ty: Type,
     pub span: InFile<SyntaxNodePtr>,
 }
+
+#[derive(Debug, PartialEq, Eq)]
+pub struct IncoherentImpl {
+    pub file_id: HirFileId,
+    pub impl_: AstPtr<ast::Impl>,
+}
+
+#[derive(Debug, PartialEq, Eq)]
+pub struct TraitImplOrphan {
+    pub file_id: HirFileId,
+    pub impl_: AstPtr<ast::Impl>,
+}

crates/hir/src/lib.rs

@@ -60,7 +60,7 @@ use hir_def::{
 };
 use hir_expand::{name::name, MacroCallKind};
 use hir_ty::{
-    all_super_traits, autoderef,
+    all_super_traits, autoderef, check_orphan_rules,
     consteval::{try_const_usize, unknown_const_as_generic, ConstEvalError, ConstExt},
     diagnostics::BodyValidationDiagnostic,
     known_const_to_ast,
@@ -95,7 +95,7 @@ pub use crate::{
         MacroExpansionParseError, MalformedDerive, MismatchedArgCount,
         MismatchedTupleStructPatArgCount, MissingFields, MissingMatchArms, MissingUnsafe,
         MovedOutOfRef, NeedMut, NoSuchField, PrivateAssocItem, PrivateField,
-        ReplaceFilterMapNextWithFindMap, TypeMismatch, TypedHole, UndeclaredLabel,
+        ReplaceFilterMapNextWithFindMap, TraitImplOrphan, TypeMismatch, TypedHole, UndeclaredLabel,
         UnimplementedBuiltinMacro, UnreachableLabel, UnresolvedExternCrate, UnresolvedField,
         UnresolvedImport, UnresolvedMacroCall, UnresolvedMethodCall, UnresolvedModule,
         UnresolvedProcMacro, UnusedMut, UnusedVariable,
@@ -624,6 +624,11 @@ impl Module {
                 acc.push(IncoherentImpl { impl_: ast_id_map.get(node.ast_id()), file_id }.into())
             }
 
+            if !impl_def.check_orphan_rules(db) {
+                let ast_id_map = db.ast_id_map(file_id);
+                acc.push(TraitImplOrphan { impl_: ast_id_map.get(node.ast_id()), file_id }.into())
+            }
+
             for item in impl_def.items(db) {
                 let def: DefWithBody = match item {
                     AssocItem::Function(it) => it.into(),
@@ -3406,6 +3411,10 @@ impl Impl {
         let src = self.source(db)?;
         src.file_id.as_builtin_derive_attr_node(db.upcast())
     }
+
+    pub fn check_orphan_rules(self, db: &dyn HirDatabase) -> bool {
+        check_orphan_rules(db, self.id)
+    }
 }
 
 #[derive(Clone, PartialEq, Eq, Debug, Hash)]

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

@@ -0,0 +1,106 @@
+use hir::InFile;
+
+use crate::{Diagnostic, DiagnosticCode, DiagnosticsContext};
+
+// Diagnostic: trait-impl-orphan
+//
+// Only traits defined in the current crate can be implemented for arbitrary types
+pub(crate) fn trait_impl_orphan(
+    ctx: &DiagnosticsContext<'_>,
+    d: &hir::TraitImplOrphan,
+) -> Diagnostic {
+    Diagnostic::new_with_syntax_node_ptr(
+        ctx,
+        DiagnosticCode::RustcHardError("E0117"),
+        format!("only traits defined in the current crate can be implemented for arbitrary types"),
+        InFile::new(d.file_id, d.impl_.clone().into()),
+    )
+    // Not yet checked for false positives
+    .experimental()
+}
+
+#[cfg(test)]
+mod tests {
+    use crate::tests::check_diagnostics;
+
+    #[test]
+    fn simple() {
+        check_diagnostics(
+            r#"
+//- /foo.rs crate:foo
+pub trait Foo {}
+//- /bar.rs crate:bar
+pub struct Bar;
+//- /main.rs crate:main deps:foo,bar
+struct LocalType;
+trait LocalTrait {}
+  impl foo::Foo for bar::Bar {}
+//^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ error: only traits defined in the current crate can be implemented for arbitrary types
+impl foo::Foo for LocalType {}
+impl LocalTrait for bar::Bar {}
+"#,
+        );
+    }
+
+    #[test]
+    fn generics() {
+        check_diagnostics(
+            r#"
+//- /foo.rs crate:foo
+pub trait Foo<T> {}
+//- /bar.rs crate:bar
+pub struct Bar<T>(T);
+//- /main.rs crate:main deps:foo,bar
+struct LocalType<T>;
+trait LocalTrait<T> {}
+  impl<T> foo::Foo<T> for bar::Bar<T> {}
+//^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ error: only traits defined in the current crate can be implemented for arbitrary types
+
+  impl<T> foo::Foo<T> for bar::Bar<LocalType<T>> {}
+//^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ error: only traits defined in the current crate can be implemented for arbitrary types
+
+  impl<T> foo::Foo<LocalType<T>> for bar::Bar<T> {}
+
+  impl<T> foo::Foo<bar::Bar<LocalType<T>>> for bar::Bar<LocalType<T>> {}
+//^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ error: only traits defined in the current crate can be implemented for arbitrary types
+"#,
+        );
+    }
+
+    #[test]
+    fn fundamental() {
+        check_diagnostics(
+            r#"
+//- /foo.rs crate:foo
+pub trait Foo<T> {}
+//- /bar.rs crate:bar
+pub struct Bar<T>(T);
+#[lang = "owned_box"]
+#[fundamental]
+pub struct Box<T>(T);
+//- /main.rs crate:main deps:foo,bar
+struct LocalType;
+  impl<T> foo::Foo<T> for bar::Box<T> {}
+//^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ error: only traits defined in the current crate can be implemented for arbitrary types
+  impl<T> foo::Foo<T> for &LocalType {}
+  impl<T> foo::Foo<T> for bar::Box<LocalType> {}
+"#,
+        );
+    }
+
+    #[test]
+    fn dyn_object() {
+        check_diagnostics(
+            r#"
+//- /foo.rs crate:foo
+pub trait Foo<T> {}
+//- /bar.rs crate:bar
+pub struct Bar;
+//- /main.rs crate:main deps:foo,bar
+trait LocalTrait {}
+impl<T> foo::Foo<T> for dyn LocalTrait {}
+impl<T> foo::Foo<dyn LocalTrait> for Bar {}
+"#,
+        );
+    }
+}

crates/ide-diagnostics/src/lib.rs

@@ -44,6 +44,7 @@ mod handlers {
     pub(crate) mod private_assoc_item;
     pub(crate) mod private_field;
     pub(crate) mod replace_filter_map_next_with_find_map;
+    pub(crate) mod trait_impl_orphan;
     pub(crate) mod typed_hole;
     pub(crate) mod type_mismatch;
     pub(crate) mod unimplemented_builtin_macro;
@@ -301,9 +302,9 @@ pub fn diagnostics(
         )
     }));
 
-    let parse = parse.syntax_node();
+    let parse = sema.parse(file_id);
 
-    for node in parse.descendants() {
+    for node in parse.syntax().descendants() {
         handlers::useless_braces::useless_braces(&mut res, file_id, &node);
         handlers::field_shorthand::field_shorthand(&mut res, file_id, &node);
         handlers::json_is_not_rust::json_in_items(&sema, &mut res, file_id, &node, config);
@@ -358,6 +359,7 @@ pub fn diagnostics(
             AnyDiagnostic::PrivateAssocItem(d) => handlers::private_assoc_item::private_assoc_item(&ctx, &d),
             AnyDiagnostic::PrivateField(d) => handlers::private_field::private_field(&ctx, &d),
             AnyDiagnostic::ReplaceFilterMapNextWithFindMap(d) => handlers::replace_filter_map_next_with_find_map::replace_filter_map_next_with_find_map(&ctx, &d),
+            AnyDiagnostic::TraitImplOrphan(d) => handlers::trait_impl_orphan::trait_impl_orphan(&ctx, &d),
             AnyDiagnostic::TypedHole(d) => handlers::typed_hole::typed_hole(&ctx, &d),
             AnyDiagnostic::TypeMismatch(d) => handlers::type_mismatch::type_mismatch(&ctx, &d),
             AnyDiagnostic::UndeclaredLabel(d) => handlers::undeclared_label::undeclared_label(&ctx, &d),
@@ -386,7 +388,7 @@ pub fn diagnostics(
 
     handle_lint_attributes(
         &ctx.sema,
-        &parse,
+        parse.syntax(),
         &mut rustc_stack,
         &mut clippy_stack,
         &mut diagnostics_of_range,

crates/ide-diagnostics/src/tests.rs

@@ -5,7 +5,7 @@ use expect_test::Expect;
 use ide_db::{
     assists::AssistResolveStrategy,
     base_db::{fixture::WithFixture, SourceDatabaseExt},
-    RootDatabase,
+    LineIndexDatabase, RootDatabase,
 };
 use stdx::trim_indent;
 use test_utils::{assert_eq_text, extract_annotations, MiniCore};
@@ -103,6 +103,7 @@ pub(crate) fn check_diagnostics(ra_fixture: &str) {
 pub(crate) fn check_diagnostics_with_config(config: DiagnosticsConfig, ra_fixture: &str) {
     let (db, files) = RootDatabase::with_many_files(ra_fixture);
     for file_id in files {
+        let line_index = db.line_index(file_id);
         let diagnostics = super::diagnostics(&db, &config, &AssistResolveStrategy::All, file_id);
 
         let expected = extract_annotations(&db.file_text(file_id));
@@ -136,8 +137,16 @@ pub(crate) fn check_diagnostics_with_config(config: DiagnosticsConfig, ra_fixtur
             }
         }
         if expected != actual {
-            let fneg = expected.iter().filter(|x| !actual.contains(x)).collect::<Vec<_>>();
-            let fpos = actual.iter().filter(|x| !expected.contains(x)).collect::<Vec<_>>();
+            let fneg = expected
+                .iter()
+                .filter(|x| !actual.contains(x))
+                .map(|(range, s)| (line_index.line_col(range.start()), range, s))
+                .collect::<Vec<_>>();
+            let fpos = actual
+                .iter()
+                .filter(|x| !expected.contains(x))
+                .map(|(range, s)| (line_index.line_col(range.start()), range, s))
+                .collect::<Vec<_>>();
 
             panic!("Diagnostic test failed.\nFalse negatives: {fneg:?}\nFalse positives: {fpos:?}");
         }