From 31bb7215e0bc40bff0a8b3ca74a9e767676473a1 Mon Sep 17 00:00:00 2001
From: "Michael J. Sullivan" <sully@msully.net>
Date: Tue, 3 Oct 2023 11:53:10 -0700
Subject: [PATCH] Produce proper error messages when defining a field twice
 (#5785)

Fixes #5725.
---
 edb/edgeql/declarative.py | 50 +++++++++++++++++++++++++++++----------
 edb/edgeql/tracer.py      |  4 ++++
 tests/test_schema.py      | 43 +++++++++++++++++++++++++++++++++
 3 files changed, 84 insertions(+), 13 deletions(-)

diff --git a/edb/edgeql/declarative.py b/edb/edgeql/declarative.py
index d2e60637050..1b9bac11260 100644
--- a/edb/edgeql/declarative.py
+++ b/edb/edgeql/declarative.py
@@ -233,7 +233,7 @@ def ensure_pointer_kind(
 
 def get_verbosename_from_fqname(
     fq_name: s_name.QualName,
-    ctx: DepTraceContext,
+    ctx: DepTraceContext | LayoutTraceContext,
 ) -> str:
     traceobj = ctx.objects[fq_name]
     assert traceobj is not None
@@ -249,13 +249,15 @@ def get_verbosename_from_fqname(
     elif isinstance(traceobj, qltracer.ScalarType):
         clsname = 'scalar'
     elif isinstance(traceobj, qltracer.Function):
-        node = ctx.ddlgraph[fq_name].item
-        assert isinstance(node, qlast.FunctionCommand)
-        params = ','.join(
-            qlcodegen.generate_source(param, sdlmode=True)
-            for param in node.params
-        )
-        name = f"{str(fq_name).split('@@', 1)[0]}({params})"
+        name = str(fq_name).split('@@', 1)[0]
+        if isinstance(ctx, DepTraceContext):
+            node = ctx.ddlgraph[fq_name].item
+            assert isinstance(node, qlast.FunctionCommand)
+            params = ','.join(
+                qlcodegen.generate_source(param, sdlmode=True)
+                for param in node.params
+            )
+            name = f"{name}({params})"
     elif isinstance(traceobj, qltracer.Pointer):
         ofobj, name = str(fq_name).split('@', 1)
         ofobj = f" of object type '{ofobj}'"
@@ -276,6 +278,11 @@ def get_verbosename_from_fqname(
         if name == str(s_indexes.DEFAULT_INDEX):
             name = ''
         ofobj = f" of object type '{ofobj}'"
+    elif isinstance(traceobj, qltracer.Field):
+        clsname = 'field'
+        obj, name = fq_name.name.rsplit('@', 1)
+        ofobj = ' of ' + get_verbosename_from_fqname(
+            s_name.QualName(fq_name.module, obj), ctx)
 
     if name:
         return f"{clsname} '{name}'{ofobj}"
@@ -752,6 +759,22 @@ def _trace_item_layout(
             )
             ctx.objects[idx_name] = qltracer.ConcreteIndex(idx_name)
 
+        elif isinstance(decl, qlast.SetField):
+            field_name = s_name.QualName(
+                module=fq_name.module,
+                name=f'{fq_name.name}@{decl.name}',
+            )
+
+            # Trivial fields don't get added to the ddlgraph, which is
+            # where duplication checks are normally done, so do the
+            # check here instead.
+            if field_name in ctx.objects:
+                vn = get_verbosename_from_fqname(field_name, ctx)
+                msg = f'{vn} was already declared'
+                raise errors.InvalidDefinitionError(msg, context=decl.context)
+
+            ctx.objects[field_name] = qltracer.Field(field_name)
+
 
 RECURSION_GUARD: Set[s_name.QualName] = set()
 
@@ -1084,9 +1107,7 @@ def _register_item(
     if fq_name in ctx.ddlgraph:
         vn = get_verbosename_from_fqname(fq_name, ctx)
         msg = f'{vn} was already declared'
-
-        raise errors.InvalidDefinitionError(
-            msg, context=decl.context)
+        raise errors.InvalidDefinitionError(msg, context=decl.context)
 
     if deps:
         deps = set(deps)
@@ -1333,8 +1354,11 @@ def _get_pointer_deps(
     # is is important for properly doing cardinality inference
     # on expressions involving it.
     # PERF: We should avoid actually searching all the objects.
-    for propname in ctx.objects:
-        if str(propname).startswith(str(pointer) + '@'):
+    for propname, prop in ctx.objects.items():
+        if (
+            str(propname).startswith(str(pointer) + '@')
+            and not isinstance(prop, qltracer.Field)
+        ):
             result.add(propname)
 
     return result
diff --git a/edb/edgeql/tracer.py b/edb/edgeql/tracer.py
index cfa4dba154c..c04f1615924 100644
--- a/edb/edgeql/tracer.py
+++ b/edb/edgeql/tracer.py
@@ -88,6 +88,10 @@ class ConcreteIndex(NamedObject):
     pass
 
 
+class Field(NamedObject):
+    pass
+
+
 class Type(NamedObject):
     def is_scalar(self) -> bool:
         return False
diff --git a/tests/test_schema.py b/tests/test_schema.py
index 48d5fe30cf5..c7f0f1e17fa 100644
--- a/tests/test_schema.py
+++ b/tests/test_schema.py
@@ -892,6 +892,49 @@ def test_schema_module_reserved_01(self):
             }
         """
 
+    @tb.must_fail(
+        errors.InvalidDefinitionError,
+        "field 'default' .*was already declared"
+    )
+    def test_schema_field_dupe_01(self):
+        """
+        type SimpleNumbers {
+            property bar: str;
+            property foo: str {
+                default := '';
+                default := '';
+            }
+        }
+        """
+
+    @tb.must_fail(
+        errors.InvalidDefinitionError,
+        "field 'default' .*was already declared"
+    )
+    def test_schema_field_dupe_02(self):
+        """
+        type SimpleNumbers {
+            property bar: str;
+            property foo: str {
+                default := .bar;
+                default := .bar;
+            }
+        }
+        """
+
+    @tb.must_fail(
+        errors.InvalidDefinitionError,
+        "field 'foo' .*was already declared"
+    )
+    def test_schema_field_dupe_03(self):
+        """
+        type SimpleNumbers {
+            bar: str;
+            foo := .bar ++ "!";
+            foo := .bar ++ "!";
+        }
+        """
+
     @tb.must_fail(
         errors.InvalidFunctionDefinitionError,
         r"cannot create the `test::foo\(VARIADIC bar: "