Trait coherence: disallow inherent impls on external types

This improves the trait coherence checks for impl self case,
and polishes a bit of the wording on the existing diagnostics.

* Add CompileError::InherentImplForExternalType and diagnostics.
  Reason/issue/help now refer to "package" to match coherence scope

* Enforce package-level check in impl-self type checking
  Reject inherent impls for external nominal types (struct/enum)

Temporary whitelist: allow inherent impls on std::storage::StorageKey<_>

This is a workaround so current code that uses this pattern keeps
working.

Author: tritao

sway-core/src/semantic_analysis/ast_node/declaration/impl_trait.rs

@@ -526,6 +526,56 @@ impl TyImplSelfOrTrait {
                     implementing_for.type_id(),
                 )?;
 
+                // Disallow inherent implementations for types defined outside the current package
+                let current_pkg = ctx.namespace().current_package_ref();
+                let is_external = match &*type_engine.get(implementing_for.type_id()) {
+                    TypeInfo::Struct(decl_id) => {
+                        let s = decl_engine.get_struct(decl_id);
+                        let pkg_name = s.call_path.prefixes.first().map(|p| p.as_str());
+                        match pkg_name {
+                            Some(name) => name != current_pkg.name().as_str(),
+                            None => false,
+                        }
+                    }
+                    TypeInfo::Enum(decl_id) => {
+                        let e = decl_engine.get_enum(decl_id);
+                        let pkg_name = e.call_path.prefixes.first().map(|p| p.as_str());
+                        match pkg_name {
+                            Some(name) => name != current_pkg.name().as_str(),
+                            None => false,
+                        }
+                    }
+                    _ => false,
+                };
+
+                // Temporary workaround: allow inherent impls on `std::storage::StorageKey<_>`.
+                let is_storage_key_in_std = match &*type_engine.get(implementing_for.type_id()) {
+                    TypeInfo::Struct(decl_id) => {
+                        let s = decl_engine.get_struct(decl_id);
+                        s.call_path.suffix.as_str() == "StorageKey"
+                            && s.call_path.prefixes.len() >= 2
+                            && s.call_path.prefixes[0].as_str() == "std"
+                            && s.call_path.prefixes[1].as_str() == "storage"
+                    }
+                    _ => false,
+                };
+
+                if is_external && !is_storage_key_in_std {
+                    let type_name = engines.help_out(implementing_for.type_id()).to_string();
+                    let type_definition_span = match &*type_engine.get(implementing_for.type_id()) {
+                        TypeInfo::Struct(decl_id) => {
+                            Some(decl_engine.get_struct(decl_id).span.clone())
+                        }
+                        TypeInfo::Enum(decl_id) => Some(decl_engine.get_enum(decl_id).span.clone()),
+                        _ => None,
+                    };
+                    return Err(handler.emit_err(CompileError::InherentImplForExternalType {
+                        type_name,
+                        span: implementing_for.span(),
+                        type_definition_span,
+                    }));
+                }
+
                 implementing_for.type_id().check_type_parameter_bounds(
                     handler,
                     ctx.by_ref(),

sway-error/src/error.rs

@@ -1085,12 +1085,18 @@ pub enum CompileError {
     },
     #[error("This expression has type \"{argument_type}\", which does not implement \"std::marker::Error\". Panic expression arguments must implement \"Error\".")]
     PanicExpressionArgumentIsNotError { argument_type: String, span: Span },
-    #[error("Coherence violation: only traits defined in this crate can be implemented for external types.")]
+    #[error("Coherence violation: only traits defined in this package can be implemented for external types.")]
     IncoherentImplDueToOrphanRule {
         trait_name: String,
         type_name: String,
         span: Span,
     },
+    #[error("Coherence violation: inherent implementations are not allowed for external types.")]
+    InherentImplForExternalType {
+        type_name: String,
+        span: Span,
+        type_definition_span: Option<Span>,
+    },
 }
 
 impl std::convert::From<TypeError> for CompileError {
@@ -1325,6 +1331,7 @@ impl Spanned for CompileError {
             } => enum_variant_name.span(),
             PanicExpressionArgumentIsNotError { span, .. } => span.clone(),
             IncoherentImplDueToOrphanRule { span, .. } => span.clone(),
+            InherentImplForExternalType { span, .. } => span.clone(),
         }
     }
 }
@@ -3121,22 +3128,22 @@ impl ToDiagnostic for CompileError {
             IncoherentImplDueToOrphanRule { trait_name, type_name, span } => Diagnostic {
                 reason: Some(Reason::new(
                     code(1),
-                    "coherence violation: only traits defined in this module can be implemented for external types".into()
+                    "coherence violation: only traits defined in this package can be implemented for external types".into()
                 )),
                 issue: Issue::error(
                     source_engine,
                     span.clone(),
                     format!(
-                        "cannot implement `{trait_name}` for `{type_name}`: both originate outside this module"
+                        "cannot implement `{trait_name}` for `{type_name}`: both originate outside this package"
                     ),
                 ),
                 hints: vec![],
                 help: {
                     let help = vec![
-                        "only traits defined in this module can be implemented for external types".to_string(),
+                        "only traits defined in this package can be implemented for external types".to_string(),
                         Diagnostic::help_empty_line(),
                         format!(
-                            "move this impl into the module that defines `{type_name}`"
+                            "move this impl into the package that defines `{type_name}`"
                         ),
                         format!(
                             "or define and use a local trait instead of `{trait_name}` to avoid the orphan rule"
@@ -3145,6 +3152,31 @@ impl ToDiagnostic for CompileError {
                     help
                 },
             },
+            InherentImplForExternalType { type_name, span, type_definition_span } => Diagnostic {
+                reason: Some(Reason::new(
+                    code(1),
+                    "coherence violation: inherent implementations must be defined in the type's defining package".into()
+                )),
+                issue: Issue::error(
+                    source_engine,
+                    span.clone(),
+                    format!(
+                        "cannot define inherent implementation for `{type_name}`: type is defined in a different package"
+                    ),
+                ),
+                hints: match type_definition_span.clone() {
+                    Some(def_span) => vec![Hint::info(
+                        source_engine,
+                        def_span,
+                        format!("Type `{type_name}` is defined here."),
+                    )],
+                    None => vec![],
+                },
+                help: vec![
+                    "move this impl into the package that defines the type".to_string(),
+                    "or define and use a local trait instead to avoid the orphan rule".to_string(),
+                ],
+            },
             ABIDuplicateName { span, other_span: other, is_attribute } => Diagnostic {
                 reason: Some(Reason::new(code(1), "Duplicated name found for renamed ABI type".into())),
                 issue: Issue::error(

test/src/e2e_vm_tests/test_programs/should_fail/trait_coherence/impl_self_overlap/test.toml

@@ -1,4 +1,5 @@
 category = "fail"
 
-#check: $()Type contains duplicate declarations
-#check: $()Method "do_something" already declared in type "MyType".
+# Expect coherent error for inherent impl on external type.
+#check: $()coherence violation: inherent implementations must be defined in the type's defining package
+#check: $()cannot define inherent implementation for `MyType`

test/src/e2e_vm_tests/test_programs/should_fail/trait_coherence/orphan_rules/impl_orphan/stdout.snap

@@ -7,16 +7,16 @@ output:
     Building test/src/e2e_vm_tests/test_programs/should_fail/trait_coherence/orphan_rules/impl_orphan
    Compiling library impl_orphan_lib (test/src/e2e_vm_tests/test_programs/should_fail/trait_coherence/orphan_rules/impl_orphan_lib)
    Compiling library impl_orphan (test/src/e2e_vm_tests/test_programs/should_fail/trait_coherence/orphan_rules/impl_orphan)
-error: coherence violation: only traits defined in this module can be implemented for external types
+error: coherence violation: only traits defined in this package can be implemented for external types
  --> test/src/e2e_vm_tests/test_programs/should_fail/trait_coherence/orphan_rules/impl_orphan/src/lib.sw:4:1
   |
 ...
 4 | impl Vec<bool> for bool {}
-  | ^^^^^^^^^^^^^^^^^^^^^^^^^^ cannot implement `Vec` for `bool`: both originate outside this module
+  | ^^^^^^^^^^^^^^^^^^^^^^^^^^ cannot implement `Vec` for `bool`: both originate outside this package
   |
-  = help: only traits defined in this module can be implemented for external types
+  = help: only traits defined in this package can be implemented for external types
   = help:  
-  = help: move this impl into the module that defines `bool`
+  = help: move this impl into the package that defines `bool`
   = help: or define and use a local trait instead of `Vec` to avoid the orphan rule
 ____
 

test/src/e2e_vm_tests/test_programs/should_pass/language/generic_transpose/src/main.sw

@@ -18,8 +18,12 @@ impl<T, E> MyResult<T, E> {
     }
 }
 
-impl<T, E> Option<MyResult<T, E>> {
-    pub fn transpose(self) -> MyResult<Option<T>, E> {
+trait OptionTranspose<T, E> {
+    fn transpose(self) -> MyResult<Option<T>, E>;
+}
+
+impl<T, E> OptionTranspose<T, E> for Option<MyResult<T, E>> {
+    fn transpose(self) -> MyResult<Option<T>, E> {
       match self {
           Some(MyResult::MyOk(x)) => MyResult::MyOk(Some(x)),
           Some(MyResult::MyErr(e)) => MyResult::MyErr(e),