Skip to content

Commit

Permalink
Fix a number of problems with toggling the new scoping futures (#8293)
Browse files Browse the repository at this point in the history
Several of these were just existing bugs related to
_propagate_if_expr_refs .

 * Fix drops of scoping futures
   We were trying to access the future after it was deleted.
 * Fix toggling scoping futures when extensions are installed
 * Fix propagating changes to default arguments.
 * Fix dummy expression generation for types that need backtick-quoting
 * Make constraint compilation code a little more robust
* Add a _scoping_noop_test future for testing purposes and exercise it
some.

Fixes #8290.
  • Loading branch information
msullivan authored Feb 4, 2025
1 parent 79d6c86 commit dac7068
Show file tree
Hide file tree
Showing 8 changed files with 109 additions and 33 deletions.
2 changes: 1 addition & 1 deletion edb/ir/astexpr.py
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ def is_possibly_wrapped_distinct_expr(
if not isinstance(tree, irast.SelectStmt):
return None

return is_pure_distinct_expr(tree.result)
return is_set_expr(tree.result)


def is_set_expr(tree: irast.Base) -> Optional[List[irast.Base]]:
Expand Down
6 changes: 2 additions & 4 deletions edb/pgsql/schemamech.py
Original file line number Diff line number Diff line change
Expand Up @@ -62,8 +62,7 @@ def _get_exclusive_refs(tree: irast.Statement) -> Sequence[irast.Base] | None:
# Check if the expression is
# std::_is_exclusive(<arg>) [and std::_is_exclusive(<arg>)...]

assert isinstance(tree.expr.expr, irast.SelectStmt)
expr = tree.expr.expr.result
expr = tree.expr.expr

return irastexpr.get_constraint_references(expr)

Expand Down Expand Up @@ -256,7 +255,6 @@ def _compile_constraint_data(
options=options,
)
assert isinstance(ir, irast.Statement)
assert isinstance(ir.expr.expr, irast.SelectStmt)

except_ir: Optional[irast.Statement] = None
except_data = None
Expand All @@ -273,7 +271,7 @@ def _compile_constraint_data(
except_data = _edgeql_tree_to_expr_data(except_sql.ast)

terminal_refs: set[irast.Set] = (
ir_utils.get_longest_paths(ir.expr.expr.result)
ir_utils.get_longest_paths(ir.expr.expr)
)
if except_ir is not None:
terminal_refs.update(
Expand Down
13 changes: 13 additions & 0 deletions edb/schema/functions.py
Original file line number Diff line number Diff line change
Expand Up @@ -558,6 +558,19 @@ def compile_expr_field(
return super().compile_expr_field(
schema, context, field, value, track_schema_ref_exprs)

def get_dummy_expr_field_value(
self,
schema: s_schema.Schema,
context: sd.CommandContext,
field: so.Field[Any],
value: Any,
) -> Optional[s_expr.Expression]:
if field.name == 'default':
type = self.scls.get_type(schema)
return s_types.type_dummy_expr(type, schema)
else:
raise NotImplementedError(f'unhandled field {field.name!r}')


class CreateParameter(ParameterCommand, sd.CreateObject[Parameter]):

Expand Down
14 changes: 12 additions & 2 deletions edb/schema/futures.py
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,9 @@
from edb.edgeql import qltypes

from . import delta as sd
from . import objects as so
from . import modules as s_mod
from . import name as sn
from . import objects as so
from . import schema as s_schema


Expand Down Expand Up @@ -152,6 +153,7 @@ class AlterFutureBehavior(
# any schema elements.
@register_handler('simple_scoping')
@register_handler('warn_old_scoping')
@register_handler('_scoping_noop_test')
def toggle_scoping_future(
cmd: FutureBehaviorCommand,
schema: s_schema.Schema,
Expand All @@ -165,7 +167,14 @@ def toggle_scoping_future(

# We need a subcommand to apply the _propagate_if_expr_refs on, so
# make an alter.
dummy_object = cmd.scls
dummy_object = (
cmd.scls if isinstance(cmd, sd.CreateObject)
# HACK: On drops, the future doesn't exist, so grab
# a nonsense object we know will be there.
# The drops *shouldn't* ever fail, so it *shouldn't*
# show up in messages.
else schema.get_global(s_mod.Module, '__derived__')
)
alter_cmd = dummy_object.init_delta_command(
schema, cmdtype=sd.AlterObject)

Expand All @@ -180,6 +189,7 @@ def toggle_scoping_future(

all_expr_objects: list[so.Object] = list(schema.get_objects(
exclude_stdlib=True,
exclude_extensions=True,
extra_filters=[lambda _, x: isinstance(x, types)],
))
extra_refs = {
Expand Down
13 changes: 11 additions & 2 deletions edb/schema/types.py
Original file line number Diff line number Diff line change
Expand Up @@ -2927,8 +2927,17 @@ def type_dummy_expr(
) -> Optional[s_expr.Expression]:
if isinstance(typ, so.DerivableInheritingObject):
typ = typ.get_nearest_non_derived_parent(schema)
text = f'assert_exists(<{typ.get_displayname(schema)}>{{}})'
return s_expr.Expression(text=text)

q = qlast.FunctionCall(
func=('__std__', 'assert_exists'),
args=[
qlast.TypeCast(
type=utils.typeref_to_ast(schema, typ),
expr=qlast.Set(elements=[]),
)
],
)
return s_expr.Expression.from_ast(q, schema)


class TypeCommand(sd.ObjectCommand[TypeT]):
Expand Down
5 changes: 5 additions & 0 deletions tests/test_dump01.py
Original file line number Diff line number Diff line change
Expand Up @@ -1902,6 +1902,11 @@ async def test_dump01_branch_data(self):
include_data=True,
check_method=DumpTestCaseMixin.ensure_schema_data_integrity)

async def test_dump01_future_scope(self):
await self.con.execute('''
create future _scoping_noop_test;
''')


class TestDump01Compat(
tb.DumpCompatTestCase,
Expand Down
33 changes: 9 additions & 24 deletions tests/test_edgeql_ddl.py
Original file line number Diff line number Diff line change
Expand Up @@ -9452,30 +9452,6 @@ async def test_edgeql_ddl_extension_02(self):
DROP EXTENSION TestAuthExtension;
""")

async def test_edgeql_ddl_all_extensions_01(self):
# Install all extensions and then delete them all
exts = await self.con.query('''
select distinct sys::ExtensionPackage.name
''')

ext_commands = ''.join(f'using extension {ext};\n' for ext in exts)
await self.con.execute(f"""
START MIGRATION TO {{
{ext_commands}
module default {{ }}
}};
POPULATE MIGRATION;
COMMIT MIGRATION;
""")

await self.con.execute(f"""
START MIGRATION TO {{
module default {{ }}
}};
POPULATE MIGRATION;
COMMIT MIGRATION;
""")

async def test_edgeql_ddl_role_01(self):
if not self.has_create_role:
self.skipTest("create role is not supported by the backend")
Expand Down Expand Up @@ -16326,6 +16302,7 @@ async def test_edgeql_ddl_scoping_future_01(self):
create type T;
insert T;
insert T;
create function f(x: int64 = 0) -> int64 using (x);
create function get_whatever() -> bool using (
all(T = T)
);
Expand Down Expand Up @@ -16387,6 +16364,14 @@ async def test_edgeql_ddl_scoping_future_01(self):
[dict(func=False, alias=False, query=False)],
)

async def test_edgeql_ddl_scoping_future_02(self):
await self.con.execute("""
create future simple_scoping;
""")
await self.con.execute("""
drop future simple_scoping;
""")

async def test_edgeql_ddl_no_volatile_computable_01(self):
async with self.assertRaisesRegexTx(
edgedb.QueryError,
Expand Down
56 changes: 56 additions & 0 deletions tests/test_edgeql_userddl.py
Original file line number Diff line number Diff line change
Expand Up @@ -417,3 +417,59 @@ async def test_edgeql_userddl_29(self):
await self.con.execute('''
drop type ext::_test::X;
''')

async def test_edgeql_userddl_all_extensions_01(self):
# Install all extensions and then delete them all
exts = await self.con.query('''
select distinct sys::ExtensionPackage.name
''')

# This tests that toggling scoping futures works with
# extensions, and that the extensions work if it is enabled
# first.
await self.con.execute(f"""
START MIGRATION TO {{
using future warn_old_scoping;
module default {{ }}
}};
POPULATE MIGRATION;
COMMIT MIGRATION;
""")

ext_commands = ''.join(f'using extension {ext};\n' for ext in exts)
await self.con.execute(f"""
START MIGRATION TO {{
using future warn_old_scoping;
{ext_commands}
module default {{ }}
}};
POPULATE MIGRATION;
COMMIT MIGRATION;
""")

await self.con.execute(f"""
START MIGRATION TO {{
{ext_commands}
module default {{ }}
}};
POPULATE MIGRATION;
COMMIT MIGRATION;
""")

await self.con.execute(f"""
START MIGRATION TO {{
using future warn_old_scoping;
{ext_commands}
module default {{ }}
}};
POPULATE MIGRATION;
COMMIT MIGRATION;
""")

await self.con.execute(f"""
START MIGRATION TO {{
module default {{ }}
}};
POPULATE MIGRATION;
COMMIT MIGRATION;
""")

0 comments on commit dac7068

Please sign in to comment.