Check complete ability implementations during canonicalization

crates/compiler/can/src/annotation.rs

@@ -1,6 +1,6 @@
 use crate::env::Env;
 use crate::procedure::References;
-use crate::scope::Scope;
+use crate::scope::{PendingAbilitiesInScope, Scope};
 use roc_collections::{ImMap, MutSet, SendMap, VecMap, VecSet};
 use roc_module::ident::{Ident, Lowercase, TagName};
 use roc_module::symbol::Symbol;
@@ -267,7 +267,7 @@ pub fn canonicalize_annotation(
     annotation: &TypeAnnotation,
     region: Region,
     var_store: &mut VarStore,
-    pending_abilities_in_scope: &[Symbol],
+    pending_abilities_in_scope: &PendingAbilitiesInScope,
 ) -> Annotation {
     let mut introduced_variables = IntroducedVariables::default();
     let mut references = VecSet::default();
@@ -908,7 +908,7 @@ fn canonicalize_has_clause(
     var_store: &mut VarStore,
     introduced_variables: &mut IntroducedVariables,
     clause: &Loc<roc_parse::ast::HasClause<'_>>,
-    pending_abilities_in_scope: &[Symbol],
+    pending_abilities_in_scope: &PendingAbilitiesInScope,
     references: &mut VecSet<Symbol>,
 ) -> Result<(), Type> {
     let Loc {
@@ -929,7 +929,7 @@ fn canonicalize_has_clause(
             let symbol = make_apply_symbol(env, ability.region, scope, module_name, ident)?;
 
             // Ability defined locally, whose members we are constructing right now...
-            if !pending_abilities_in_scope.contains(&symbol)
+            if !pending_abilities_in_scope.contains_key(&symbol)
                 // or an ability that was imported from elsewhere
                 && !scope.abilities_store.is_ability(symbol)
             {

crates/compiler/can/src/def.rs

@@ -19,6 +19,7 @@ use crate::scope::create_alias;
 use crate::scope::{PendingAbilitiesInScope, Scope};
 use roc_collections::ReferenceMatrix;
 use roc_collections::VecMap;
+use roc_collections::VecSet;
 use roc_collections::{ImSet, MutMap, SendMap};
 use roc_error_macros::internal_error;
 use roc_module::ident::Ident;
@@ -302,7 +303,7 @@ fn canonicalize_alias<'a>(
     output: &mut Output,
     var_store: &mut VarStore,
     scope: &mut Scope,
-    pending_abilities_in_scope: &[Symbol],
+    pending_abilities_in_scope: &PendingAbilitiesInScope,
 
     name: Loc<Symbol>,
     ann: &'a Loc<ast::TypeAnnotation<'a>>,
@@ -526,6 +527,65 @@ fn canonicalize_claimed_ability_impl<'a>(
     }
 }
 
+struct SeparatedMembers {
+    not_required: Vec<Symbol>,
+    not_implemented: Vec<Symbol>,
+}
+
+/// Partitions ability members in a `has [ Ability {...members} ]` clause into the members the
+/// opaque type claims to implement but are not part of the ability, and the ones it does not
+/// implement.
+fn separate_implemented_and_required_members(
+    implemented: VecSet<Symbol>,
+    required: VecSet<Symbol>,
+) -> SeparatedMembers {
+    use std::cmp::Ordering;
+
+    let mut implemented = implemented.into_vec();
+    let mut required = required.into_vec();
+
+    implemented.sort();
+    required.sort();
+
+    let mut implemented = implemented.into_iter().peekable();
+    let mut required = required.into_iter().peekable();
+
+    let mut not_required = vec![];
+    let mut not_implemented = vec![];
+
+    loop {
+        // Equal => both required and implemented
+        // Less => implemented but not required
+        // Greater => required but not implemented
+
+        let ord = match (implemented.peek(), required.peek()) {
+            (Some(implemented), Some(required)) => Some(implemented.cmp(required)),
+            (Some(_), None) => Some(Ordering::Less),
+            (None, Some(_)) => Some(Ordering::Greater),
+            (None, None) => None,
+        };
+
+        match ord {
+            Some(Ordering::Less) => {
+                not_required.push(implemented.next().unwrap());
+            }
+            Some(Ordering::Greater) => {
+                not_implemented.push(required.next().unwrap());
+            }
+            Some(Ordering::Equal) => {
+                _ = implemented.next().unwrap();
+                _ = required.next().unwrap();
+            }
+            None => break,
+        }
+    }
+
+    SeparatedMembers {
+        not_required,
+        not_implemented,
+    }
+}
+
 #[inline(always)]
 #[allow(clippy::too_many_arguments)]
 fn canonicalize_opaque<'a>(
@@ -533,7 +593,7 @@ fn canonicalize_opaque<'a>(
     output: &mut Output,
     var_store: &mut VarStore,
     scope: &mut Scope,
-    pending_abilities_in_scope: &[Symbol],
+    pending_abilities_in_scope: &PendingAbilitiesInScope,
 
     name: Loc<Symbol>,
     ann: &'a Loc<ast::TypeAnnotation<'a>>,
@@ -567,16 +627,20 @@ fn canonicalize_opaque<'a>(
 
             let ability_region = ability.region;
 
-            let ability = match ability.value {
+            let (ability, members) = match ability.value {
                 ast::TypeAnnotation::Apply(module_name, ident, []) => {
                     match make_apply_symbol(env, region, scope, module_name, ident) {
                         Ok(ability) => {
-                            if scope.abilities_store.is_ability(ability)
-                                || pending_abilities_in_scope.contains(&ability)
-                            {
+                            let opt_members = scope
+                                .abilities_store
+                                .members_of_ability(ability)
+                                .map(|members| members.iter().copied().collect())
+                                .or_else(|| pending_abilities_in_scope.get(&ability).cloned());
+
+                            if let Some(members) = opt_members {
                                 // This is an ability we already imported into the scope,
                                 // or which is also undergoing canonicalization at the moment.
-                                ability
+                                (ability, members)
                             } else {
                                 env.problem(Problem::NotAnAbility(ability_region));
                                 continue;
@@ -624,6 +688,44 @@ fn canonicalize_opaque<'a>(
                     }
                 }
 
+                // Check that the members this opaque claims to implement corresponds 1-to-1 with
+                // the members the ability offers.
+                let SeparatedMembers {
+                    not_required,
+                    not_implemented,
+                } = separate_implemented_and_required_members(
+                    impl_map.iter().map(|(member, _)| *member).collect(),
+                    members,
+                );
+
+                if !not_required.is_empty() {
+                    for sym in not_required.iter() {
+                        impl_map.remove(sym);
+                    }
+                    env.problem(Problem::ImplementsNonRequired {
+                        region,
+                        ability,
+                        not_required,
+                    });
+                    // Implementing something that's not required is a recoverable error, we don't
+                    // need to skip association of the implemented abilities.
+                }
+
+                if !not_implemented.is_empty() {
+                    env.problem(Problem::DoesNotImplementAbility {
+                        region,
+                        ability,
+                        not_implemented,
+                    });
+                    // However not implementing something that is required is not recoverable for
+                    // an ability, so skip association.
+                    // TODO: can we "partially" associate members of an ability and generate
+                    // RuntimeErrors for unimplemented members?
+                    // TODO: can we derive implementations of unimplemented members for builtin
+                    // abilities?
+                    continue;
+                }
+
                 supported_abilities.push(OpaqueSupports::Implemented {
                     ability_name: ability,
                     impls: impl_map
@@ -744,8 +846,14 @@ pub(crate) fn canonicalize_defs<'a>(
         scope.register_debug_idents();
     }
 
-    let (aliases, symbols_introduced) =
-        canonicalize_type_defs(env, &mut output, var_store, scope, pending_type_defs);
+    let (aliases, symbols_introduced) = canonicalize_type_defs(
+        env,
+        &mut output,
+        var_store,
+        scope,
+        &pending_abilities_in_scope,
+        pending_type_defs,
+    );
 
     // Now that we have the scope completely assembled, and shadowing resolved,
     // we're ready to canonicalize any body exprs.
@@ -871,6 +979,7 @@ fn canonicalize_type_defs<'a>(
     output: &mut Output,
     var_store: &mut VarStore,
     scope: &mut Scope,
+    pending_abilities_in_scope: &PendingAbilitiesInScope,
     pending_type_defs: Vec<PendingTypeDef<'a>>,
 ) -> (VecMap<Symbol, Alias>, MutMap<Symbol, Region>) {
     enum TypeDef<'a> {
@@ -889,8 +998,6 @@ fn canonicalize_type_defs<'a>(
     }
 
     let mut type_defs = MutMap::default();
-    let mut pending_abilities_in_scope = Vec::new();
-
     let mut referenced_type_symbols = VecMap::default();
 
     // Determine which idents we introduced in the course of this process.
@@ -936,7 +1043,6 @@ fn canonicalize_type_defs<'a>(
 
                 referenced_type_symbols.insert(name.value, referenced_symbols);
                 type_defs.insert(name.value, TypeDef::Ability(name, members));
-                pending_abilities_in_scope.push(name.value);
             }
             PendingTypeDef::InvalidAlias { .. }
             | PendingTypeDef::InvalidAbility { .. }
@@ -976,7 +1082,7 @@ fn canonicalize_type_defs<'a>(
                     output,
                     var_store,
                     scope,
-                    &pending_abilities_in_scope,
+                    pending_abilities_in_scope,
                     name,
                     ann,
                     &vars,
@@ -1031,7 +1137,7 @@ fn resolve_abilities<'a>(
     var_store: &mut VarStore,
     scope: &mut Scope,
     abilities: MutMap<Symbol, Vec<PendingAbilityMember>>,
-    pending_abilities_in_scope: &[Symbol],
+    pending_abilities_in_scope: &PendingAbilitiesInScope,
 ) {
     for (ability, members) in abilities {
         let mut can_members = Vec::with_capacity(members.len());
@@ -1684,7 +1790,7 @@ fn canonicalize_pending_value_def<'a>(
     use PendingValueDef::*;
 
     // All abilities should be resolved by the time we're canonicalizing value defs.
-    let pending_abilities_in_scope = &[];
+    let pending_abilities_in_scope = &Default::default();
 
     let output = match pending_def {
         AnnotationOnly(_, loc_can_pattern, loc_ann) => {

crates/compiler/can/src/module.rs

@@ -434,7 +434,7 @@ pub fn canonicalize_module_defs<'a>(
         .iter()
         .map(|(symbol, loc_ann)| {
             // We've already canonicalized the module, so there are no pending abilities.
-            let pending_abilities_in_scope = &[];
+            let pending_abilities_in_scope = &Default::default();
 
             let ann = canonicalize_annotation(
                 &mut env,

crates/compiler/can/src/scope.rs

@@ -1,4 +1,4 @@
-use roc_collections::VecMap;
+use roc_collections::{VecMap, VecSet};
 use roc_module::ident::Ident;
 use roc_module::symbol::{IdentId, IdentIds, ModuleId, Symbol};
 use roc_problem::can::RuntimeError;
@@ -10,7 +10,7 @@ use crate::abilities::PendingAbilitiesStore;
 use bitvec::vec::BitVec;
 
 // ability -> member names
-pub(crate) type PendingAbilitiesInScope = VecMap<Symbol, Vec<Symbol>>;
+pub(crate) type PendingAbilitiesInScope = VecMap<Symbol, VecSet<Symbol>>;
 
 #[derive(Clone, Debug)]
 pub struct Scope {

crates/compiler/problem/src/can.rs

@@ -155,6 +155,16 @@ pub enum Problem {
         duplicate: Region,
     },
     NotAnAbility(Region),
+    ImplementsNonRequired {
+        region: Region,
+        ability: Symbol,
+        not_required: Vec<Symbol>,
+    },
+    DoesNotImplementAbility {
+        region: Region,
+        ability: Symbol,
+        not_implemented: Vec<Symbol>,
+    },
 }
 
 #[derive(Clone, Debug, PartialEq)]

crates/reporting/src/error/canonicalize.rs

@@ -55,6 +55,8 @@ const OPTIONAL_ABILITY_IMPLEMENTATION: &str = "OPTIONAL ABILITY IMPLEMENTATION";
 const QUALIFIED_ABILITY_IMPLEMENTATION: &str = "QUALIFIED ABILITY IMPLEMENTATION";
 const ABILITY_IMPLEMENTATION_NOT_IDENTIFIER: &str = "ABILITY IMPLEMENTATION NOT IDENTIFIER";
 const DUPLICATE_IMPLEMENTATION: &str = "DUPLICATE IMPLEMENTATION";
+const UNNECESSARY_IMPLEMENTATIONS: &str = "UNNECESSARY IMPLEMENTATIONS";
+const INCOMPLETE_ABILITY_IMPLEMENTATION: &str = "INCOMPLETE ABILITY IMPLEMENTATION";
 
 pub fn can_problem<'b>(
     alloc: &'b RocDocAllocator<'b>,
@@ -850,6 +852,56 @@ pub fn can_problem<'b>(
             title = DUPLICATE_IMPLEMENTATION.to_string();
             severity = Severity::RuntimeError;
         }
+        Problem::ImplementsNonRequired {
+            region,
+            ability,
+            not_required,
+        } => {
+            doc = alloc.stack([
+                alloc.concat([
+                    alloc.reflow("This type implements members that are not part of the "),
+                    alloc.symbol_unqualified(ability),
+                    alloc.reflow(" ability:"),
+                ]),
+                alloc.region(lines.convert_region(region)),
+                alloc.reflow("The following implemented members should not be listed:"),
+                alloc.type_block(
+                    alloc.intersperse(
+                        not_required
+                            .into_iter()
+                            .map(|sym| alloc.symbol_unqualified(sym)),
+                        alloc.string(",".to_string()).append(alloc.space()),
+                    ),
+                ),
+            ]);
+            title = UNNECESSARY_IMPLEMENTATIONS.to_string();
+            severity = Severity::Warning;
+        }
+        Problem::DoesNotImplementAbility {
+            region,
+            ability,
+            not_implemented,
+        } => {
+            doc = alloc.stack([
+                alloc.concat([
+                    alloc.reflow("This type does not fully implement the "),
+                    alloc.symbol_unqualified(ability),
+                    alloc.reflow(" ability:"),
+                ]),
+                alloc.region(lines.convert_region(region)),
+                alloc.reflow("The following necessary members are missing implementations:"),
+                alloc.type_block(
+                    alloc.intersperse(
+                        not_implemented
+                            .into_iter()
+                            .map(|sym| alloc.symbol_unqualified(sym)),
+                        alloc.string(",".to_string()).append(alloc.space()),
+                    ),
+                ),
+            ]);
+            title = INCOMPLETE_ABILITY_IMPLEMENTATION.to_string();
+            severity = Severity::RuntimeError;
+        }
     };
 
     Report {

crates/reporting/tests/test_reporting.rs

@@ -8413,17 +8413,28 @@ All branches in an `if` must have the same type!
             eq = \@Id m, @Id n -> m == n
             "#
         ),
-        @r#"
-        ── INCOMPLETE ABILITY IMPLEMENTATION ───────────────────── /code/proj/Main.roc ─
+        @r###"
+    ── INCOMPLETE ABILITY IMPLEMENTATION ───────────────────── /code/proj/Main.roc ─
 
-        The type `Id` does not fully implement the ability `Eq`. The following
-        specializations are missing:
+    This type does not fully implement the `Eq` ability:
 
-        A specialization for `le`, which is defined here:
+    7│  Id := U64 has [Eq {eq}]
+                       ^^^^^^^
 
-        5│      le : a, a -> Bool | a has Eq
-                ^^
-        "#
+    The following necessary members are missing implementations:
+
+        le
+
+    ── INCOMPLETE ABILITY IMPLEMENTATION ───────────────────── /code/proj/Main.roc ─
+
+    The type `Id` does not fully implement the ability `Eq`. The following
+    specializations are missing:
+
+    A specialization for `le`, which is defined here:
+
+    5│      le : a, a -> Bool | a has Eq
+            ^^
+    "###
     );
 
     test_report!(
@@ -9175,6 +9186,17 @@ All branches in an `if` must have the same type!
     Tip: consider adding a value of name `eq` in this scope, or using
     another variable that implements this ability member, like
     { eq: myeq }
+
+    ── INCOMPLETE ABILITY IMPLEMENTATION ───────────────────── /code/proj/Main.roc ─
+
+    This type does not fully implement the `Eq` ability:
+
+    5│  A := U8 has [Eq {eq}]
+                     ^^^^^^^
+
+    The following necessary members are missing implementations:
+
+        eq
     "###
     );
 
@@ -9205,6 +9227,17 @@ All branches in an `if` must have the same type!
         myEq
         eq
         U8
+
+    ── INCOMPLETE ABILITY IMPLEMENTATION ───────────────────── /code/proj/Main.roc ─
+
+    This type does not fully implement the `Eq` ability:
+
+    5│  A := U8 has [ Eq {eq: aEq} ]
+                      ^^^^^^^^^^^^
+
+    The following necessary members are missing implementations:
+
+        eq
     "###
     );
 
@@ -9232,6 +9265,17 @@ All branches in an `if` must have the same type!
     Custom implementations must be supplied fully.
 
 
+
+    ── INCOMPLETE ABILITY IMPLEMENTATION ───────────────────── /code/proj/Main.roc ─
+
+    This type does not fully implement the `Eq` ability:
+
+    5│  A := U8 has [ Eq {eq ? aEq} ]
+                      ^^^^^^^^^^^^^
+
+    The following necessary members are missing implementations:
+
+        eq
     "###
     );
 
@@ -9261,6 +9305,17 @@ All branches in an `if` must have the same type!
     Hint: if you want this implementation to be derived, don't include a
     record of implementations. For example,    has [Encoding] will attempt
     to derive `Encoding`
+
+    ── INCOMPLETE ABILITY IMPLEMENTATION ───────────────────── /code/proj/Main.roc ─
+
+    This type does not fully implement the `Encoding` ability:
+
+    5│  A := U8 has [ Encoding {toEncoder ? myEncoder} ]
+                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+    The following necessary members are missing implementations:
+
+        toEncoder
     "###
     );
 
@@ -9285,6 +9340,17 @@ All branches in an `if` must have the same type!
 
     Custom implementations must be defined in the local scope, and
     unqualified.
+
+    ── INCOMPLETE ABILITY IMPLEMENTATION ───────────────────── /code/proj/Main.roc ─
+
+    This type does not fully implement the `Eq` ability:
+
+    5│  A := U8 has [ Eq {eq : Bool.eq} ]
+                      ^^^^^^^^^^^^^^^^^
+
+    The following necessary members are missing implementations:
+
+        eq
     "###
     );
 
@@ -9311,6 +9377,17 @@ All branches in an `if` must have the same type!
     unqualified identifiers, not arbitrary expressions.
 
     Tip: consider defining this expression as a variable.
+
+    ── INCOMPLETE ABILITY IMPLEMENTATION ───────────────────── /code/proj/Main.roc ─
+
+    This type does not fully implement the `Eq` ability:
+
+    5│  A := U8 has [ Eq {eq : \m, n -> m == n} ]
+                      ^^^^^^^^^^^^^^^^^^^^^^^^^
+
+    The following necessary members are missing implementations:
+
+        eq
     "###
     );