From 10db3f8574ce8acb6322778b460e63ad695f5b25 Mon Sep 17 00:00:00 2001 From: Ayaz Hafiz Date: Mon, 25 Jul 2022 11:35:20 -0400 Subject: [PATCH] Detect ability specializations that overload different opaque types --- crates/compiler/can/src/abilities.rs | 9 +++++++- crates/compiler/can/src/def.rs | 25 +++++++++++++++++++++- crates/compiler/problem/src/can.rs | 5 +++++ crates/reporting/src/error/canonicalize.rs | 20 +++++++++++++++++ crates/reporting/tests/test_reporting.rs | 13 +++++++++++ 5 files changed, 70 insertions(+), 2 deletions(-) diff --git a/crates/compiler/can/src/abilities.rs b/crates/compiler/can/src/abilities.rs index 0047c7e6f3..40f58ad53b 100644 --- a/crates/compiler/can/src/abilities.rs +++ b/crates/compiler/can/src/abilities.rs @@ -279,6 +279,13 @@ impl IAbilitiesStore { id } + /// Finds the implementation key for a symbol specializing the ability member, if it specializes any. + /// For example, suppose `hashId : Id -> U64` specializes `hash : a -> U64 | a has Hash`. + /// Calling this with `hashId` would retrieve (hash, hashId). + pub fn impl_key(&self, specializing_symbol: Symbol) -> Option<&ImplKey> { + self.specialization_to_root.get(&specializing_symbol) + } + /// Creates a store from [`self`] that closes over the abilities/members given by the /// imported `symbols`, and their specializations (if any). pub fn closure_from_imported(&self, symbols: &VecSet) -> PendingAbilitiesStore { @@ -373,7 +380,7 @@ impl IAbilitiesStore { &self, specializing_symbol: Symbol, ) -> Option<(ImplKey, &AbilityMemberData)> { - let impl_key = self.specialization_to_root.get(&specializing_symbol)?; + let impl_key = self.impl_key(specializing_symbol)?; debug_assert!(self.ability_members.contains_key(&impl_key.ability_member)); let root_data = self .ability_members diff --git a/crates/compiler/can/src/def.rs b/crates/compiler/can/src/def.rs index e8cf48b6c4..00a41d12fd 100644 --- a/crates/compiler/can/src/def.rs +++ b/crates/compiler/can/src/def.rs @@ -1,4 +1,5 @@ use crate::abilities::AbilityMemberData; +use crate::abilities::ImplKey; use crate::abilities::MemberVariables; use crate::abilities::PendingMemberType; use crate::annotation::canonicalize_annotation; @@ -671,8 +672,30 @@ fn canonicalize_opaque<'a>( Err(()) => continue, }; - let member_impl = MemberImpl::Impl(impl_symbol); + // Did the user claim this implementation for a specialization of a different + // type? e.g. + // + // A has [Hash {hash: myHash}] + // B has [Hash {hash: myHash}] + // + // If so, that's an error and we drop the impl for this opaque type. + let member_impl = match scope.abilities_store.impl_key(impl_symbol) { + Some(ImplKey { + opaque, + ability_member, + }) => { + env.problem(Problem::OverloadedSpecialization { + overload: loc_impl.region, + original_opaque: *opaque, + ability_member: *ability_member, + }); + MemberImpl::Error + } + None => MemberImpl::Impl(impl_symbol), + }; + // Did the user already claim an implementation for the ability member for this + // type previously? (e.g. Hash {hash: hash1, hash: hash2}) let opt_old_impl_symbol = impl_map.insert(member, Loc::at(loc_impl.region, member_impl)); diff --git a/crates/compiler/problem/src/can.rs b/crates/compiler/problem/src/can.rs index 8f90017cd4..385a7438dc 100644 --- a/crates/compiler/problem/src/can.rs +++ b/crates/compiler/problem/src/can.rs @@ -169,6 +169,11 @@ pub enum Problem { region: Region, }, NoIdentifiersIntroduced(Region), + OverloadedSpecialization { + overload: Region, + original_opaque: Symbol, + ability_member: Symbol, + }, } #[derive(Clone, Debug, PartialEq)] diff --git a/crates/reporting/src/error/canonicalize.rs b/crates/reporting/src/error/canonicalize.rs index cdf962b928..4f82134a0e 100644 --- a/crates/reporting/src/error/canonicalize.rs +++ b/crates/reporting/src/error/canonicalize.rs @@ -923,6 +923,26 @@ pub fn can_problem<'b>( title = "UNNECESSARY DEFINITION".to_string(); severity = Severity::Warning; } + Problem::OverloadedSpecialization { + ability_member, + overload, + original_opaque, + } => { + doc = alloc.stack([ + alloc.reflow("This ability member specialization is already claimed to specialize another opaque type:"), + alloc.region(lines.convert_region(overload)), + alloc.concat([ + alloc.reflow("Previously, we found it to specialize "), + alloc.symbol_unqualified(ability_member), + alloc.reflow(" for "), + alloc.symbol_unqualified(original_opaque), + alloc.reflow("."), + ]), + alloc.reflow("Ability specializations can only provide implementations for one opauqe type, since all opaque types are different!"), + ]); + title = "OVERLOADED SPECIALIZATION".to_string(); + severity = Severity::Warning; + } }; Report { diff --git a/crates/reporting/tests/test_reporting.rs b/crates/reporting/tests/test_reporting.rs index 47e2b2c71f..8ce9c537f5 100644 --- a/crates/reporting/tests/test_reporting.rs +++ b/crates/reporting/tests/test_reporting.rs @@ -8463,6 +8463,19 @@ All branches in an `if` must have the same type! ), // TODO: the error message here could be seriously improved! @r###" + ── OVERLOADED SPECIALIZATION ───────────────────────────── /code/proj/Main.roc ─ + + This ability member specialization is already claimed to specialize + another opaque type: + + 7│ Two := {} has [Hash {hash}] + ^^^^ + + Previously, we found it to specialize `hash` for `One`. + + Ability specializations can only provide implementations for one + opauqe type, since all opaque types are different! + ── TYPE MISMATCH ───────────────────────────────────────── /code/proj/Main.roc ─ This specialization of `hash` is overly general: