Fix double-wrapping of spaces before implements opaque keyword by making them distinct

crates/compiler/parse/src/ast.rs

@@ -841,7 +841,7 @@ pub enum TypeDef<'a> {
     Opaque {
         header: TypeHeader<'a>,
         typ: Loc<TypeAnnotation<'a>>,
-        derived: Option<Loc<ImplementsAbilities<'a>>>,
+        derived: Option<&'a ImplementsAbilities<'a>>,
     },
 
     /// An ability definition. E.g.
@@ -1552,31 +1552,11 @@ pub enum ImplementsAbility<'a> {
 }
 
 #[derive(Debug, Copy, Clone, PartialEq)]
-pub enum ImplementsAbilities<'a> {
-    /// `implements [Eq { eq: myEq }, Hash]`
-    Implements(Collection<'a, Loc<ImplementsAbility<'a>>>),
-
-    // We preserve this for the formatter; canonicalization ignores it.
-    SpaceBefore(&'a ImplementsAbilities<'a>, &'a [CommentOrNewline<'a>]),
-    SpaceAfter(&'a ImplementsAbilities<'a>, &'a [CommentOrNewline<'a>]),
-}
-
-impl ImplementsAbilities<'_> {
-    pub fn collection(&self) -> &Collection<Loc<ImplementsAbility>> {
-        let mut it = self;
-        loop {
-            match it {
-                Self::SpaceBefore(inner, _) | Self::SpaceAfter(inner, _) => {
-                    it = inner;
-                }
-                Self::Implements(collection) => return collection,
-            }
-        }
-    }
-
-    pub fn is_empty(&self) -> bool {
-        self.collection().is_empty()
-    }
+pub struct ImplementsAbilities<'a> {
+    pub before_implements_kw: &'a [CommentOrNewline<'a>],
+    pub implements: Region,
+    pub after_implements_kw: &'a [CommentOrNewline<'a>],
+    pub item: Loc<Collection<'a, Loc<ImplementsAbility<'a>>>>,
 }
 
 #[derive(Debug, Copy, Clone, PartialEq)]
@@ -2278,15 +2258,6 @@ impl<'a> Spaceable<'a> for ImplementsAbility<'a> {
     }
 }
 
-impl<'a> Spaceable<'a> for ImplementsAbilities<'a> {
-    fn before(&'a self, spaces: &'a [CommentOrNewline<'a>]) -> Self {
-        ImplementsAbilities::SpaceBefore(self, spaces)
-    }
-    fn after(&'a self, spaces: &'a [CommentOrNewline<'a>]) -> Self {
-        ImplementsAbilities::SpaceAfter(self, spaces)
-    }
-}
-
 impl<'a> Expr<'a> {
     pub const REPL_OPAQUE_FUNCTION: Self = Expr::Var {
         module_name: "",
@@ -2396,7 +2367,6 @@ impl_extract_spaces!(Tag);
 impl_extract_spaces!(AssignedField<T>);
 impl_extract_spaces!(TypeAnnotation);
 impl_extract_spaces!(ImplementsAbility);
-impl_extract_spaces!(ImplementsAbilities);
 impl_extract_spaces!(Implements);
 
 impl<'a, T: Copy> ExtractSpaces<'a> for Spaced<'a, T> {
@@ -2720,7 +2690,11 @@ impl<'a> Malformed for TypeDef<'a> {
                 header,
                 typ,
                 derived,
-            } => header.is_malformed() || typ.is_malformed() || derived.is_malformed(),
+            } => {
+                header.is_malformed()
+                    || typ.is_malformed()
+                    || derived.map(|d| d.item.is_malformed()).unwrap_or_default()
+            }
             TypeDef::Ability {
                 header,
                 loc_implements,
@@ -2762,19 +2736,6 @@ impl<'a> Malformed for ImplementsAbility<'a> {
     }
 }
 
-impl<'a> Malformed for ImplementsAbilities<'a> {
-    fn is_malformed(&self) -> bool {
-        match self {
-            ImplementsAbilities::Implements(abilities) => {
-                abilities.iter().any(|ability| ability.is_malformed())
-            }
-            ImplementsAbilities::SpaceBefore(has, _) | ImplementsAbilities::SpaceAfter(has, _) => {
-                has.is_malformed()
-            }
-        }
-    }
-}
-
 impl<'a> Malformed for AbilityImpls<'a> {
     fn is_malformed(&self) -> bool {
         match self {

crates/compiler/parse/src/blankspace.rs

@@ -1,5 +1,6 @@
 use crate::ast::CommentOrNewline;
 use crate::ast::Spaceable;
+use crate::ast::SpacesBefore;
 use crate::parser::succeed;
 use crate::parser::Progress;
 use crate::parser::SpaceProblem;
@@ -175,6 +176,24 @@ where
     )
 }
 
+pub fn plain_spaces_before<'a, P, S, E>(
+    parser: P,
+    indent_problem: fn(Position) -> E,
+) -> impl Parser<'a, SpacesBefore<'a, S>, E>
+where
+    S: 'a,
+    P: 'a + Parser<'a, S, E>,
+    E: 'a + SpaceProblem,
+{
+    parser::map(
+        and(space0_e(indent_problem), parser),
+        |(space_list, item): (&'a [CommentOrNewline<'a>], S)| SpacesBefore {
+            before: space_list,
+            item,
+        },
+    )
+}
+
 pub fn space0_after_e<'a, P, S, E>(
     parser: P,
     indent_problem: fn(Position) -> E,

crates/compiler/parse/src/expr.rs

@@ -1158,20 +1158,17 @@ fn alias_signature<'a>() -> impl Parser<'a, Loc<TypeAnnotation<'a>>, EExpr<'a>>
     increment_min_indent(specialize_err(EExpr::Type, type_annotation::located(false)))
 }
 
-fn opaque_signature<'a>() -> impl Parser<
-    'a,
-    (
-        Loc<TypeAnnotation<'a>>,
-        Option<Loc<ImplementsAbilities<'a>>>,
-    ),
-    EExpr<'a>,
-> {
+fn opaque_signature<'a>(
+) -> impl Parser<'a, (Loc<TypeAnnotation<'a>>, Option<&'a ImplementsAbilities<'a>>), EExpr<'a>> {
     and(
         specialize_err(EExpr::Type, type_annotation::located_opaque_signature(true)),
-        optional(backtrackable(specialize_err(
-            EExpr::Type,
-            space0_before_e(type_annotation::implements_abilities(), EType::TIndentStart),
-        ))),
+        optional(map_with_arena(
+            backtrackable(specialize_err(
+                EExpr::Type,
+                type_annotation::implements_abilities(),
+            )),
+            |arena, item| &*arena.alloc(item),
+        )),
     )
 }
 

crates/compiler/parse/src/normalize.rs

@@ -3,14 +3,14 @@ use bumpalo::Bump;
 use roc_module::called_via::{BinOp, UnaryOp};
 use roc_region::all::{Loc, Position, Region};
 
+use crate::ast::ImplementsAbilities;
 use crate::{
     ast::{
         AbilityImpls, AbilityMember, AssignedField, Collection, Defs, Expr, FullAst, Header,
-        Implements, ImplementsAbilities, ImplementsAbility, ImplementsClause, ImportAlias,
-        ImportAsKeyword, ImportExposingKeyword, ImportedModuleName, IngestedFileAnnotation,
-        IngestedFileImport, ModuleImport, ModuleImportParams, Pattern, PatternAs, Spaced, Spaces,
-        SpacesBefore, StrLiteral, StrSegment, Tag, TypeAnnotation, TypeDef, TypeHeader, ValueDef,
-        WhenBranch,
+        Implements, ImplementsAbility, ImplementsClause, ImportAlias, ImportAsKeyword,
+        ImportExposingKeyword, ImportedModuleName, IngestedFileAnnotation, IngestedFileImport,
+        ModuleImport, ModuleImportParams, Pattern, PatternAs, Spaced, Spaces, SpacesBefore,
+        StrLiteral, StrSegment, Tag, TypeAnnotation, TypeDef, TypeHeader, ValueDef, WhenBranch,
     },
     header::{
         AppHeader, ExposedName, ExposesKeyword, HostedHeader, ImportsEntry, ImportsKeyword,
@@ -372,7 +372,7 @@ impl<'a> Normalize<'a> for TypeDef<'a> {
                     vars: vars.normalize(arena),
                 },
                 typ: typ.normalize(arena),
-                derived: derived.normalize(arena),
+                derived: derived.map(|item| &*arena.alloc(item.normalize(arena))),
             },
             Ability {
                 header: TypeHeader { name, vars },
@@ -1001,6 +1001,17 @@ impl<'a> Normalize<'a> for AbilityImpls<'a> {
     }
 }
 
+impl<'a> Normalize<'a> for ImplementsAbilities<'a> {
+    fn normalize(&self, arena: &'a Bump) -> Self {
+        ImplementsAbilities {
+            before_implements_kw: &[],
+            implements: Region::zero(),
+            after_implements_kw: &[],
+            item: self.item.normalize(arena),
+        }
+    }
+}
+
 impl<'a> Normalize<'a> for ImplementsAbility<'a> {
     fn normalize(&self, arena: &'a Bump) -> Self {
         match *self {
@@ -1017,18 +1028,6 @@ impl<'a> Normalize<'a> for ImplementsAbility<'a> {
     }
 }
 
-impl<'a> Normalize<'a> for ImplementsAbilities<'a> {
-    fn normalize(&self, arena: &'a Bump) -> Self {
-        match *self {
-            ImplementsAbilities::Implements(derived) => {
-                ImplementsAbilities::Implements(derived.normalize(arena))
-            }
-            ImplementsAbilities::SpaceBefore(derived, _)
-            | ImplementsAbilities::SpaceAfter(derived, _) => derived.normalize(arena),
-        }
-    }
-}
-
 impl<'a> Normalize<'a> for PatternAs<'a> {
     fn normalize(&self, arena: &'a Bump) -> Self {
         PatternAs {

crates/compiler/parse/src/type_annotation.rs

@@ -4,8 +4,8 @@ use crate::ast::{
     TypeAnnotation, TypeHeader,
 };
 use crate::blankspace::{
-    self, space0_around_ee, space0_before_e, space0_before_optional_after, space0_e,
-    spaces_before_optional_after,
+    self, plain_spaces_before, space0_around_ee, space0_before_e, space0_before_optional_after,
+    space0_e, spaces_before_optional_after,
 };
 use crate::expr::record_field;
 use crate::ident::{lowercase_ident, lowercase_ident_keyword_e};
@@ -784,25 +784,39 @@ fn parse_implements_clause_chain_after_where<'a>(
 }
 
 /// Parse a implements-abilities clause, e.g. `implements [Eq, Hash]`.
-pub fn implements_abilities<'a>() -> impl Parser<'a, Loc<ImplementsAbilities<'a>>, EType<'a>> {
-    increment_min_indent(skip_first(
-        // Parse "implements"; we don't care about this keyword
-        crate::parser::keyword(crate::keyword::IMPLEMENTS, EType::TImplementsClause),
-        // Parse "Hash"; this may be qualified from another module like "Hash.Hash"
-        space0_before_e(
-            loc(map(
-                collection_trailing_sep_e(
-                    byte(b'[', EType::TStart),
-                    loc(parse_implements_ability()),
-                    byte(b',', EType::TEnd),
-                    byte(b']', EType::TEnd),
-                    ImplementsAbility::SpaceBefore,
+pub fn implements_abilities<'a>() -> impl Parser<'a, ImplementsAbilities<'a>, EType<'a>> {
+    map(
+        plain_spaces_before(
+            increment_min_indent(and(
+                // Parse "implements"; we don't care about this keyword
+                map(
+                    loc(crate::parser::keyword(
+                        crate::keyword::IMPLEMENTS,
+                        EType::TImplementsClause,
+                    )),
+                    |item| item.region,
+                ),
+                // Parse "Hash"; this may be qualified from another module like "Hash.Hash"
+                plain_spaces_before(
+                    loc(collection_trailing_sep_e(
+                        byte(b'[', EType::TStart),
+                        loc(parse_implements_ability()),
+                        byte(b',', EType::TEnd),
+                        byte(b']', EType::TEnd),
+                        ImplementsAbility::SpaceBefore,
+                    )),
+                    EType::TIndentEnd,
                 ),
-                ImplementsAbilities::Implements,
             )),
-            EType::TIndentEnd,
+            EType::TIndentStart,
         ),
-    ))
+        |item| ImplementsAbilities {
+            before_implements_kw: item.before,
+            implements: item.item.0,
+            after_implements_kw: item.item.1.before,
+            item: item.item.1.item,
+        },
+    )
 }
 
 fn parse_implements_ability<'a>() -> impl Parser<'a, ImplementsAbility<'a>, EType<'a>> {