Repository: geldata/gel
Pull request: Drop FINAL from the syntax
The `final` attribute has some internal uses, but they don't exactly
correspond to what FINAL ought to mean in the language. It's
undocumented, so just yank it.

The one snag is that we actually currently when describing DDL will
*always* describe a scalar type as being FINAL, so dumps will contain
`CREATE FINAL SCALAR TYPE`. This means that we need to continue accepting
that particular syntax, but disallow it based on our compat version.
Ugly, but oh well. If we later add a proper FINAL, then we'll continue
ignoring FINAL when the compat ver is old enough.

Fixes #2607.

Problem statement:
final seems busted
* We have a `final` property on SubclassableObject. Doing some introspection, it seems to be set on scalar types and on the "source" and "target" properties of links.
* The main implication of final seems to be that the objects are skipped when doing a propagate_ref into children?
* There is syntax for altering the final property on types and pointers (though not for setting it as part of CREATE)
* Using that syntax doesn't accomplish much productive. (For types it didn't seem to do anything. For a pointer, it caused an ISE when I then tried to subtype the type.)

So either we should:
 * Drop the syntax for `SET FINAL`, etc
Or:
 * Implement creation syntax for final
 * Do the appropriate checks: that a type isn't inherited, and that a pointer isn't overloaded
 * Implementing the pointer thing likely conflicts with the existing use of final. (Which seems easily surmountable.)

Hints from issue discussion:
I think that the right thing for 1.0 is the first option of "drop the syntax"?
There's `CREATE FINAL SCALAR TYPE` syntax (and corresponding SDL syntax).
It also doesn't really seem to do anything?
It's also undocumented, so +1 for dropping the syntaxes for now.

Commit message: Drop FINAL from the syntax

The `final` attribute has some internal uses, but they don't exactly
correspond to what FINAL ought to mean in the language. It's
undocumented, so just yank it.

The one snag is that we actually currently when describing DDL will
*always* describe a scalar type as being FINAL, so dumps will contain
`CREATE FINAL SCALAR TYPE`. This means that we need to continue accepting
that particular syntax, but disallow it based on our compat version.
Ugly, but oh well. If we later add a proper FINAL, then we'll continue
ignoring FINAL when the compat ver is old enough.

Fixes #2607.

Patch to review:
diff --git a/docs/edgeql/introspection/constraints.rst b/docs/edgeql/introspection/constraints.rst
index 87471c1c0de..74ebe709f11 100644
--- a/docs/edgeql/introspection/constraints.rst
+++ b/docs/edgeql/introspection/constraints.rst
@@ -41,7 +41,6 @@ Introspection of the ``schema::Constraint``:
                 Object { name: 'finalexpr' },
                 Object { name: 'id' },
                 Object { name: 'abstract' },
-                Object { name: 'final' },
                 Object { name: 'name' },
                 Object { name: 'return_typemod' },
                 Object { name: 'subjectexpr' }
diff --git a/docs/edgeql/introspection/objects.rst b/docs/edgeql/introspection/objects.rst
index 311fc18999d..6f81472eb98 100644
--- a/docs/edgeql/introspection/objects.rst
+++ b/docs/edgeql/introspection/objects.rst
@@ -39,7 +39,6 @@ Introspection of the ``schema::ObjectType``:
             properties: {
                 Object { name: 'id' },
                 Object { name: 'abstract' },
-                Object { name: 'final' },
                 Object { name: 'name' }
             }
         }
@@ -71,7 +70,6 @@ Introspection of ``User``:
     ... SELECT ObjectType {
     ...     name,
     ...     abstract,
-    ...     final,
     ...     bases: { name },
     ...     ancestors: { name },
     ...     annotations: { name, @value },
@@ -95,7 +93,6 @@ Introspection of ``User``:
         Object {
             name: 'default::User',
             abstract: false,
-            final: false,
             bases: {Object { name: 'default::Addressable' }},
             ancestors: {
                 Object { name: 'std::BaseObject' },
diff --git a/docs/edgeql/introspection/scalars.rst b/docs/edgeql/introspection/scalars.rst
index b095c681fba..29907142ea0 100644
--- a/docs/edgeql/introspection/scalars.rst
+++ b/docs/edgeql/introspection/scalars.rst
@@ -37,7 +37,6 @@ Introspection of the ``schema::ScalarType``:
                 Object { name: 'enum_values' },
                 Object { name: 'id' },
                 Object { name: 'abstract' },
-                Object { name: 'final' },
                 Object { name: 'name' }
             }
         }
@@ -53,7 +52,6 @@ Introspection of the built-in scalar :eql:type:`str`:
     ...     default,
     ...     enum_values,
     ...     abstract,
-    ...     final,
     ...     bases: { name },
     ...     ancestors: { name },
     ...     annotations: { name, @value },
@@ -66,7 +64,6 @@ Introspection of the built-in scalar :eql:type:`str`:
             default: {},
             enum_values: {},
             abstract: {},
-            final: {},
             bases: {Object { name: 'std::anyscalar' }},
             ancestors: {Object { name: 'std::anyscalar' }},
             annotations: {},
@@ -91,7 +88,6 @@ Introspection of the enum scalar ``Color``:
     ...     default,
     ...     enum_values,
     ...     abstract,
-    ...     final,
     ...     bases: { name },
     ...     ancestors: { name },
     ...     annotations: { name, @value },
@@ -104,7 +100,6 @@ Introspection of the enum scalar ``Color``:
             default: {},
             enum_values: ['Red', 'Green', 'Blue'],
             abstract: {},
-            final: true,
             bases: {Object { name: 'std::anyenum' }},
             ancestors: {
                 Object { name: 'std::anyscalar' },
diff --git a/edb/edgeql/ast.py b/edb/edgeql/ast.py
index 0a9666b1bf2..cdd20b529b9 100644
--- a/edb/edgeql/ast.py
+++ b/edb/edgeql/ast.py
@@ -692,6 +692,9 @@ class DropObject(ObjectDDL):
 
 
 class CreateExtendingObject(CreateObject, BasesMixin):
+    # final is not currently implemented, and the syntax is not
+    # supported except in old dumps. We track it only to allow us to
+    # error on it.
     final: bool = False
 
 
diff --git a/edb/edgeql/codegen.py b/edb/edgeql/codegen.py
index 2d64be9e082..868974fccd4 100644
--- a/edb/edgeql/codegen.py
+++ b/edb/edgeql/codegen.py
@@ -1213,13 +1213,6 @@ def _process_special_set(
                 keywords.extend(('SET', 'DELEGATED'))
             else:
                 keywords.extend(('SET', 'NOT', 'DELEGATED'))
-        elif fname == 'final':
-            if node.value is None:
-                keywords.extend(('RESET', 'FINAL'))
-            elif self._eval_bool_expr(node.value):
-                keywords.extend(('SET', 'FINAL'))
-            else:
-                keywords.extend(('SET', 'NOT', 'FINAL'))
         elif fname == 'cardinality':
             if node.value is None:
                 keywords.extend(('RESET', 'CARDINALITY'))
@@ -1371,8 +1364,6 @@ def visit_CreateScalarType(self, node: qlast.CreateScalarType) -> None:
         keywords = []
         if node.abstract:
             keywords.append('ABSTRACT')
-        if node.final:
-            keywords.append('FINAL')
         keywords.append('SCALAR')
         keywords.append('TYPE')
 
@@ -1656,8 +1647,6 @@ def visit_CreateObjectType(self, node: qlast.CreateObjectType) -> None:
 
         if node.abstract:
             keywords.append('ABSTRACT')
-        if node.final:
-            keywords.append('FINAL')
         keywords.append('TYPE')
 
         after_name = lambda: self._ddl_visit_bases(node)
diff --git a/edb/edgeql/parser/grammar/ddl.py b/edb/edgeql/parser/grammar/ddl.py
index 772c7bd014f..50cf167ee81 100644
--- a/edb/edgeql/parser/grammar/ddl.py
+++ b/edb/edgeql/parser/grammar/ddl.py
@@ -512,38 +512,6 @@ def reduce_RESET_ABSTRACT(self, *kids):
         )
 
 
-class AlterFinal(Nonterm):
-
-    def reduce_DROP_FINAL(self, *kids):
-        # TODO: Raise a DeprecationWarning once we have facility for that.
-        self.val = qlast.SetField(
-            name='final',
-            value=qlast.BooleanConstant.from_python(False),
-            special_syntax=True,
-        )
-
-    def reduce_SET_NOT_FINAL(self, *kids):
-        self.val = qlast.SetField(
-            name='final',
-            value=qlast.BooleanConstant.from_python(False),
-            special_syntax=True,
-        )
-
-    def reduce_SET_FINAL(self, *kids):
-        self.val = qlast.SetField(
-            name='final',
-            value=qlast.BooleanConstant.from_python(True),
-            special_syntax=True,
-        )
-
-    def reduce_RESET_FINAL(self, *kids):
-        self.val = qlast.SetField(
-            name='final',
-            value=None,
-            special_syntax=True,
-        )
-
-
 class OptInheritPosition(Nonterm):
     def reduce_BEFORE_NodeName(self, *kids):
         self.val = qlast.Position(ref=kids[1].val, position='BEFORE')
@@ -572,9 +540,6 @@ def reduce_DROP_EXTENDING_SimpleTypeNameList(self, *kids):
     def reduce_AlterAbstract(self, *kids):
         self.val = kids[0].val
 
-    def reduce_AlterFinal(self, *kids):
-        self.val = kids[0].val
-
 
 class AlterExtending(Nonterm):
     def reduce_EXTENDING_TypeNameList_OptInheritPosition(self, *kids):
@@ -587,9 +552,6 @@ def reduce_DROP_EXTENDING_TypeNameList(self, *kids):
     def reduce_AlterAbstract(self, *kids):
         self.val = kids[0].val
 
-    def reduce_AlterFinal(self, *kids):
-        self.val = kids[0].val
-
 
 class AlterOwnedStmt(Nonterm):
 
@@ -1091,6 +1053,10 @@ def reduce_CreateFinalScalarTypeStmt(self, *kids):
             CREATE FINAL SCALAR TYPE NodeName \
             OptExtending OptCreateScalarTypeCommandsBlock \
         """
+        # Old dumps (1.0-beta.3 and earlier) specify FINAL for all
+        # scalar types, despite it not doing anything and being
+        # undocumented. So we need to support it in the syntax, and we
+        # reject later it when not reading an old dump.
         self.val = qlast.CreateScalarType(
             name=kids[4].val,
             final=True,
diff --git a/edb/edgeql/parser/grammar/sdl.py b/edb/edgeql/parser/grammar/sdl.py
index d38f338fd01..39ed335ce40 100644
--- a/edb/edgeql/parser/grammar/sdl.py
+++ b/edb/edgeql/parser/grammar/sdl.py
@@ -429,18 +429,6 @@ def reduce_CreateAbstractScalarTypeStmt(self, *kids):
             commands=kids[5].val,
         )
 
-    def reduce_CreateFinalScalarTypeStmt(self, *kids):
-        r"""%reduce \
-            FINAL SCALAR TYPE NodeName \
-            OptExtending CreateScalarTypeSDLCommandsBlock \
-        """
-        self.val = qlast.CreateScalarType(
-            final=True,
-            name=kids[3].val,
-            bases=kids[4].val,
-            commands=kids[5].val,
-        )
-
     def reduce_ScalarTypeDeclaration(self, *kids):
         r"""%reduce \
             SCALAR TYPE NodeName \
@@ -465,17 +453,6 @@ def reduce_CreateAbstractScalarTypeStmt(self, *kids):
             bases=kids[4].val,
         )
 
-    def reduce_CreateFinalScalarTypeStmt(self, *kids):
-        r"""%reduce \
-            FINAL SCALAR TYPE NodeName \
-            OptExtending \
-        """
-        self.val = qlast.CreateScalarType(
-            final=True,
-            name=kids[3].val,
-            bases=kids[4].val,
-        )
-
     def reduce_ScalarTypeDeclaration(self, *kids):
         r"""%reduce \
             SCALAR TYPE NodeName \
diff --git a/edb/schema/inheriting.py b/edb/schema/inheriting.py
index fa12f086851..662a7639172 100644
--- a/edb/schema/inheriting.py
+++ b/edb/schema/inheriting.py
@@ -527,7 +527,7 @@ def get_ast_attr_for_field(
         astnode: Type[qlast.DDLOperation],
     ) -> Optional[str]:
         if (
-            field in {'abstract', 'final'}
+            field in {'abstract'}
             and issubclass(astnode, qlast.CreateObject)
         ):
             return field
@@ -748,9 +748,6 @@ def _cmd_tree_from_ast(
         if bases is not None:
             cmd.set_attribute_value('bases', bases)
 
-        if getattr(astnode, 'final', False):
-            cmd.set_attribute_value('final', True)
-
         return cmd
 
     def _apply_field_ast(
@@ -974,9 +971,6 @@ def _cmd_tree_from_ast(
 
                 cmd.add(rebase_cmd)
 
-        if getattr(astnode, 'final', False):
-            cmd.set_attribute_value('final', True)
-
         return cmd
 
 
diff --git a/edb/schema/objects.py b/edb/schema/objects.py
index a25e1d68e45..0f550a2f9e0 100644
--- a/edb/schema/objects.py
+++ b/edb/schema/objects.py
@@ -2820,7 +2820,6 @@ class SubclassableObject(Object):
         bool,
         default=False,
         inheritable=False,
-        special_ddl_syntax=True,
         compcoef=0.909,
     )
 
diff --git a/edb/schema/scalars.py b/edb/schema/scalars.py
index fb5c21f1c16..36ce7995807 100644
--- a/edb/schema/scalars.py
+++ b/edb/schema/scalars.py
@@ -24,6 +24,7 @@
 from edb import errors
 
 from edb.common import checked
+from edb.common import verutils
 from edb.edgeql import ast as qlast
 from edb.edgeql import qltypes
 
@@ -370,6 +371,17 @@ def _cmd_tree_from_ast(
             create_cmd = cmd
 
         if isinstance(astnode, qlast.CreateScalarType):
+            # We don't support FINAL, but old dumps specify it on all
+            # CREATE SCALAR TYPEs, so we need to permit it in those
+            # cases.
+            if astnode.final and not context.compat_ver_is_before(
+                (1, 0, verutils.VersionStage.BETA, 4)
+            ):
+                raise errors.SchemaError(
+                    f'FINAL is not supported',
+                    context=astnode.context,
+                )
+
             bases = [
                 s_utils.ast_to_type_shell(
                     b,
diff --git a/tests/test_edgeql_data_migration.py b/tests/test_edgeql_data_migration.py
index 6ccb3f1a523..bbda656b193 100644
--- a/tests/test_edgeql_data_migration.py
+++ b/tests/test_edgeql_data_migration.py
@@ -1631,7 +1631,7 @@ async def test_edgeql_migration_describe_enum_01(self):
             'proposed': {
                 'statements': [{
                     'text': (
-                        "CREATE FINAL SCALAR TYPE test::EnumType1"
+                        "CREATE SCALAR TYPE test::EnumType1"
                         " EXTENDING enum<foo, bar>;"
                     )
                 }],
@@ -1692,7 +1692,7 @@ async def test_edgeql_migration_describe_enum_02(self):
             'proposed': {
                 'statements': [{
                     'text': (
-                        "CREATE FINAL SCALAR TYPE test::EnumType02"
+                        "CREATE SCALAR TYPE test::EnumType02"
                         " EXTENDING enum<foo, bar>;"
                     )
                 }],
@@ -1753,7 +1753,7 @@ async def test_edgeql_migration_describe_enum_02(self):
             'proposed': {
                 'statements': [{
                     'text': (
-                        "CREATE FINAL SCALAR TYPE test::EnumType02"
+                        "CREATE SCALAR TYPE test::EnumType02"
                         " EXTENDING enum<foo, bar>;"
                     )
                 }],
diff --git a/tests/test_edgeql_ddl.py b/tests/test_edgeql_ddl.py
index fc3412cb78e..5e117f644f5 100644
--- a/tests/test_edgeql_ddl.py
+++ b/tests/test_edgeql_ddl.py
@@ -4874,6 +4874,14 @@ async def test_edgeql_ddl_scalar_08(self):
                 ALTER SCALAR TYPE myint CREATE CONSTRAINT std::one_of(1);
             ''')
 
+    async def test_edgeql_ddl_scalar_09(self):
+        with self.assertRaisesRegex(
+                edgedb.SchemaError,
+                r'FINAL is not supported'):
+            await self.con.execute('''
+                CREATE FINAL SCALAR TYPE myint EXTENDING std::int64;
+            ''')
+
     async def test_edgeql_ddl_cast_01(self):
         await self.con.execute('''
             CREATE SCALAR TYPE type_a EXTENDING std::str;
diff --git a/tests/test_schema_syntax.py b/tests/test_schema_syntax.py
index 638a443729c..b0d8514acfd 100644
--- a/tests/test_schema_syntax.py
+++ b/tests/test_schema_syntax.py
@@ -827,6 +827,8 @@ def test_eschema_syntax_scalar_06(self):
         };
         """
 
+    @tb.must_fail(errors.EdgeQLSyntaxError, r"Unexpected 'final'",
+                  line=3, col=13)
     def test_eschema_syntax_scalar_07(self):
         """
         module test {


Code review comments:
diff_hunk: @@ -370,6 +371,17 @@ def _cmd_tree_from_ast(
             create_cmd = cmd
 
         if isinstance(astnode, qlast.CreateScalarType):
+            # We don't support FINAL, but old dumps specify it on all
+            # CREATE SCALAR TYPEs, so we need to permit it in those
+            # cases.
+            if astnode.final and not context.compat_ver_is_before(
+                (1, 0, verutils.VersionStage.BETA, 4)
+            ):
+                raise errors.SchemaError(
path: edb/schema/scalars.py
text: @user1:
UnsupportedFeatureError?