diff --git a/crates/compiler/can/src/annotation.rs b/crates/compiler/can/src/annotation.rs index 8647a23cfd..abfa18be5d 100644 --- a/crates/compiler/can/src/annotation.rs +++ b/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>, - pending_abilities_in_scope: &[Symbol], + pending_abilities_in_scope: &PendingAbilitiesInScope, references: &mut VecSet, ) -> 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) { diff --git a/crates/compiler/can/src/def.rs b/crates/compiler/can/src/def.rs index fd397dcec8..5213a6e182 100644 --- a/crates/compiler/can/src/def.rs +++ b/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, ann: &'a Loc>, @@ -526,6 +527,65 @@ fn canonicalize_claimed_ability_impl<'a>( } } +struct SeparatedMembers { + not_required: Vec, + not_implemented: Vec, +} + +/// 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, + required: VecSet, +) -> 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, ann: &'a Loc>, @@ -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>, ) -> (VecMap, MutMap) { 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>, - 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) => { diff --git a/crates/compiler/can/src/module.rs b/crates/compiler/can/src/module.rs index c8f73f684b..d107e9485a 100644 --- a/crates/compiler/can/src/module.rs +++ b/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, diff --git a/crates/compiler/can/src/scope.rs b/crates/compiler/can/src/scope.rs index dd43015027..64b19e7d6b 100644 --- a/crates/compiler/can/src/scope.rs +++ b/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>; +pub(crate) type PendingAbilitiesInScope = VecMap>; #[derive(Clone, Debug)] pub struct Scope { diff --git a/crates/compiler/problem/src/can.rs b/crates/compiler/problem/src/can.rs index b0bf0296b7..e52e6a7289 100644 --- a/crates/compiler/problem/src/can.rs +++ b/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, + }, + DoesNotImplementAbility { + region: Region, + ability: Symbol, + not_implemented: Vec, + }, } #[derive(Clone, Debug, PartialEq)] diff --git a/crates/reporting/src/error/canonicalize.rs b/crates/reporting/src/error/canonicalize.rs index b423636298..cd9c9afba5 100644 --- a/crates/reporting/src/error/canonicalize.rs +++ b/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 { diff --git a/crates/reporting/tests/test_reporting.rs b/crates/reporting/tests/test_reporting.rs index 6a3e255e73..33921f969e 100644 --- a/crates/reporting/tests/test_reporting.rs +++ b/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 "### );