Repository: geldata/gel
Pull request: Remove type simplification from schema.
close #7221

Make type simplification for schema utils `ensure_union_type` and `ensure_intersection_type` explicit. Remove the type simplification when generating types from a user schema.

When creating types from a schema, the implicit simplification can cause an error when the name of the simplified type differs from the non-simplified type. This can occur if the type of a link is the union of a type and a base type (eg. `type A; type A_ extending A;` `link foo -> A | A_`).

Problem statement:
Property or link with union type of type and a derived type produces error
Given the schema
```
  type A;
  type A_ extending A;

  type Bad {
    property foo -> A | A_;
  };
```

The following error is produced:
```
SchemaError: object type 'default::A' already exists
```

Given the schema
```
  type A;
  type A_ extending A;
  type B;

  type Bad {
    property foo -> A | A_ | B;
  };
```

The following error is produced:
```
error: type 'default::(default:A | default:A_ | default:B)' does not exist
   │
22 │     property foo -> A | A_ | B;
   │                     ^^^^^^^^^^ error
```

Hints from issue discussion:

Commit message: Add test.

Patch to review:
diff --git a/edb/edgeql/compiler/schemactx.py b/edb/edgeql/compiler/schemactx.py
index 48b952aef9c..3aa74a3ddf4 100644
--- a/edb/edgeql/compiler/schemactx.py
+++ b/edb/edgeql/compiler/schemactx.py
@@ -396,16 +396,26 @@ def derive_view_name(
 
 
 def get_union_type(
-    types: Iterable[s_types.TypeT],
+    types: Sequence[s_types.TypeT],
     *,
     opaque: bool = False,
     preserve_derived: bool = False,
     ctx: context.ContextLevel,
 ) -> s_types.TypeT:
 
+    targets: Sequence[s_types.Type]
+    if preserve_derived:
+        targets = s_utils.simplify_union_types_preserve_derived(
+            ctx.env.schema, types
+        )
+    else:
+        targets = s_utils.simplify_union_types(
+            ctx.env.schema, types
+        )
+
     ctx.env.schema, union, created = s_utils.ensure_union_type(
-        ctx.env.schema, types,
-        opaque=opaque, preserve_derived=preserve_derived, transient=True)
+        ctx.env.schema, targets,
+        opaque=opaque, transient=True)
 
     if created:
         ctx.env.created_schema_objects.add(union)
@@ -422,13 +432,15 @@ def get_union_type(
 
 
 def get_intersection_type(
-    types: Iterable[s_types.TypeT],
+    types: Sequence[s_types.TypeT],
     *,
     ctx: context.ContextLevel,
 ) -> s_types.TypeT:
 
+    targets: Sequence[s_types.Type]
+    targets = s_utils.simplify_intersection_types(ctx.env.schema, types)
     ctx.env.schema, intersection, created = s_utils.ensure_intersection_type(
-        ctx.env.schema, types, transient=True)
+        ctx.env.schema, targets, transient=True)
 
     if created:
         ctx.env.created_schema_objects.add(intersection)
diff --git a/edb/schema/objtypes.py b/edb/schema/objtypes.py
index bbf8b3da3f0..c912092df9b 100644
--- a/edb/schema/objtypes.py
+++ b/edb/schema/objtypes.py
@@ -640,9 +640,12 @@ def _alter_finalize(
                 # them in the schema to produce the correct as_alter_delta.
                 nschema = _delete_to_delist(delete, schema)
 
-                nschema, nunion = utils.get_union_type(
+                types = utils.simplify_union_types(
+                    schema, union.get_union_of(schema).objects(schema)
+                )
+                nschema, nunion, _ = utils.ensure_union_type(
                     nschema,
-                    types=union.get_union_of(schema).objects(schema),
+                    types=types,
                     opaque=union.get_is_opaque_union(schema),
                     module=union.get_name(schema).module,
                 )
diff --git a/edb/schema/pointers.py b/edb/schema/pointers.py
index e958025b004..35095be1c7b 100644
--- a/edb/schema/pointers.py
+++ b/edb/schema/pointers.py
@@ -3139,8 +3139,9 @@ def get_or_create_union_pointer(
 
     far_endpoints = [p.get_far_endpoint(schema, direction)
                      for p in components]
-    targets: List[s_types.Type] = [p for p in far_endpoints
+    targets: Sequence[s_types.Type] = [p for p in far_endpoints
                                    if isinstance(p, s_types.Type)]
+    targets = utils.simplify_union_types(schema, targets)
 
     target: s_types.Type
 
@@ -3219,8 +3220,10 @@ def get_or_create_intersection_pointer(
     if len(components) == 1:
         return schema, components[0]
 
+    targets: Sequence[s_types.Type]
     targets = list(filter(None, [p.get_target(schema) for p in components]))
-    schema, target = utils.get_intersection_type(
+    targets = utils.simplify_intersection_types(schema, targets)
+    schema, target, _ = utils.ensure_intersection_type(
         schema, targets, module=modname)
 
     cardinality = qltypes.SchemaCardinality.One
diff --git a/edb/schema/types.py b/edb/schema/types.py
index 88bdf0f0e6e..005942fc543 100644
--- a/edb/schema/types.py
+++ b/edb/schema/types.py
@@ -876,25 +876,28 @@ def apply(
         context: sd.CommandContext,
     ) -> s_schema.Schema:
 
+        from edb.schema import types as s_types
+
         if not context.canonical:
-            components = [
+            components: Sequence[s_types.Type] = [
                 c.resolve(schema)
                 for c in self.get_attribute_value('components')
             ]
 
-            new_schema, union_type = utils.get_union_type(
+            new_schema, union_type, created = utils.ensure_union_type(
                 schema,
                 components,
                 opaque=self.get_attribute_value('is_opaque_union') or False,
                 module=self.classname.module,
             )
 
-            delta = union_type.as_create_delta(
-                schema=new_schema,
-                context=so.ComparisonContext(),
-            )
+            if created:
+                delta = union_type.as_create_delta(
+                    schema=new_schema,
+                    context=so.ComparisonContext(),
+                )
 
-            self.add(delta)
+                self.add(delta)
 
         for cmd in self.get_subcommands():
             schema = cmd.apply(schema, context)
diff --git a/edb/schema/utils.py b/edb/schema/utils.py
index 7574e7e12de..899e23a6b03 100644
--- a/edb/schema/utils.py
+++ b/edb/schema/utils.py
@@ -1020,83 +1020,48 @@ def enrich_schema_lookup_error(
 
 def ensure_union_type(
     schema: s_schema.Schema,
-    types: Iterable[s_types.Type],
+    types: Sequence[s_types.Type],
     *,
     opaque: bool = False,
     module: Optional[str] = None,
-    preserve_derived: bool = False,
     transient: bool = False,
 ) -> Tuple[s_schema.Schema, s_types.Type, bool]:
 
     from edb.schema import objtypes as s_objtypes
-    from edb.schema import types as s_types
-
-    type_set: Set[s_types.Type] = set()
-    for t in types:
-        union_of = t.get_union_of(schema)
-        if union_of:
-            type_set.update(union_of.objects(schema))
-        else:
-            type_set.add(t)
-    # IF we need to preserve derived types, that means that we don't
-    # want to minimize them and instead keep them as is to be
-    # considered in the type union.
-    derived: Set[s_types.Type] = set()
-    components: Set[s_types.Type] = set()
-    for t in type_set:
-        if (
-            preserve_derived and
-            isinstance(t, s_types.InheritingType) and
-            t.get_is_derived(schema)
-        ):
-            derived.add(t)
-        else:
-            components.add(t)
-
-    components_list: List[s_types.Type]
 
-    if all(isinstance(c, s_types.InheritingType) for c in components):
-        components_list = list(minimize_class_set_by_most_generic(
-            schema,
-            cast(Set[s_types.InheritingType], components),
-        ))
-    else:
-        components_list = list(components)
-    components_list.extend(list(derived))
-
-    if len(components_list) == 1 and not opaque:
-        return schema, next(iter(components_list)), False
+    if len(types) == 1 and not opaque:
+        return schema, next(iter(types)), False
 
     seen_scalars = False
     seen_objtypes = False
     created = False
 
-    for component in components_list:
-        if isinstance(component, s_objtypes.ObjectType):
+    for t in types:
+        if isinstance(t, s_objtypes.ObjectType):
             if seen_scalars:
-                raise _union_error(schema, components_list)
+                raise _union_error(schema, types)
             seen_objtypes = True
         else:
             if seen_objtypes:
-                raise _union_error(schema, components_list)
+                raise _union_error(schema, types)
             seen_scalars = True
 
     if seen_scalars:
-        uniontype: s_types.Type = components_list[0]
-        for t1 in components_list[1:]:
+        uniontype: s_types.Type = types[0]
+        for t1 in types[1:]:
 
             schema, common_type = (
                 uniontype.find_common_implicitly_castable_type(t1, schema)
             )
 
             if common_type is None:
-                raise _union_error(schema, components_list)
+                raise _union_error(schema, types)
             else:
                 uniontype = common_type
     else:
         objtypes = cast(
             Sequence[s_objtypes.ObjectType],
-            components_list,
+            types,
         )
         schema, uniontype, created = s_objtypes.get_or_create_union_type(
             schema,
@@ -1109,18 +1074,65 @@ def ensure_union_type(
     return schema, uniontype, created
 
 
-def get_union_type(
+def simplify_union_types(
     schema: s_schema.Schema,
-    types: Iterable[s_types.Type],
-    *,
-    opaque: bool = False,
-    module: Optional[str] = None,
-) -> Tuple[s_schema.Schema, s_types.Type]:
+    types: Sequence[s_types.Type],
+) -> Sequence[s_types.Type]:
+
+    from edb.schema import types as s_types
+
+    components: Set[s_types.Type] = set()
+    for t in types:
+        union_of = t.get_union_of(schema)
+        if union_of:
+            components.update(union_of.objects(schema))
+        else:
+            components.add(t)
+
+    if all(isinstance(c, s_types.InheritingType) for c in components):
+        return list(minimize_class_set_by_most_generic(
+            schema,
+            cast(Set[s_types.InheritingType], components),
+        ))
+    else:
+        return list(components)
 
-    schema, union, _ = ensure_union_type(
-        schema, types, opaque=opaque, module=module)
 
-    return schema, union
+def simplify_union_types_preserve_derived(
+    schema: s_schema.Schema,
+    types: Sequence[s_types.Type],
+) -> Sequence[s_types.Type]:
+
+    from edb.schema import types as s_types
+
+    components: Set[s_types.Type] = set()
+    for t in types:
+        union_of = t.get_union_of(schema)
+        if union_of:
+            components.update(union_of.objects(schema))
+        else:
+            components.add(t)
+
+    derived = set(
+        t
+        for t in components
+        if (
+            isinstance(t, s_types.InheritingType)
+            and t.get_is_derived(schema)
+        )
+    )
+
+    nonderived: Sequence[s_types.Type] = [
+        t
+        for t in components
+        if t not in derived
+    ]
+    nonderived = minimize_class_set_by_most_generic(
+        schema,
+        cast(Set[s_types.InheritingType], nonderived),
+    )
+
+    return list(nonderived) + list(derived)
 
 
 def get_non_overlapping_union(
@@ -1151,72 +1163,64 @@ def _union_error(
 
 def ensure_intersection_type(
     schema: s_schema.Schema,
-    types: Iterable[s_types.Type],
+    types: Sequence[s_types.Type],
     *,
     transient: bool = False,
     module: Optional[str] = None,
 ) -> Tuple[s_schema.Schema, s_types.Type, bool]:
 
     from edb.schema import objtypes as s_objtypes
-    from edb.schema import types as s_types
 
-    components: Set[s_types.Type] = set()
-    for t in types:
-        intersection_of = t.get_intersection_of(schema)
-        if intersection_of:
-            components.update(intersection_of.objects(schema))
-        else:
-            components.add(t)
-
-    components_list: Sequence[s_types.Type]
-
-    if all(isinstance(c, s_types.InheritingType) for c in components):
-        components_list = minimize_class_set_by_least_generic(
-            schema,
-            cast(Set[s_types.InheritingType], components),
-        )
-    else:
-        components_list = list(components)
-
-    if len(components_list) == 1:
-        return schema, next(iter(components_list)), False
+    if len(types) == 1:
+        return schema, next(iter(types)), False
 
     seen_scalars = False
     seen_objtypes = False
 
-    for component in components_list:
-        if component.is_object_type():
+    for t in types:
+        if t.is_object_type():
             if seen_scalars:
-                raise _intersection_error(schema, components_list)
+                raise _intersection_error(schema, types)
             seen_objtypes = True
         else:
             if seen_objtypes:
-                raise _intersection_error(schema, components_list)
+                raise _intersection_error(schema, types)
             seen_scalars = True
 
     if seen_scalars:
         # Non-related scalars and collections cannot for intersection types.
-        raise _intersection_error(schema, components_list)
+        raise _intersection_error(schema, types)
     else:
         return s_objtypes.get_or_create_intersection_type(
             schema,
-            components=cast(Iterable[s_objtypes.ObjectType], components_list),
+            components=cast(Iterable[s_objtypes.ObjectType], types),
             module=module,
             transient=transient,
         )
 
 
-def get_intersection_type(
+def simplify_intersection_types(
     schema: s_schema.Schema,
-    types: Iterable[s_types.Type],
-    *,
-    module: Optional[str] = None,
-) -> Tuple[s_schema.Schema, s_types.Type]:
+    types: Sequence[s_types.Type],
+) -> Sequence[s_types.Type]:
 
-    schema, intersection, _ = ensure_intersection_type(
-        schema, types, module=module)
+    from edb.schema import types as s_types
 
-    return schema, intersection
+    components: Set[s_types.Type] = set()
+    for t in types:
+        intersection_of = t.get_intersection_of(schema)
+        if intersection_of:
+            components.update(intersection_of.objects(schema))
+        else:
+            components.add(t)
+
+    if all(isinstance(c, s_types.InheritingType) for c in components):
+        return minimize_class_set_by_least_generic(
+            schema,
+            cast(Set[s_types.InheritingType], components),
+        )
+    else:
+        return list(components)
 
 
 def _intersection_error(
diff --git a/tests/test_schema.py b/tests/test_schema.py
index c2e8c0200b2..f1b153312ce 100644
--- a/tests/test_schema.py
+++ b/tests/test_schema.py
@@ -4029,6 +4029,18 @@ def test_schema_get_migration_union_ptrs_01(self):
 
         self._assert_migration_consistency(schema)
 
+    def test_schema_get_migration_union_ptrs_02(self):
+        schema = r'''
+        type A;
+        type A_ extending A;
+        type B;
+        type C {
+            link foo -> A | A_ | B;
+        };
+        '''
+
+        self._assert_migration_consistency(schema)
+
     def test_schema_get_migration_except_01(self):
         schema = r'''
         type ExceptTest {


Code review comments:
diff_hunk: @@ -4029,6 +4029,18 @@ def test_schema_get_migration_union_ptrs_01(self):
 
         self._assert_migration_consistency(schema)
 
+    def test_schema_get_migration_union_ptrs_02(self):
+        schema = r'''
+        type A;
+        type A_ extending A;
+        type B;
+        type C {
+            link foo -> A | A_ | B;
+        };
+        '''
+
+        self._assert_migration_consistency(schema)
path: tests/test_schema.py
text: @user1:
Could you reframe this as being one of the `self._assert_migration_equivalence(` tests, and testing that it works to *remove* a link with a type like this?

diff_hunk: @@ -640,9 +640,12 @@ def _alter_finalize(
                 # them in the schema to produce the correct as_alter_delta.
                 nschema = _delete_to_delist(delete, schema)
 
-                nschema, nunion = utils.get_union_type(
+                types = utils.simplify_union_types(
+                    schema, union.get_union_of(schema).objects(schema)
+                )
path: edb/schema/objtypes.py
text: @user1:
Do we want this simplification? It seems like probably not?

Also the surrounding code seems bizarre and makes no sense to me, and so of course I wrote it.