Repository: geldata/gel
Pull request: Partial paths in defaults
Closes #4473

Does not yet handle references links (may not even throw an error).

Problem statement:
Reference properties in a default Schema value
Current error message goes: `edgedb error: QueryError: could not resolve partial path`. I had to dig in multiple places (as always) and found a similar topic in Discord, which was not reported as a limitation in docs, and still is not AFAI searched the docs. 

Discord have a lot of conversation where limitations of certain things are well known especially by @elprans and @colinhacks, but they kindly move around in Discord as walking encyclopedias of EdgeDB but AFAI experienced often do not report them on docs.

Hints from issue discussion:
To make it precise, this issue here is that if we do something like
```
create type Foo { create property x -> str; create property y -> str { set default := .x }; };
```
we get that partial path error message. (Which should be improved in general!)
We should explain that we don't support referring to other properties in a default.
Or actually we could just implement referring to other properties in a default.
Any workarounds for this?

Commit message: fmt, fix tests

Patch to review:
diff --git a/edb/edgeql/ast.py b/edb/edgeql/ast.py
index b39f862ebba..34de159b300 100644
--- a/edb/edgeql/ast.py
+++ b/edb/edgeql/ast.py
@@ -267,6 +267,7 @@ class Subject(SpecialAnchor):  # __subject__
 
 class DetachedExpr(Expr):  # DETACHED Expr
     expr: Expr
+    preserve_path_prefix: bool = False
 
 
 class GlobalExpr(Expr):  # GLOBAL Name
diff --git a/edb/edgeql/compiler/expr.py b/edb/edgeql/compiler/expr.py
index 132228b7743..ff1ffdcb311 100644
--- a/edb/edgeql/compiler/expr.py
+++ b/edb/edgeql/compiler/expr.py
@@ -173,6 +173,9 @@ def compile_DetachedExpr(
     ctx: context.ContextLevel,
 ) -> irast.Set:
     with ctx.detached() as subctx:
+        if expr.preserve_path_prefix:
+            subctx.partial_path_prefix = ctx.partial_path_prefix
+
         ir = dispatch.compile(expr.expr, ctx=subctx)
     # Wrap the result in another set, so that the inner namespace
     # doesn't leak out into any shapes (since computable computation
diff --git a/edb/edgeql/compiler/viewgen.py b/edb/edgeql/compiler/viewgen.py
index 2853f9ad784..0c4c78ef171 100644
--- a/edb/edgeql/compiler/viewgen.py
+++ b/edb/edgeql/compiler/viewgen.py
@@ -28,6 +28,7 @@
 
 from edb import errors
 from edb.common import context as pctx
+from edb.common import topological
 from edb.common.typeutils import not_none
 
 from edb.ir import ast as irast
@@ -196,7 +197,7 @@ def _process_view(
             transparent=True, ctx=ctx)
 
     pointers: List[s_pointers.Pointer] = []
-    pointer_entries = []
+    pointer_entries: List[Tuple[s_pointers.Pointer, irast.Set | None]] = []
 
     elements = elements or ()
     for shape_el in elements:
@@ -246,70 +247,15 @@ def _process_view(
 
     if exprtype.is_insert():
         explicit_ptrs = {
-            ptrcls.get_local_name(ctx.env.schema)
-            for ptrcls in pointers
+            ptrcls.get_local_name(ctx.env.schema) for ptrcls in pointers
         }
-        scls_pointers = stype.get_pointers(ctx.env.schema)
-        for pn, ptrcls in scls_pointers.items(ctx.env.schema):
-            if (pn in explicit_ptrs or
-                    ptrcls.is_pure_computable(ctx.env.schema)):
-                continue
 
-            default_expr = ptrcls.get_default(ctx.env.schema)
-            if not default_expr:
-                if (
-                    ptrcls.get_required(ctx.env.schema)
-                    and pn != sn.UnqualName('__type__')
-                ):
-                    if ptrcls.is_property(ctx.env.schema):
-                        # If the target is a sequence, there's no need
-                        # for an explicit value.
-                        ptrcls_target = ptrcls.get_target(ctx.env.schema)
-                        assert ptrcls_target is not None
-                        if ptrcls_target.issubclass(
-                                ctx.env.schema,
-                                ctx.env.schema.get(
-                                    'std::sequence',
-                                    type=s_objects.SubclassableObject)):
-                            continue
-                    vn = ptrcls.get_verbosename(
-                        ctx.env.schema, with_parent=True)
-                    raise errors.MissingRequiredError(
-                        f'missing value for required {vn}')
-                else:
-                    continue
-
-            ptrcls_sn = ptrcls.get_shortname(ctx.env.schema)
-            default_ql = qlast.ShapeElement(
-                expr=qlast.Path(
-                    steps=[
-                        qlast.Ptr(
-                            ptr=qlast.ObjectRef(
-                                name=ptrcls_sn.name,
-                                module=ptrcls_sn.module,
-                            ),
-                        ),
-                    ],
-                ),
-                compexpr=qlast.DetachedExpr(
-                    expr=default_expr.qlast,
-                ),
+        pointer_entries.extend(
+            gen_pointers_from_defaults(
+                explicit_ptrs, view_scls, ir_set, stype, path_id_namespace,
+                view_rptr, exprtype, ctx
             )
-
-            with ctx.new() as scopectx:
-                pointer_entries.append(
-                    _normalize_view_ptr_expr(
-                        ir_set,
-                        default_ql,
-                        view_scls,
-                        path_id=path_id,
-                        path_id_namespace=path_id_namespace,
-                        exprtype=exprtype,
-                        from_default=True,
-                        view_rptr=view_rptr,
-                        ctx=scopectx,
-                    ),
-                )
+        )
 
     set_shape = []
     shape_ptrs: List[ShapePtr] = []
@@ -394,6 +340,107 @@ def _process_view(
     return view_scls, ir_set
 
 
+def gen_pointers_from_defaults(
+    explicit_ptrs: Set[sn.UnqualName],
+    view_scls: s_objtypes.ObjectType,
+    ir_set: irast.Set,
+    stype: s_objtypes.ObjectType,
+    path_id_namespace: Optional[irast.Namespace],
+    view_rptr: Optional[context.ViewRPtr],
+    exprtype: s_types.ExprType,
+    ctx: context.ContextLevel,
+) -> List[Tuple[s_pointers.Pointer, irast.Set | None]]:
+    path_id = ir_set.path_id
+    result: List[Tuple[s_pointers.Pointer, irast.Set | None]] = []
+
+    scls_pointers = stype.get_pointers(ctx.env.schema)
+    for pn, ptrcls in scls_pointers.items(ctx.env.schema):
+        if pn in explicit_ptrs or ptrcls.is_pure_computable(ctx.env.schema):
+            continue
+
+        default_expr = ptrcls.get_default(ctx.env.schema)
+        if not default_expr:
+            if ptrcls.get_required(ctx.env.schema) and pn != sn.UnqualName(
+                "__type__"
+            ):
+                if ptrcls.is_property(ctx.env.schema):
+                    # If the target is a sequence, there's no need
+                    # for an explicit value.
+                    ptrcls_target = ptrcls.get_target(ctx.env.schema)
+                    assert ptrcls_target is not None
+                    if ptrcls_target.issubclass(
+                        ctx.env.schema,
+                        ctx.env.schema.get(
+                            "std::sequence", type=s_objects.SubclassableObject
+                        ),
+                    ):
+                        continue
+                vn = ptrcls.get_verbosename(ctx.env.schema, with_parent=True)
+                raise errors.MissingRequiredError(
+                    f"missing value for required {vn}"
+                )
+            else:
+                continue
+
+        ptrcls_sn = ptrcls.get_shortname(ctx.env.schema)
+        default_ql = qlast.ShapeElement(
+            expr=qlast.Path(
+                steps=[
+                    qlast.Ptr(
+                        ptr=qlast.ObjectRef(
+                            name=ptrcls_sn.name,
+                            module=ptrcls_sn.module,
+                        ),
+                    ),
+                ],
+            ),
+            compexpr=qlast.DetachedExpr(
+                expr=default_expr.qlast,
+                preserve_path_prefix=True,
+            ),
+        )
+
+        with ctx.new() as scopectx:
+            result.append(
+                _normalize_view_ptr_expr(
+                    ir_set,
+                    default_ql,
+                    view_scls,
+                    path_id=path_id,
+                    path_id_namespace=path_id_namespace,
+                    exprtype=exprtype,
+                    from_default=True,
+                    view_rptr=view_rptr,
+                    ctx=scopectx,
+                ),
+            )
+
+    schema = ctx.env.schema
+
+    # toposort defaults
+    pointer_indexes = {}
+    for (index, (pointer, _)) in enumerate(result):
+        p = pointer.get_nearest_non_derived_parent(schema)
+        pointer_indexes[p.get_name(schema).name] = index
+    graph = {}
+    for (index, (_, irset)) in enumerate(result):
+        assert irset
+        dep_pointers = irutils.collect_pointers(irset)
+        dep_rptrs = (
+            pointer.target.path_id.rptr() for pointer in dep_pointers
+            if pointer.source.typeref.id == stype.id
+        )
+        deps = {
+            pointer_indexes[rpts.name.name] for rpts in dep_rptrs
+            if rpts and rpts.name.name in pointer_indexes
+        }
+        graph[index] = topological.DepGraphEntry(
+            item=index, deps=deps, extra=False,
+        )
+
+    return [result[i] for i in topological.sort(graph, allow_unresolved=True)]
+
+
 def _maybe_fixup_lprop(
     path_id: irast.PathId,
     ptrcls: s_pointers.Pointer,
@@ -453,7 +500,6 @@ def _compile_qlexpr(
     *,
     ptrcls: Optional[s_pointers.Pointer],
     ptrsource: s_sources.Source,
-    path_id: irast.PathId,
     ptr_name: sn.QualName,
     exprtype: s_types.ExprType = s_types.ExprType.Select,
     is_linkprop: bool,
@@ -698,7 +744,7 @@ def _normalize_view_ptr_expr(
             irexpr, _ = _compile_qlexpr(
                 ir_source, qlexpr, view_scls,
                 ptrcls=qlptrcls, ptrsource=qlptrsource,
-                path_id=path_id, ptr_name=ptr_name, is_linkprop=is_linkprop,
+                ptr_name=ptr_name, is_linkprop=is_linkprop,
                 exprtype=exprtype, ctx=ctx)
             materialized = setgen.should_materialize(
                 irexpr, ptrcls=ptrcls,
@@ -770,7 +816,7 @@ def _normalize_view_ptr_expr(
 
         irexpr, sub_view_rptr = _compile_qlexpr(
             ir_source, qlexpr, view_scls, ptrcls=ptrcls, ptrsource=ptrsource,
-            path_id=path_id, ptr_name=ptr_name, is_linkprop=is_linkprop,
+            ptr_name=ptr_name, is_linkprop=is_linkprop,
             exprtype=exprtype, ctx=ctx)
         materialized = setgen.should_materialize(
             irexpr, ptrcls=ptrcls,
diff --git a/edb/ir/utils.py b/edb/ir/utils.py
index c123a5683c2..e0a9c33bbd3 100644
--- a/edb/ir/utils.py
+++ b/edb/ir/utils.py
@@ -451,3 +451,21 @@ def contains_set_of_op(ir: irast.Base) -> bool:
     flt = (lambda n: any(x == ft.TypeModifier.SetOfType
                          for x in n.params_typemods))
     return bool(ast.find_children(ir, irast.Call, flt, terminate_early=True))
+
+
+class PointerCollectorVisitor(ast.NodeVisitor):
+    collected: List[irast.Pointer]
+
+    def __init__(self) -> None:
+        super().__init__()
+        self.collected = []
+
+    def visit_Pointer(self, pointer: irast.Pointer) -> None:
+        self.collected.append(pointer)
+
+
+def collect_pointers(expr: irast.Base) -> List[irast.Pointer]:
+    """Walk over AST and collect all irast.Pointers"""
+    visitor = PointerCollectorVisitor()
+    visitor.visit(expr)
+    return visitor.collected
diff --git a/edb/schema/pointers.py b/edb/schema/pointers.py
index 3dbe03a2165..cbd98544b3f 100644
--- a/edb/schema/pointers.py
+++ b/edb/schema/pointers.py
@@ -27,6 +27,7 @@
 
 from edb.common import enum as s_enum
 from edb.common import struct
+from edb.common import parsing
 
 from edb.edgeql import ast as qlast
 from edb.edgeql import compiler as qlcompiler
@@ -1323,6 +1324,7 @@ def _compile_expr(
         target_as_singleton: bool = False,
         expr_description: Optional[str] = None,
         no_query_rewrites: bool = False,
+        source_context: Optional[parsing.ParserContext] = None,
     ) -> s_expr.CompiledExpression:
         singletons: List[Union[s_types.Type, Pointer]] = []
 
@@ -1368,9 +1370,8 @@ def _compile_expr(
             else:
                 singletons.append(self.scls)
 
-        compiled = expr.compiled(
-            schema=schema,
-            options=qlcompiler.CompilerOptions(
+        try:
+            options = qlcompiler.CompilerOptions(
                 modaliases=context.modaliases,
                 schema_object_context=self.get_schema_metaclass(),
                 anchors={qlast.Source().name: source},
@@ -1381,20 +1382,28 @@ def _compile_expr(
                 ),
                 track_schema_ref_exprs=track_schema_ref_exprs,
                 in_ddl_context_name=in_ddl_context_name,
-            ),
-        )
+            )
 
-        if singleton_result_expected and compiled.cardinality.is_multi():
-            if expr_description is None:
-                expr_description = 'an expression'
+            compiled = expr.compiled(schema=schema, options=options)
 
-            raise errors.SchemaError(
-                f'possibly more than one element returned by '
-                f'{expr_description}, while a singleton is expected',
-                context=expr.qlast.context,
-            )
+            if singleton_result_expected and compiled.cardinality.is_multi():
+                if expr_description is None:
+                    expr_description = 'an expression'
+
+                raise errors.SchemaError(
+                    f'possibly more than one element returned by '
+                    f'{expr_description}, while a singleton is expected',
+                    context=expr.qlast.context,
+                )
 
-        return compiled
+            return compiled
+
+        except errors.QueryError as e:
+            if source_context:
+                e.set_source_context(source_context)
+            if not e.has_source_context():
+                e.set_source_context(expr.qlast.context)
+            raise
 
     def compile_expr_field(
         self,
@@ -1584,6 +1593,9 @@ def validate_object(
         context: sd.CommandContext,
     ) -> None:
         """Check that pointer definition is sound."""
+        from edb.ir import utils as irutils
+        from edb.ir import ast as irast
+
         referrer_ctx = self.get_referrer_context(context)
         if referrer_ctx is None:
             return
@@ -1598,18 +1610,17 @@ def validate_object(
 
         if default_expr is not None:
 
-            source_context = self.get_attribute_source_context('default')
-
-            try:
-                default_expr = default_expr.ensure_compiled(schema)
-            except errors.QueryError as e:
-                e.set_source_context(source_context)
-                raise
+            if not default_expr.irast:
+                default_expr = self._compile_expr(
+                    schema, context, default_expr
+                )
+                assert default_expr.irast
 
             if scls.is_id_pointer(schema):
                 self._check_id_default(
                     schema, context, default_expr.irast.expr)
 
+            source_context = self.get_attribute_source_context('default')
             default_schema = default_expr.irast.schema
             default_type = default_expr.irast.stype
             assert default_type is not None
@@ -1646,6 +1657,23 @@ def validate_object(
                     context=source_context,
                 )
 
+            # prevent references to local links, only properties
+            pointers = irutils.collect_pointers(default_expr.irast)
+            scls_source = scls.get_source(schema)
+            assert scls_source
+            for pointer in pointers:
+                if pointer.source.typeref.id != scls_source.id:
+                    continue
+                if not isinstance(pointer.ptrref, irast.PointerRef):
+                    continue
+                s_pointer = schema.get_by_id(pointer.ptrref.id, type=Pointer)
+                if not s_pointer.is_property(schema):
+                    raise errors.SchemaDefinitionError(
+                        f'default expression cannot refer to local links',
+                        context=source_context,
+                        hint='this is a temporary implementation restriction'
+                    )
+
     def _check_id_default(
         self,
         schema: s_schema.Schema,
diff --git a/tests/test_edgeql_ddl.py b/tests/test_edgeql_ddl.py
index 8d41e73b3e8..95d00b0f16e 100644
--- a/tests/test_edgeql_ddl.py
+++ b/tests/test_edgeql_ddl.py
@@ -559,17 +559,14 @@ async def test_edgeql_ddl_13(self):
             """)
 
     async def test_edgeql_ddl_14(self):
-        with self.assertRaisesRegex(
-                edgedb.QueryError,
-                f'__source__ cannot be used in this expression'):
-            await self.con.execute("""
-                CREATE TYPE TestSelfLink1 {
-                    CREATE PROPERTY foo1 -> std::str;
-                    CREATE PROPERTY bar1 -> std::str {
-                        SET default := __source__.foo1;
-                    };
+        await self.con.execute("""
+            CREATE TYPE TestSelfLink1 {
+                CREATE PROPERTY foo1 -> std::str;
+                CREATE PROPERTY bar1 -> std::str {
+                    SET default := __source__.foo1;
                 };
-            """)
+            };
+        """)
 
     async def test_edgeql_ddl_15(self):
         await self.con.execute(r"""
@@ -607,18 +604,14 @@ async def test_edgeql_ddl_15(self):
         )
 
     async def test_edgeql_ddl_16(self):
-        with self.assertRaisesRegex(
-                edgedb.SchemaDefinitionError,
-                'possibly more than one element'):
-            await self.con.execute(r"""
-                CREATE TYPE TestSelfLink3 {
-                    CREATE PROPERTY foo3 -> std::str;
-                    CREATE PROPERTY bar3 -> std::str {
-                        # NOTE: this is a set of all TestSelfLink3.foo3
-                        SET default := TestSelfLink3.foo3;
-                    };
+        await self.con.execute(r"""
+            CREATE TYPE TestSelfLink3 {
+                CREATE PROPERTY foo3 -> std::str;
+                CREATE PROPERTY bar3 -> std::str {
+                    SET default := TestSelfLink3.foo3;
                 };
-            """)
+            };
+        """)
 
     async def test_edgeql_ddl_18(self):
         await self.con.execute("""
diff --git a/tests/test_edgeql_insert.py b/tests/test_edgeql_insert.py
index 4dc9e6fb775..82772f53cd2 100644
--- a/tests/test_edgeql_insert.py
+++ b/tests/test_edgeql_insert.py
@@ -1779,6 +1779,34 @@ async def test_edgeql_insert_default_06(self):
         # The result should not include the default param
         assert not hasattr(obj, 'num')
 
+    async def test_edgeql_insert_default_07(self):
+        await self.con.query(r'''
+            create type Foo {
+                create property a -> str;
+                create property b -> str;
+                create property c -> str;
+            };
+
+            alter type Foo {
+                alter property a { set default := 'a=' ++ .b };
+                alter property b { set default := 'b=' ++ .c };
+                alter property c { set default := 'c=' ++ .a };
+            };
+        ''')
+        await self.con.query(r'''
+            insert Foo { a := 'given' };
+        ''')
+        await self.con.query(r'''
+            insert Foo { b := 'given' };
+        ''')
+        await self.con.query(r'''
+            insert Foo { c := 'given' };
+        ''')
+        res = await self.con.query(r'''
+            select Foo filter exists .a and exists .b and exists .c
+        ''')
+        assert len(res) == 3
+
     async def test_edgeql_insert_as_expr_01(self):
         await self.con.execute(r'''
             # insert several objects, then annotate one of the inserted batch


Code review comments:
diff_hunk: @@ -451,3 +451,21 @@ def contains_set_of_op(ir: irast.Base) -> bool:
     flt = (lambda n: any(x == ft.TypeModifier.SetOfType
                          for x in n.params_typemods))
     return bool(ast.find_children(ir, irast.Call, flt, terminate_early=True))
+
+
+class PointerCollectorVisitor(ast.NodeVisitor):
+    collected: List[irast.Pointer]
+
+    def __init__(self) -> None:
+        super().__init__()
+        self.collected = []
+
+    def visit_Pointer(self, pointer: irast.Pointer) -> None:
+        self.collected.append(pointer)
+
+
+def collect_pointers(expr: irast.Base) -> List[irast.Pointer]:
path: edb/ir/utils.py
text: @user1:
I think you can use `find_children` instead of defining a visitor.
(And at that point, it's probably best just to inline the call to `find_children`)

diff_hunk: @@ -1646,6 +1657,23 @@ def validate_object(
                     context=source_context,
                 )
 
+            # prevent references to local links, only properties
+            pointers = irutils.collect_pointers(default_expr.irast)
+            scls_source = scls.get_source(schema)
+            assert scls_source
+            for pointer in pointers:
+                if pointer.source.typeref.id != scls_source.id:
+                    continue
+                if not isinstance(pointer.ptrref, irast.PointerRef):
+                    continue
+                s_pointer = schema.get_by_id(pointer.ptrref.id, type=Pointer)
+                if not s_pointer.is_property(schema):
+                    raise errors.SchemaDefinitionError(
+                        f'default expression cannot refer to local links',
path: edb/schema/pointers.py
text: @user1:
What does local mean here?

@user1:
Are all links busted, or just multi ones?

Do multi properties work?
(On both sides: being referenced, and having them reference something else)

@author:
All links are busted:
```
	type Bar {
		property b -> int32;
	}
  
	type Foo {
		link bar -> Bar;

		property a -> int32 {
			default := .bar.b;
		};
	}
```
```
edgedb> insert Foo { bar := (insert Bar { b := 3 }) };
File "/home/aljaz/EdgeDB/edgedb/edb/pgsql/compiler/pathctx.py", line 716, in get_rvar_path_var
          if (path_id, aspect) in (outs := rvar.query.get_path_outputs(flavor)):
                                           ^^^^^^^^^^
        File "/home/aljaz/EdgeDB/edgedb/edb/pgsql/ast.py", line 312, in query
          raise AssertionError('cannot retrieve query from a dynamic range var')
      AssertionError: cannot retrieve query from a dynamic range var
```

@author:
Also multi properties:
```
	type Bar {
		property b -> int32;
	}
  
	type Foo {
		#link bar -> Bar;
		multi property b -> int32;

		property a -> int32 {
			default := count(.b);
		};
	}
```
```
insert Foo { b:= {2, 3, 4} };

        File "/home/aljaz/EdgeDB/edgedb/edb/pgsql/compiler/relctx.py", line 397, in maybe_get_path_var
          return pathctx.get_rvar_path_var(
                 ^^^^^^^^^^^^^^^^^^^^^^^^^^
        File "/home/aljaz/EdgeDB/edgedb/edb/pgsql/compiler/pathctx.py", line 716, in get_rvar_path_var
          if (path_id, aspect) in (outs := rvar.query.get_path_outputs(flavor)):
                                           ^^^^^^^^^^
        File "/home/aljaz/EdgeDB/edgedb/edb/pgsql/ast.py", line 312, in query
          raise AssertionError('cannot retrieve query from a dynamic range var')
      AssertionError: cannot retrieve query from a dynamic range var
```

@author:
I won't even check the other reference direction.