From 958f64c8fc043b6e31529a5cb6595d3e79f75ea3 Mon Sep 17 00:00:00 2001 From: Ayaz Hafiz Date: Tue, 9 Aug 2022 08:13:44 -0700 Subject: [PATCH 1/8] Turn on reporting test for underivable record --- crates/reporting/tests/test_reporting.rs | 24 ++++++++++++++++++++++-- 1 file changed, 22 insertions(+), 2 deletions(-) diff --git a/crates/reporting/tests/test_reporting.rs b/crates/reporting/tests/test_reporting.rs index 496fc388a7..50dc8c9bc5 100644 --- a/crates/reporting/tests/test_reporting.rs +++ b/crates/reporting/tests/test_reporting.rs @@ -10296,13 +10296,13 @@ All branches in an `if` must have the same type! ); test_report!( - #[ignore = "needs structural deriving to be turned on first"] nested_opaque_cannot_derive_encoding, indoc!( r#" app "test" imports [Decode.{Decoder, DecoderFormatting, decoder}] provides [main] to "./platform" - A : {} + A := {} + main = myDecoder : Decoder {x : A} fmt | fmt has DecoderFormatting myDecoder = decoder @@ -10311,6 +10311,26 @@ All branches in an `if` must have the same type! "# ), @r###" + ── TYPE MISMATCH ───────────────────────────────────────── /code/proj/Main.roc ─ + + This expression has a type that does not implement the abilities it's expected to: + + 7│ myDecoder = decoder + ^^^^^^^ + + Roc can't generate an implementation of the `Decode.Decoding` ability + for + + { x : A } + + In particular, an implementation for + + A + + cannot be generated. + + Tip: `A` does not implement `Decoding`. Consider adding a custom + implementation or `has Decode.Decoding` to the definition of `A`. "### ); From d2b9cc056f3da09c74ddbf9dec7c23ea187ca75b Mon Sep 17 00:00:00 2001 From: Ayaz Hafiz Date: Tue, 9 Aug 2022 08:15:09 -0700 Subject: [PATCH 2/8] Record with optionally-typed fields cannot be derived for decoding --- crates/compiler/solve/src/ability.rs | 37 +++++++++++++++++++----- crates/reporting/tests/test_reporting.rs | 34 ++++++++++++++++++++++ 2 files changed, 64 insertions(+), 7 deletions(-) diff --git a/crates/compiler/solve/src/ability.rs b/crates/compiler/solve/src/ability.rs index a356fa74d0..45105b458e 100644 --- a/crates/compiler/solve/src/ability.rs +++ b/crates/compiler/solve/src/ability.rs @@ -6,8 +6,10 @@ use roc_module::symbol::Symbol; use roc_region::all::{Loc, Region}; use roc_solve_problem::{TypeError, UnderivableReason, Unfulfilled}; use roc_types::num::NumericRange; -use roc_types::subs::{instantiate_rigids, Content, FlatType, GetSubsSlice, Rank, Subs, Variable}; -use roc_types::types::{AliasKind, Category, MemberImpl, PatternCategory}; +use roc_types::subs::{ + instantiate_rigids, Content, FlatType, GetSubsSlice, Rank, RecordFields, Subs, Variable, +}; +use roc_types::types::{AliasKind, Category, MemberImpl, PatternCategory, RecordField}; use roc_unify::unify::{Env, MustImplementConstraints}; use roc_unify::unify::{MustImplementAbility, Obligated}; @@ -476,7 +478,11 @@ trait DerivableVisitor { } #[inline(always)] - fn visit_record(var: Variable) -> Result { + fn visit_record( + _subs: &Subs, + var: Variable, + _fields: RecordFields, + ) -> Result { Err(DerivableError::NotDerivable(var)) } @@ -574,7 +580,7 @@ trait DerivableVisitor { } } Record(fields, ext) => { - let descend = Self::visit_record(var)?; + let descend = Self::visit_record(subs, var, fields)?; if descend.0 { push_var_slice!(fields.variables()); if !matches!( @@ -682,7 +688,11 @@ impl DerivableVisitor for DeriveEncoding { } #[inline(always)] - fn visit_record(_var: Variable) -> Result { + fn visit_record( + _subs: &Subs, + _var: Variable, + _fields: RecordFields, + ) -> Result { Ok(Descend(true)) } @@ -753,8 +763,21 @@ impl DerivableVisitor for DeriveDecoding { } #[inline(always)] - fn visit_record(_var: Variable) -> Result { - Ok(Descend(true)) + fn visit_record( + subs: &Subs, + var: Variable, + fields: RecordFields, + ) -> Result { + let has_optional_field = subs + .get_subs_slice(fields.record_fields()) + .iter() + .any(|field| matches!(field, RecordField::Optional(..))); + + if has_optional_field { + Err(DerivableError::NotDerivable(var)) + } else { + Ok(Descend(true)) + } } #[inline(always)] diff --git a/crates/reporting/tests/test_reporting.rs b/crates/reporting/tests/test_reporting.rs index 50dc8c9bc5..1026f10a8f 100644 --- a/crates/reporting/tests/test_reporting.rs +++ b/crates/reporting/tests/test_reporting.rs @@ -10507,4 +10507,38 @@ All branches in an `if` must have the same type! Note: `Decoding` cannot be generated for functions. "### ); + + test_report!( + record_with_optional_field_types_cannot_derive_decoding, + indoc!( + r#" + app "test" imports [Decode.{Decoder, DecoderFormatting, decoder}] provides [main] to "./platform" + + main = + myDecoder : Decoder {x : Str, y ? Str} fmt | fmt has DecoderFormatting + myDecoder = decoder + + myDecoder + "# + ), + @r###" + ── TYPE MISMATCH ───────────────────────────────────────── /code/proj/Main.roc ─ + + This expression has a type that does not implement the abilities it's expected to: + + 5│ myDecoder = decoder + ^^^^^^^ + + Roc can't generate an implementation of the `Decode.Decoding` ability + for + + { x : Str, y ? Str } + + Note: I can't derive decoding for a record with an optional field, + which in this case is `.y`. Optional record fields are polymorphic over + records that may or may not contain them at compile time, but are not + a concept that extends to runtime! + Maybe you wanted to use a `Result`? + "### + ); } From 55fe1df9957eb17c31b5b177b0d52bde72215a9c Mon Sep 17 00:00:00 2001 From: Ayaz Hafiz Date: Tue, 9 Aug 2022 08:44:20 -0700 Subject: [PATCH 3/8] Add more context to derivability errors when they happen --- Cargo.lock | 1 + crates/compiler/solve/src/ability.rs | 196 ++++++++++++++++------- crates/compiler/solve_problem/src/lib.rs | 20 ++- crates/reporting/Cargo.toml | 1 + crates/reporting/src/error/type.rs | 118 ++++++++------ 5 files changed, 225 insertions(+), 111 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index e1029bf238..138a0b7ab5 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4037,6 +4037,7 @@ dependencies = [ "roc_collections", "roc_constrain", "roc_derive", + "roc_error_macros", "roc_exhaustive", "roc_fmt", "roc_load", diff --git a/crates/compiler/solve/src/ability.rs b/crates/compiler/solve/src/ability.rs index 45105b458e..bc4bfdb794 100644 --- a/crates/compiler/solve/src/ability.rs +++ b/crates/compiler/solve/src/ability.rs @@ -4,7 +4,7 @@ use roc_collections::{VecMap, VecSet}; use roc_error_macros::{internal_error, todo_abilities}; use roc_module::symbol::Symbol; use roc_region::all::{Loc, Region}; -use roc_solve_problem::{TypeError, UnderivableReason, Unfulfilled}; +use roc_solve_problem::{NotDerivableContext, TypeError, UnderivableReason, Unfulfilled}; use roc_types::num::NumericRange; use roc_types::subs::{ instantiate_rigids, Content, FlatType, GetSubsSlice, Rank, RecordFields, Subs, Variable, @@ -278,11 +278,14 @@ impl ObligationCache { // can derive! None } - Some(Err(DerivableError::NotDerivable(failure_var))) => Some(if failure_var == var { - UnderivableReason::SurfaceNotDerivable + Some(Err(NotDerivable { + var: failure_var, + context, + })) => Some(if failure_var == var { + UnderivableReason::SurfaceNotDerivable(context) } else { let (error_type, _skeletons) = subs.var_to_error_type(failure_var); - UnderivableReason::NestedNotDerivable(error_type) + UnderivableReason::NestedNotDerivable(error_type, context) }), None => Some(UnderivableReason::NotABuiltin), }; @@ -430,8 +433,9 @@ fn is_builtin_number_alias(symbol: Symbol) -> bool { ) } -enum DerivableError { - NotDerivable(Variable), +struct NotDerivable { + var: Variable, + context: NotDerivableContext, } struct Descend(bool); @@ -445,36 +449,51 @@ trait DerivableVisitor { } #[inline(always)] - fn visit_flex_able(var: Variable, ability: Symbol) -> Result<(), DerivableError> { + fn visit_flex_able(var: Variable, ability: Symbol) -> Result<(), NotDerivable> { if ability != Self::ABILITY { - Err(DerivableError::NotDerivable(var)) + Err(NotDerivable { + var, + context: NotDerivableContext::NoContext, + }) } else { Ok(()) } } #[inline(always)] - fn visit_rigid_able(var: Variable, ability: Symbol) -> Result<(), DerivableError> { + fn visit_rigid_able(var: Variable, ability: Symbol) -> Result<(), NotDerivable> { if ability != Self::ABILITY { - Err(DerivableError::NotDerivable(var)) + Err(NotDerivable { + var, + context: NotDerivableContext::NoContext, + }) } else { Ok(()) } } #[inline(always)] - fn visit_recursion(var: Variable) -> Result { - Err(DerivableError::NotDerivable(var)) + fn visit_recursion(var: Variable) -> Result { + Err(NotDerivable { + var, + context: NotDerivableContext::NoContext, + }) } #[inline(always)] - fn visit_apply(var: Variable, _symbol: Symbol) -> Result { - Err(DerivableError::NotDerivable(var)) + fn visit_apply(var: Variable, _symbol: Symbol) -> Result { + Err(NotDerivable { + var, + context: NotDerivableContext::NoContext, + }) } #[inline(always)] - fn visit_func(var: Variable) -> Result { - Err(DerivableError::NotDerivable(var)) + fn visit_func(var: Variable) -> Result { + Err(NotDerivable { + var, + context: NotDerivableContext::Function, + }) } #[inline(always)] @@ -482,43 +501,67 @@ trait DerivableVisitor { _subs: &Subs, var: Variable, _fields: RecordFields, - ) -> Result { - Err(DerivableError::NotDerivable(var)) + ) -> Result { + Err(NotDerivable { + var, + context: NotDerivableContext::NoContext, + }) } #[inline(always)] - fn visit_tag_union(var: Variable) -> Result { - Err(DerivableError::NotDerivable(var)) + fn visit_tag_union(var: Variable) -> Result { + Err(NotDerivable { + var, + context: NotDerivableContext::NoContext, + }) } #[inline(always)] - fn visit_recursive_tag_union(var: Variable) -> Result { - Err(DerivableError::NotDerivable(var)) + fn visit_recursive_tag_union(var: Variable) -> Result { + Err(NotDerivable { + var, + context: NotDerivableContext::NoContext, + }) } #[inline(always)] - fn visit_function_or_tag_union(var: Variable) -> Result { - Err(DerivableError::NotDerivable(var)) + fn visit_function_or_tag_union(var: Variable) -> Result { + Err(NotDerivable { + var, + context: NotDerivableContext::NoContext, + }) } #[inline(always)] - fn visit_empty_record(var: Variable) -> Result<(), DerivableError> { - Err(DerivableError::NotDerivable(var)) + fn visit_empty_record(var: Variable) -> Result<(), NotDerivable> { + Err(NotDerivable { + var, + context: NotDerivableContext::NoContext, + }) } #[inline(always)] - fn visit_empty_tag_union(var: Variable) -> Result<(), DerivableError> { - Err(DerivableError::NotDerivable(var)) + fn visit_empty_tag_union(var: Variable) -> Result<(), NotDerivable> { + Err(NotDerivable { + var, + context: NotDerivableContext::NoContext, + }) } #[inline(always)] - fn visit_alias(var: Variable, _symbol: Symbol) -> Result { - Err(DerivableError::NotDerivable(var)) + fn visit_alias(var: Variable, _symbol: Symbol) -> Result { + Err(NotDerivable { + var, + context: NotDerivableContext::NoContext, + }) } #[inline(always)] - fn visit_ranged_number(var: Variable, _range: NumericRange) -> Result<(), DerivableError> { - Err(DerivableError::NotDerivable(var)) + fn visit_ranged_number(var: Variable, _range: NumericRange) -> Result<(), NotDerivable> { + Err(NotDerivable { + var, + context: NotDerivableContext::NoContext, + }) } #[inline(always)] @@ -527,7 +570,7 @@ trait DerivableVisitor { abilities_store: &AbilitiesStore, subs: &mut Subs, var: Variable, - ) -> Result<(), DerivableError> { + ) -> Result<(), NotDerivable> { let mut stack = vec![var]; let mut seen_recursion_vars = vec![]; @@ -545,14 +588,18 @@ trait DerivableVisitor { let content = subs.get_content_without_compacting(var); use Content::*; - use DerivableError::*; use FlatType::*; match *content { FlexVar(opt_name) => { // Promote the flex var to be bound to the ability. subs.set_content(var, Content::FlexAbleVar(opt_name, Self::ABILITY)); } - RigidVar(_) => return Err(NotDerivable(var)), + RigidVar(_) => { + return Err(NotDerivable { + var, + context: NotDerivableContext::NoContext, + }) + } FlexAbleVar(_, ability) => Self::visit_flex_able(var, ability)?, RigidAbleVar(_, ability) => Self::visit_rigid_able(var, ability)?, RecursionVar { @@ -622,7 +669,12 @@ trait DerivableVisitor { EmptyRecord => Self::visit_empty_record(var)?, EmptyTagUnion => Self::visit_empty_tag_union(var)?, - Erroneous(_) => return Err(NotDerivable(var)), + Erroneous(_) => { + return Err(NotDerivable { + var, + context: NotDerivableContext::NoContext, + }) + } }, Alias( Symbol::NUM_NUM | Symbol::NUM_INTEGER | Symbol::NUM_FLOATINGPOINT, @@ -639,7 +691,10 @@ trait DerivableVisitor { .is_err() && !Self::is_derivable_builtin_opaque(opaque) { - return Err(NotDerivable(var)); + return Err(NotDerivable { + var, + context: NotDerivableContext::Opaque(opaque), + }); } } Alias(symbol, _alias_variables, real_var, AliasKind::Structural) => { @@ -650,9 +705,17 @@ trait DerivableVisitor { } RangedNumber(range) => Self::visit_ranged_number(var, range)?, - LambdaSet(..) => return Err(NotDerivable(var)), + LambdaSet(..) => { + return Err(NotDerivable { + var, + context: NotDerivableContext::NoContext, + }) + } Error => { - return Err(NotDerivable(var)); + return Err(NotDerivable { + var, + context: NotDerivableContext::NoContext, + }); } } } @@ -671,19 +734,22 @@ impl DerivableVisitor for DeriveEncoding { } #[inline(always)] - fn visit_recursion(_var: Variable) -> Result { + fn visit_recursion(_var: Variable) -> Result { Ok(Descend(true)) } #[inline(always)] - fn visit_apply(var: Variable, symbol: Symbol) -> Result { + fn visit_apply(var: Variable, symbol: Symbol) -> Result { if matches!( symbol, Symbol::LIST_LIST | Symbol::SET_SET | Symbol::DICT_DICT | Symbol::STR_STR, ) { Ok(Descend(true)) } else { - Err(DerivableError::NotDerivable(var)) + Err(NotDerivable { + var, + context: NotDerivableContext::NoContext, + }) } } @@ -692,37 +758,37 @@ impl DerivableVisitor for DeriveEncoding { _subs: &Subs, _var: Variable, _fields: RecordFields, - ) -> Result { + ) -> Result { Ok(Descend(true)) } #[inline(always)] - fn visit_tag_union(_var: Variable) -> Result { + fn visit_tag_union(_var: Variable) -> Result { Ok(Descend(true)) } #[inline(always)] - fn visit_recursive_tag_union(_var: Variable) -> Result { + fn visit_recursive_tag_union(_var: Variable) -> Result { Ok(Descend(true)) } #[inline(always)] - fn visit_function_or_tag_union(_var: Variable) -> Result { + fn visit_function_or_tag_union(_var: Variable) -> Result { Ok(Descend(true)) } #[inline(always)] - fn visit_empty_record(_var: Variable) -> Result<(), DerivableError> { + fn visit_empty_record(_var: Variable) -> Result<(), NotDerivable> { Ok(()) } #[inline(always)] - fn visit_empty_tag_union(_var: Variable) -> Result<(), DerivableError> { + fn visit_empty_tag_union(_var: Variable) -> Result<(), NotDerivable> { Ok(()) } #[inline(always)] - fn visit_alias(_var: Variable, symbol: Symbol) -> Result { + fn visit_alias(_var: Variable, symbol: Symbol) -> Result { if is_builtin_number_alias(symbol) { Ok(Descend(false)) } else { @@ -731,7 +797,7 @@ impl DerivableVisitor for DeriveEncoding { } #[inline(always)] - fn visit_ranged_number(_var: Variable, _range: NumericRange) -> Result<(), DerivableError> { + fn visit_ranged_number(_var: Variable, _range: NumericRange) -> Result<(), NotDerivable> { Ok(()) } } @@ -746,19 +812,22 @@ impl DerivableVisitor for DeriveDecoding { } #[inline(always)] - fn visit_recursion(_var: Variable) -> Result { + fn visit_recursion(_var: Variable) -> Result { Ok(Descend(true)) } #[inline(always)] - fn visit_apply(var: Variable, symbol: Symbol) -> Result { + fn visit_apply(var: Variable, symbol: Symbol) -> Result { if matches!( symbol, Symbol::LIST_LIST | Symbol::SET_SET | Symbol::DICT_DICT | Symbol::STR_STR, ) { Ok(Descend(true)) } else { - Err(DerivableError::NotDerivable(var)) + Err(NotDerivable { + var, + context: NotDerivableContext::NoContext, + }) } } @@ -767,46 +836,49 @@ impl DerivableVisitor for DeriveDecoding { subs: &Subs, var: Variable, fields: RecordFields, - ) -> Result { + ) -> Result { let has_optional_field = subs .get_subs_slice(fields.record_fields()) .iter() .any(|field| matches!(field, RecordField::Optional(..))); if has_optional_field { - Err(DerivableError::NotDerivable(var)) + Err(NotDerivable { + var, + context: NotDerivableContext::NoContext, + }) } else { Ok(Descend(true)) } } #[inline(always)] - fn visit_tag_union(_var: Variable) -> Result { + fn visit_tag_union(_var: Variable) -> Result { Ok(Descend(true)) } #[inline(always)] - fn visit_recursive_tag_union(_var: Variable) -> Result { + fn visit_recursive_tag_union(_var: Variable) -> Result { Ok(Descend(true)) } #[inline(always)] - fn visit_function_or_tag_union(_var: Variable) -> Result { + fn visit_function_or_tag_union(_var: Variable) -> Result { Ok(Descend(true)) } #[inline(always)] - fn visit_empty_record(_var: Variable) -> Result<(), DerivableError> { + fn visit_empty_record(_var: Variable) -> Result<(), NotDerivable> { Ok(()) } #[inline(always)] - fn visit_empty_tag_union(_var: Variable) -> Result<(), DerivableError> { + fn visit_empty_tag_union(_var: Variable) -> Result<(), NotDerivable> { Ok(()) } #[inline(always)] - fn visit_alias(_var: Variable, symbol: Symbol) -> Result { + fn visit_alias(_var: Variable, symbol: Symbol) -> Result { if is_builtin_number_alias(symbol) { Ok(Descend(false)) } else { @@ -815,7 +887,7 @@ impl DerivableVisitor for DeriveDecoding { } #[inline(always)] - fn visit_ranged_number(_var: Variable, _range: NumericRange) -> Result<(), DerivableError> { + fn visit_ranged_number(_var: Variable, _range: NumericRange) -> Result<(), NotDerivable> { Ok(()) } } diff --git a/crates/compiler/solve_problem/src/lib.rs b/crates/compiler/solve_problem/src/lib.rs index 1850e03464..fafd42b1b4 100644 --- a/crates/compiler/solve_problem/src/lib.rs +++ b/crates/compiler/solve_problem/src/lib.rs @@ -1,5 +1,5 @@ use roc_can::expected::{Expected, PExpected}; -use roc_module::symbol::Symbol; +use roc_module::{ident::Lowercase, symbol::Symbol}; use roc_problem::can::CycleEntry; use roc_region::all::Region; @@ -55,7 +55,21 @@ pub enum Unfulfilled { pub enum UnderivableReason { NotABuiltin, /// The surface type is not derivable - SurfaceNotDerivable, + SurfaceNotDerivable(NotDerivableContext), /// A nested type is not derivable - NestedNotDerivable(ErrorType), + NestedNotDerivable(ErrorType, NotDerivableContext), +} + +#[derive(PartialEq, Debug, Clone)] +pub enum NotDerivableContext { + NoContext, + Function, + UnboundVar, + Opaque(Symbol), + Decode(NotDerivableDecode), +} + +#[derive(PartialEq, Debug, Clone)] +pub enum NotDerivableDecode { + OptionalRecordField(Lowercase), } diff --git a/crates/reporting/Cargo.toml b/crates/reporting/Cargo.toml index fac8f7a8eb..6db85a74f4 100644 --- a/crates/reporting/Cargo.toml +++ b/crates/reporting/Cargo.toml @@ -7,6 +7,7 @@ edition = "2021" [dependencies] roc_collections = { path = "../compiler/collections" } +roc_error_macros = { path = "../error_macros" } roc_exhaustive = { path = "../compiler/exhaustive" } roc_region = { path = "../compiler/region" } roc_module = { path = "../compiler/module" } diff --git a/crates/reporting/src/error/type.rs b/crates/reporting/src/error/type.rs index de08bcf3f6..e51aea74a9 100644 --- a/crates/reporting/src/error/type.rs +++ b/crates/reporting/src/error/type.rs @@ -2,12 +2,15 @@ use crate::error::canonicalize::{to_circular_def_doc, CIRCULAR_DEF}; use crate::report::{Annotation, Report, RocDocAllocator, RocDocBuilder, Severity}; use roc_can::expected::{Expected, PExpected}; use roc_collections::all::{HumanIndex, MutSet, SendMap}; +use roc_error_macros::internal_error; use roc_exhaustive::CtorName; use roc_module::called_via::{BinOp, CalledVia}; use roc_module::ident::{Ident, IdentStr, Lowercase, TagName}; use roc_module::symbol::Symbol; use roc_region::all::{LineInfo, Loc, Region}; -use roc_solve_problem::{TypeError, UnderivableReason, Unfulfilled}; +use roc_solve_problem::{ + NotDerivableContext, NotDerivableDecode, TypeError, UnderivableReason, Unfulfilled, +}; use roc_std::RocDec; use roc_types::pretty_print::{Parens, WILDCARD}; use roc_types::types::{ @@ -334,9 +337,11 @@ fn report_underivable_reason<'a>( UnderivableReason::NotABuiltin => { Some(alloc.reflow("Only builtin abilities can have generated implementations!")) } - UnderivableReason::SurfaceNotDerivable => underivable_hint(alloc, ability, typ), - UnderivableReason::NestedNotDerivable(nested_typ) => { - let hint = underivable_hint(alloc, ability, &nested_typ); + UnderivableReason::SurfaceNotDerivable(context) => { + underivable_hint(alloc, ability, context, typ) + } + UnderivableReason::NestedNotDerivable(nested_typ, context) => { + let hint = underivable_hint(alloc, ability, context, &nested_typ); let reason = alloc.stack( [ alloc.reflow("In particular, an implementation for"), @@ -354,59 +359,80 @@ fn report_underivable_reason<'a>( fn underivable_hint<'b>( alloc: &'b RocDocAllocator<'b>, ability: Symbol, + context: NotDerivableContext, typ: &ErrorType, ) -> Option> { - match typ { - ErrorType::Function(..) => Some(alloc.note("").append(alloc.concat([ + match context { + NotDerivableContext::NoContext => None, + NotDerivableContext::Function => Some(alloc.note("").append(alloc.concat([ alloc.symbol_unqualified(ability), alloc.reflow(" cannot be generated for functions."), ]))), - ErrorType::FlexVar(v) | ErrorType::RigidVar(v) => Some(alloc.tip().append(alloc.concat([ - alloc.reflow("This type variable is not bound to "), + NotDerivableContext::Opaque(symbol) => Some(alloc.tip().append(alloc.concat([ + alloc.symbol_unqualified(symbol), + alloc.reflow(" does not implement "), alloc.symbol_unqualified(ability), - alloc.reflow(". Consider adding a "), - alloc.keyword("has"), - alloc.reflow(" clause to bind the type variable, like "), - alloc.inline_type_block(alloc.concat([ - alloc.string("| ".to_string()), - alloc.type_variable(v.clone()), - alloc.space(), - alloc.keyword("has"), - alloc.space(), - alloc.symbol_qualified(ability), - ])), + alloc.reflow("."), + if symbol.module_id() == alloc.home { + alloc.concat([ + alloc.reflow(" Consider adding a custom implementation"), + if ability.is_builtin() { + alloc.concat([ + alloc.reflow(" or "), + alloc.inline_type_block(alloc.concat([ + alloc.keyword("has"), + alloc.space(), + alloc.symbol_qualified(ability), + ])), + alloc.reflow(" to the definition of "), + alloc.symbol_unqualified(symbol), + ]) + } else { + alloc.nil() + }, + alloc.reflow("."), + ]) + } else { + alloc.nil() + }, ]))), - ErrorType::Alias(symbol, _, _, AliasKind::Opaque) => { + NotDerivableContext::UnboundVar => { + let v = match typ { + ErrorType::FlexVar(v) => v, + ErrorType::RigidVar(v) => v, + _ => internal_error!("unbound variable context only applicable for variables"), + }; + Some(alloc.tip().append(alloc.concat([ - alloc.symbol_unqualified(*symbol), - alloc.reflow(" does not implement "), + alloc.reflow("This type variable is not bound to "), alloc.symbol_unqualified(ability), - alloc.reflow("."), - if symbol.module_id() == alloc.home { - alloc.concat([ - alloc.reflow(" Consider adding a custom implementation"), - if ability.is_builtin() { - alloc.concat([ - alloc.reflow(" or "), - alloc.inline_type_block(alloc.concat([ - alloc.keyword("has"), - alloc.space(), - alloc.symbol_qualified(ability), - ])), - alloc.reflow(" to the definition of "), - alloc.symbol_unqualified(*symbol), - ]) - } else { - alloc.nil() - }, - alloc.reflow("."), - ]) - } else { - alloc.nil() - }, + alloc.reflow(". Consider adding a "), + alloc.keyword("has"), + alloc.reflow(" clause to bind the type variable, like "), + alloc.inline_type_block(alloc.concat([ + alloc.string("| ".to_string()), + alloc.type_variable(v.clone()), + alloc.space(), + alloc.keyword("has"), + alloc.space(), + alloc.symbol_qualified(ability), + ])), ]))) } - _ => None, + NotDerivableContext::Decode(reason) => match reason { + NotDerivableDecode::OptionalRecordField(field) => { + Some(alloc.note("").append(alloc.concat([ + alloc.reflow("Roc cannot derive decoding for a record with an optional field, which in this case is "), + alloc.record_field(field), + alloc.reflow(". Optional record fields are polymorphic over records that may or may not contain them at compile time, "), + alloc.reflow("but are not a concept that extends to runtime! That means Roc cannot derive a decoder for a record with an optional value "), + alloc.reflow("by way of an optional record field. If you want to model the idea that a field may or may not be present at runtime, "), + alloc.reflow("consider using a "), + alloc.symbol_unqualified(Symbol::RESULT_RESULT), + alloc.reflow("."), + ]))) + } + }, } } From e77e53f37b5bff99e8b427ef8ac11a49b7802aa2 Mon Sep 17 00:00:00 2001 From: Ayaz Hafiz Date: Tue, 9 Aug 2022 08:48:21 -0700 Subject: [PATCH 4/8] Enable optional record field underivable error --- crates/compiler/solve/src/ability.rs | 27 ++++++++++++++------------- crates/reporting/src/error/type.rs | 4 +++- 2 files changed, 17 insertions(+), 14 deletions(-) diff --git a/crates/compiler/solve/src/ability.rs b/crates/compiler/solve/src/ability.rs index bc4bfdb794..f63ebd414c 100644 --- a/crates/compiler/solve/src/ability.rs +++ b/crates/compiler/solve/src/ability.rs @@ -4,7 +4,9 @@ use roc_collections::{VecMap, VecSet}; use roc_error_macros::{internal_error, todo_abilities}; use roc_module::symbol::Symbol; use roc_region::all::{Loc, Region}; -use roc_solve_problem::{NotDerivableContext, TypeError, UnderivableReason, Unfulfilled}; +use roc_solve_problem::{ + NotDerivableContext, NotDerivableDecode, TypeError, UnderivableReason, Unfulfilled, +}; use roc_types::num::NumericRange; use roc_types::subs::{ instantiate_rigids, Content, FlatType, GetSubsSlice, Rank, RecordFields, Subs, Variable, @@ -837,19 +839,18 @@ impl DerivableVisitor for DeriveDecoding { var: Variable, fields: RecordFields, ) -> Result { - let has_optional_field = subs - .get_subs_slice(fields.record_fields()) - .iter() - .any(|field| matches!(field, RecordField::Optional(..))); - - if has_optional_field { - Err(NotDerivable { - var, - context: NotDerivableContext::NoContext, - }) - } else { - Ok(Descend(true)) + for (field_name, _, field) in fields.iter_all() { + if matches!(subs[field], RecordField::Optional(..)) { + return Err(NotDerivable { + var, + context: NotDerivableContext::Decode(NotDerivableDecode::OptionalRecordField( + subs[field_name].clone(), + )), + }); + } } + + Ok(Descend(true)) } #[inline(always)] diff --git a/crates/reporting/src/error/type.rs b/crates/reporting/src/error/type.rs index e51aea74a9..5f47d40762 100644 --- a/crates/reporting/src/error/type.rs +++ b/crates/reporting/src/error/type.rs @@ -425,7 +425,9 @@ fn underivable_hint<'b>( alloc.reflow("Roc cannot derive decoding for a record with an optional field, which in this case is "), alloc.record_field(field), alloc.reflow(". Optional record fields are polymorphic over records that may or may not contain them at compile time, "), - alloc.reflow("but are not a concept that extends to runtime! That means Roc cannot derive a decoder for a record with an optional value "), + alloc.reflow("but are not a concept that extends to runtime!"), + alloc.hardline(), + alloc.reflow("That means Roc cannot derive a decoder for a record with an optional value "), alloc.reflow("by way of an optional record field. If you want to model the idea that a field may or may not be present at runtime, "), alloc.reflow("consider using a "), alloc.symbol_unqualified(Symbol::RESULT_RESULT), From cfc46c05aecd875d7610975cb9f77aa453679ce5 Mon Sep 17 00:00:00 2001 From: Ayaz Hafiz Date: Tue, 9 Aug 2022 08:49:38 -0700 Subject: [PATCH 5/8] Turn on reporting test that works now --- crates/reporting/tests/test_reporting.rs | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/crates/reporting/tests/test_reporting.rs b/crates/reporting/tests/test_reporting.rs index 1026f10a8f..f8591ecee8 100644 --- a/crates/reporting/tests/test_reporting.rs +++ b/crates/reporting/tests/test_reporting.rs @@ -8537,7 +8537,6 @@ All branches in an `if` must have the same type! ); test_report!( - #[ignore = "TODO does not error yet"] ability_specialization_is_duplicated_with_type_mismatch, indoc!( r#" @@ -8553,6 +8552,18 @@ All branches in an `if` must have the same type! "# ), @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! "### ); From 92ce0c0662528b04bc42a61c846026437b7e54bd Mon Sep 17 00:00:00 2001 From: Ayaz Hafiz Date: Mon, 15 Aug 2022 11:18:45 -0500 Subject: [PATCH 6/8] Fix opaque typo --- crates/reporting/src/error/canonicalize.rs | 2 +- crates/reporting/tests/test_reporting.rs | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/crates/reporting/src/error/canonicalize.rs b/crates/reporting/src/error/canonicalize.rs index 1f805aec8d..43deb099e9 100644 --- a/crates/reporting/src/error/canonicalize.rs +++ b/crates/reporting/src/error/canonicalize.rs @@ -972,7 +972,7 @@ pub fn can_problem<'b>( 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!"), + alloc.reflow("Ability specializations can only provide implementations for one opaque type, since all opaque types are different!"), ]); title = "OVERLOADED SPECIALIZATION".to_string(); severity = Severity::Warning; diff --git a/crates/reporting/tests/test_reporting.rs b/crates/reporting/tests/test_reporting.rs index f8591ecee8..999decb35c 100644 --- a/crates/reporting/tests/test_reporting.rs +++ b/crates/reporting/tests/test_reporting.rs @@ -8510,7 +8510,7 @@ All branches in an `if` must have the same type! 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! + opaque type, since all opaque types are different! ── TYPE MISMATCH ───────────────────────────────────────── /code/proj/Main.roc ─ @@ -8563,7 +8563,7 @@ All branches in an `if` must have the same type! 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! + opaque type, since all opaque types are different! "### ); From d016d5eeb9f8d011a42b4a47a22f1ad9e66c89d7 Mon Sep 17 00:00:00 2001 From: Ayaz Hafiz Date: Mon, 15 Aug 2022 11:22:58 -0500 Subject: [PATCH 7/8] Refine message --- crates/reporting/src/error/type.rs | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/crates/reporting/src/error/type.rs b/crates/reporting/src/error/type.rs index 5f47d40762..f1844f2279 100644 --- a/crates/reporting/src/error/type.rs +++ b/crates/reporting/src/error/type.rs @@ -422,16 +422,14 @@ fn underivable_hint<'b>( NotDerivableContext::Decode(reason) => match reason { NotDerivableDecode::OptionalRecordField(field) => { Some(alloc.note("").append(alloc.concat([ - alloc.reflow("Roc cannot derive decoding for a record with an optional field, which in this case is "), + alloc.reflow("I can't derive decoding for a record with an optional field, which in this case is "), alloc.record_field(field), alloc.reflow(". Optional record fields are polymorphic over records that may or may not contain them at compile time, "), alloc.reflow("but are not a concept that extends to runtime!"), alloc.hardline(), - alloc.reflow("That means Roc cannot derive a decoder for a record with an optional value "), - alloc.reflow("by way of an optional record field. If you want to model the idea that a field may or may not be present at runtime, "), - alloc.reflow("consider using a "), + alloc.reflow("Maybe you wanted to use a "), alloc.symbol_unqualified(Symbol::RESULT_RESULT), - alloc.reflow("."), + alloc.reflow("?"), ]))) } }, From a1d8f8392aa57f5fd03c6cccdc5a6a2cdd0b2882 Mon Sep 17 00:00:00 2001 From: Ayaz Hafiz Date: Sat, 20 Aug 2022 10:11:28 -0500 Subject: [PATCH 8/8] Include RigidOptional as optional field --- crates/compiler/solve/src/ability.rs | 4 ++-- crates/compiler/types/src/types.rs | 5 ++++- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/crates/compiler/solve/src/ability.rs b/crates/compiler/solve/src/ability.rs index f63ebd414c..d25fe688e5 100644 --- a/crates/compiler/solve/src/ability.rs +++ b/crates/compiler/solve/src/ability.rs @@ -11,7 +11,7 @@ use roc_types::num::NumericRange; use roc_types::subs::{ instantiate_rigids, Content, FlatType, GetSubsSlice, Rank, RecordFields, Subs, Variable, }; -use roc_types::types::{AliasKind, Category, MemberImpl, PatternCategory, RecordField}; +use roc_types::types::{AliasKind, Category, MemberImpl, PatternCategory}; use roc_unify::unify::{Env, MustImplementConstraints}; use roc_unify::unify::{MustImplementAbility, Obligated}; @@ -840,7 +840,7 @@ impl DerivableVisitor for DeriveDecoding { fields: RecordFields, ) -> Result { for (field_name, _, field) in fields.iter_all() { - if matches!(subs[field], RecordField::Optional(..)) { + if subs[field].is_optional() { return Err(NotDerivable { var, context: NotDerivableContext::Decode(NotDerivableDecode::OptionalRecordField( diff --git a/crates/compiler/types/src/types.rs b/crates/compiler/types/src/types.rs index cd4641dca5..e4da101a69 100644 --- a/crates/compiler/types/src/types.rs +++ b/crates/compiler/types/src/types.rs @@ -104,7 +104,10 @@ impl RecordField { } pub fn is_optional(&self) -> bool { - matches!(self, RecordField::Optional(..)) + matches!( + self, + RecordField::Optional(..) | RecordField::RigidOptional(..) + ) } }