Repository: geldata/gel
Pull request: Don't crash on empty set as FOR iterator.
This catches the issue and reports an error in the compiler.

Fixes #1585.

I figure there's no compelling reason to also catch the special
syntactic case in the parser, given that we need to handle it in a
more general way in the compiler anyway?

Problem statement:
InternalServerError: local variable 'derived' referenced before assignment
<!-- Please search existing issues to avoid creating duplicates. -->
- EdgeDB Version: Docker: `1.0a4-2020071615~buster`

This code causes error:
```edgeql
START TRANSACTION;
START MIGRATION TO {
    module default {
        function create_user() -> str
        {
            using EdgeQL $$
                FOR x IN {}
                UNION ()
            $$;
        };
    };
};
```

This seems related to `FOR x IN {}`.
Adding code to UNION block does not change anything, replacing `{}` with value removes error.

Hints from issue discussion:

Commit message: Don't crash on empty set as FOR iterator.

This catches the issue and reports an error in the compiler.

Fixes #1585.

Patch to review:
diff --git a/edb/edgeql/compiler/schemactx.py b/edb/edgeql/compiler/schemactx.py
index 1a8ebab3672..5e99d0d78dd 100644
--- a/edb/edgeql/compiler/schemactx.py
+++ b/edb/edgeql/compiler/schemactx.py
@@ -213,6 +213,9 @@ def derive_view(
                     stmtctx.pend_pointer_cardinality_inference(
                         ptrcls=ptr, ctx=ctx)
 
+    else:
+        raise RuntimeError("unsupported type in derive_view")
+
     ctx.view_nodes[derived.get_name(ctx.env.schema)] = derived
 
     if preserve_shape and stype in ctx.env.view_shapes:
diff --git a/edb/edgeql/compiler/stmt.py b/edb/edgeql/compiler/stmt.py
index 690c3e9c228..490f53703ea 100644
--- a/edb/edgeql/compiler/stmt.py
+++ b/edb/edgeql/compiler/stmt.py
@@ -38,6 +38,7 @@
 from edb.schema import objects as s_obj
 from edb.schema import objtypes as s_objtypes
 from edb.schema import pointers as s_pointers
+from edb.schema import pseudo as s_pseudo
 from edb.schema import types as s_types
 
 from edb.edgeql import ast as qlast
@@ -131,6 +132,14 @@ def compile_ForQuery(
             iterator_stmt = setgen.new_set_from_set(
                 iterator_view, preserve_scope_ns=True, ctx=scopectx)
 
+            ptr_target = inference.infer_type(iterator_stmt, ctx.env)
+            anytype = ptr_target.find_any(ctx.env.schema)
+            if anytype is not None:
+                raise errors.QueryError(
+                    'FOR statement has iterator of indeterminate type',
+                    context=ctx.env.type_origins.get(anytype),
+                )
+
             if iterator_ctx is not None and iterator_ctx.stmt is not None:
                 iterator_ctx.stmt.hoisted_iterators.append(iterator_stmt)
 
@@ -768,7 +777,14 @@ def fini_stmt(
     view: Optional[s_types.Type]
     path_id: Optional[irast.PathId]
 
-    if t.get_name(ctx.env.schema) == view_name:
+    if (isinstance(t, s_pseudo.PseudoType)
+            and t.is_any(ctx.env.schema)):
+        # Need to produce something valid. Should get caught as an
+        # error later.
+        view = None
+        path_id = None
+
+    elif t.get_name(ctx.env.schema) == view_name:
         # The view statement did contain a view declaration and
         # generated a view class with the requested name.
         view = t
diff --git a/tests/test_edgeql_for.py b/tests/test_edgeql_for.py
index b7a04542bd1..20dd782ec4a 100644
--- a/tests/test_edgeql_for.py
+++ b/tests/test_edgeql_for.py
@@ -19,6 +19,8 @@
 
 import os.path
 
+import edgedb
+
 from edb.testbase import server as tb
 from edb.tools import test
 
@@ -418,3 +420,21 @@ async def test_edgeql_for_in_computable_04(self):
                 }
             ]
         )
+
+    async def test_edgeql_for_empty_01(self):
+        with self.assertRaisesRegex(
+            edgedb.errors.QueryError,
+            "FOR statement has iterator of indeterminate type",
+        ):
+            await self.con.execute("""
+                SELECT (FOR x in {} UNION ());
+            """)
+
+    async def test_edgeql_for_empty_02(self):
+        with self.assertRaisesRegex(
+            edgedb.errors.QueryError,
+            "FOR statement has iterator of indeterminate type",
+        ):
+            await self.con.execute("""
+                WITH s := {} SELECT (FOR x in {s} UNION ());
+            """)


Code review comments:
diff_hunk: @@ -213,6 +213,9 @@ def derive_view(
                     stmtctx.pend_pointer_cardinality_inference(
                         ptrcls=ptr, ctx=ctx)
 
+    else:
+        raise RuntimeError("unsupported type in derive_view")
path: edb/edgeql/compiler/schemactx.py
text: @user1:
`TypeError`?

It would be propagated as an `InternalServerError` anyways, but `TypeError` looks like a better choice here.

@user2:
I think we should just standardize on something like an `AssertionError` to stop wondering about which exception to use in case of an unexpected state on the insides.  I picked `AssertionError` because we also have lots of `assert` around.

@user1:
For cases like this I'd suggest something like `UnreachableError`.

@user1:
Or even `Never(BaseException)`

@author:
`AssertionError` seems best to me if we want to standarize, since a lot of such errors are going to be assertion rerors