From f129777115963b9829f78e181ba775e6d9c49ddb Mon Sep 17 00:00:00 2001 From: Ayaz Hafiz Date: Mon, 18 Apr 2022 18:04:46 -0400 Subject: [PATCH] Explicitly disallow ability definitions in nested scopes Abilities can only be defined on the toplevel of a module. There is a technical reason to this, which is that during type solving we must introduce all abilities at the very beginning, and we need to make sure ranks are correct. But there is a practical reason as well, which is that nested ability definitions don't seem to be very useful. Note that specializations can be nested, and are allowed to be. Also, we can revisit this in the future. I just don't want experiments to break right now because someone uses an ability in a nested scope where we don't expect. Closes #2878 --- compiler/can/src/def.rs | 24 ++++++++-- compiler/parse/src/ast.rs | 6 +++ compiler/problem/src/can.rs | 3 ++ reporting/src/error/canonicalize.rs | 13 +++++ reporting/tests/test_reporting.rs | 74 ++++++++++++++++++----------- 5 files changed, 89 insertions(+), 31 deletions(-) diff --git a/compiler/can/src/def.rs b/compiler/can/src/def.rs index cd4d2c79d5..2cc1cd85ae 100644 --- a/compiler/can/src/def.rs +++ b/compiler/can/src/def.rs @@ -241,10 +241,12 @@ pub fn canonicalize_defs<'a>( let pending_type_defs = type_defs .into_iter() .filter_map(|loc_def| { - to_pending_type_def(env, loc_def.value, &mut scope).map(|(new_output, pending_def)| { - output.union(new_output); - pending_def - }) + to_pending_type_def(env, loc_def.value, &mut scope, pattern_type).map( + |(new_output, pending_def)| { + output.union(new_output); + pending_def + }, + ) }) .collect::>(); @@ -1679,6 +1681,7 @@ fn to_pending_type_def<'a>( env: &mut Env<'a>, def: &'a ast::TypeDef<'a>, scope: &mut Scope, + pattern_type: PatternType, ) -> Option<(Output, PendingTypeDef<'a>)> { use ast::TypeDef::*; @@ -1762,6 +1765,19 @@ fn to_pending_type_def<'a>( } } + Ability { + header, members, .. + } if pattern_type != PatternType::TopLevelDef => { + let header_region = header.region(); + let region = Region::span_across( + &header_region, + &members.last().map(|m| m.region()).unwrap_or(header_region), + ); + env.problem(Problem::AbilityNotOnToplevel { region }); + + Some((Output::default(), PendingTypeDef::InvalidAbility)) + } + Ability { header: TypeHeader { name, vars }, members, diff --git a/compiler/parse/src/ast.rs b/compiler/parse/src/ast.rs index f616f07d7f..0c086b32b7 100644 --- a/compiler/parse/src/ast.rs +++ b/compiler/parse/src/ast.rs @@ -278,6 +278,12 @@ pub struct AbilityMember<'a> { pub typ: Loc>, } +impl AbilityMember<'_> { + pub fn region(&self) -> Region { + Region::across_all([self.name.region, self.typ.region].iter()) + } +} + #[derive(Debug, Clone, Copy, PartialEq)] pub enum TypeDef<'a> { /// A type alias. This is like a standalone annotation, except the pattern diff --git a/compiler/problem/src/can.rs b/compiler/problem/src/can.rs index f8d1684418..a12584c580 100644 --- a/compiler/problem/src/can.rs +++ b/compiler/problem/src/can.rs @@ -135,6 +135,9 @@ pub enum Problem { loc_name: Loc, ability: Symbol, }, + AbilityNotOnToplevel { + region: Region, + }, } #[derive(Clone, Debug, PartialEq)] diff --git a/reporting/src/error/canonicalize.rs b/reporting/src/error/canonicalize.rs index d366baf6f2..33962e08ab 100644 --- a/reporting/src/error/canonicalize.rs +++ b/reporting/src/error/canonicalize.rs @@ -45,6 +45,7 @@ const ILLEGAL_HAS_CLAUSE: &str = "ILLEGAL HAS CLAUSE"; const ABILITY_MEMBER_MISSING_HAS_CLAUSE: &str = "ABILITY MEMBER MISSING HAS CLAUSE"; const ABILITY_MEMBER_HAS_EXTRANEOUS_HAS_CLAUSE: &str = "ABILITY MEMBER HAS EXTRANEOUS HAS CLAUSE"; const ABILITY_MEMBER_BINDS_MULTIPLE_VARIABLES: &str = "ABILITY MEMBER BINDS MULTIPLE VARIABLES"; +const ABILITY_NOT_ON_TOPLEVEL: &str = "ABILITY NOT ON TOP-LEVEL"; pub fn can_problem<'b>( alloc: &'b RocDocAllocator<'b>, @@ -736,6 +737,18 @@ pub fn can_problem<'b>( title = ABILITY_MEMBER_HAS_EXTRANEOUS_HAS_CLAUSE.to_string(); severity = Severity::RuntimeError; } + + Problem::AbilityNotOnToplevel { region } => { + doc = alloc.stack(vec![ + alloc.concat(vec![alloc.reflow( + "This ability definition is not on the top-level of a module:", + )]), + alloc.region(lines.convert_region(region)), + alloc.reflow("Abilities can only be defined on the top-level of a Roc module."), + ]); + title = ABILITY_NOT_ON_TOPLEVEL.to_string(); + severity = Severity::RuntimeError; + } }; Report { diff --git a/reporting/tests/test_reporting.rs b/reporting/tests/test_reporting.rs index 265edb773b..712befac26 100644 --- a/reporting/tests/test_reporting.rs +++ b/reporting/tests/test_reporting.rs @@ -9137,10 +9137,10 @@ I need all branches in an `if` to have the same type! new_report_problem_as( indoc!( r#" + app "test" provides [] to "./platform" + Hash a b c has hash : a -> U64 | a has Hash - - 1 "# ), indoc!( @@ -9149,8 +9149,8 @@ I need all branches in an `if` to have the same type! The definition of the `Hash` ability includes type variables: - 4│ Hash a b c has - ^^^^^ + 3│ Hash a b c has + ^^^^^ Abilities cannot depend on type variables, but their member values can! @@ -9159,8 +9159,8 @@ I need all branches in an `if` to have the same type! `Hash` is not used anywhere in your code. - 4│ Hash a b c has - ^^^^ + 3│ Hash a b c has + ^^^^ If you didn't intend on using `Hash` then remove it so future readers of your code don't wonder why it is there. @@ -9228,12 +9228,13 @@ I need all branches in an `if` to have the same type! new_report_problem_as( indoc!( r#" + app "test" provides [ a ] to "./platform" + Ability has ab : a -> {} | a has Ability Alias : Ability a : Alias - a "# ), indoc!( @@ -9242,8 +9243,8 @@ I need all branches in an `if` to have the same type! The definition of the `Alias` aliases references the ability `Ability`: - 6│ Alias : Ability - ^^^^^ + 5│ Alias : Ability + ^^^^^ Abilities are not types, but you can add an ability constraint to a type variable `a` by writing @@ -9256,8 +9257,8 @@ I need all branches in an `if` to have the same type! `ab` is not used anywhere in your code. - 4│ Ability has ab : a -> {} | a has Ability - ^^ + 3│ Ability has ab : a -> {} | a has Ability + ^^ If you didn't intend on using `ab` then remove it so future readers of your code don't wonder why it is there. @@ -9271,11 +9272,11 @@ I need all branches in an `if` to have the same type! new_report_problem_as( indoc!( r#" + app "test" provides [ ab ] to "./platform" + Ability has ab : a -> U64 | a has Ability Ability has ab1 : a -> U64 | a has Ability - - 1 "# ), indoc!( @@ -9284,26 +9285,16 @@ I need all branches in an `if` to have the same type! The `Ability` name is first defined here: - 4│ Ability has ab : a -> U64 | a has Ability - ^^^^^^^ + 3│ Ability has ab : a -> U64 | a has Ability + ^^^^^^^ But then it's defined a second time here: - 6│ Ability has ab1 : a -> U64 | a has Ability - ^^^^^^^ + 5│ Ability has ab1 : a -> U64 | a has Ability + ^^^^^^^ Since these abilities have the same name, it's easy to use the wrong one on accident. Give one of them a new name. - - ── UNUSED DEFINITION ─────────────────────────────────────────────────────────── - - `ab` is not used anywhere in your code. - - 4│ Ability has ab : a -> U64 | a has Ability - ^^ - - If you didn't intend on using `ab` then remove it so future readers of - your code don't wonder why it is there. "# ), ) @@ -9783,4 +9774,33 @@ I need all branches in an `if` to have the same type! ), ) } + + #[test] + fn ability_not_on_toplevel() { + new_report_problem_as( + indoc!( + r#" + app "test" provides [ main ] to "./platform" + + main = + Hash has + hash : a -> U64 | a has Hash + + 123 + "# + ), + indoc!( + r#" + ── ABILITY NOT ON TOP-LEVEL ──────────────────────────────────────────────────── + + This ability definition is not on the top-level of a module: + + 4│> Hash has + 5│> hash : a -> U64 | a has Hash + + Abilities can only be defined on the top-level of a Roc module. + "# + ), + ) + } }