Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add support for type expressions in intersections #7172

Merged
merged 11 commits into from May 14, 2024
Merged

Add support for type expressions in intersections #7172

merged 11 commits into from May 14, 2024

Conversation

dnwpark
Copy link
Contributor

@dnwpark dnwpark commented Apr 9, 2024 •
edited

Allow '|' and '&' in intersections.

Examples using the advanced types database:

{
_localdev:advancedtypes> select Ba[is Bb | Bc];
  default::CBaBc {id: 90e5466b-f69e-11ee-9882-db9098d38acf},
  default::CBaBc {id: 90e5466c-f69e-11ee-9882-9b06405472f4},
  default::CBaBbBc {id: 9234cc50-f69e-11ee-9882-7bef563d5961},
  default::CBaBbBc {id: 9234cc51-f69e-11ee-9882-c7a7a4594329},
  default::CBaBb {id: 90e54669-f69e-11ee-9882-0bcc2d82fdca},
  default::CBaBb {id: 90e5466a-f69e-11ee-9882-db6f00d601e9},
  default::CBaBbBc {id: 9234cc50-f69e-11ee-9882-7bef563d5961},
  default::CBaBbBc {id: 9234cc51-f69e-11ee-9882-c7a7a4594329},
}

_localdev:advancedtypes> select Ba[is Bb & Bc];
{
  default::CBaBbBc {id: 9234cc50-f69e-11ee-9882-7bef563d5961},
  default::CBaBbBc {id: 9234cc51-f69e-11ee-9882-c7a7a4594329},
}

_localdev:advancedtypes> select Ba[is CBa | Bb & Bc];
{
  default::CBaBbBc {id: 9234cc50-f69e-11ee-9882-7bef563d5961},
  default::CBaBbBc {id: 9234cc51-f69e-11ee-9882-c7a7a4594329},
  default::CBa {id: 900a300a-f69e-11ee-9882-5bf50e8b2919},
  default::CBa {id: 900a300b-f69e-11ee-9882-57c0233146e7},
}


_localdev:advancedtypes> select Ba[is (CBa | Bb) & Bc];
{
  default::CBaBbBc {id: 9234cc50-f69e-11ee-9882-7bef563d5961},
  default::CBaBbBc {id: 9234cc51-f69e-11ee-9882-c7a7a4594329},
}

@dnwpark dnwpark force-pushed the type-intersection-complex branch 2 times, most recently from 7f1a534 to 1fa6954 Compare April 9, 2024 18:52
@dnwpark dnwpark requested a review from msullivan April 9, 2024 19:22
elprans
elprans reviewed Apr 10, 2024
View reviewed changes
edb/edgeql/compiler/schemactx.py Outdated
@@ -30,6 +30,8 @@
Union,
Iterable,
Sequence,
List,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are on Python 3.11+ so prefer using builtin types instead, e.g list[foo] instead of List[foo]

@dnwpark dnwpark force-pushed the type-intersection-complex branch 3 times, most recently from 852cabf to 12cae6b Compare April 15, 2024 22:08
@dnwpark dnwpark force-pushed the type-intersection-complex branch 7 times, most recently from e3bbcb6 to 7e90ca9 Compare April 29, 2024 17:57
@dnwpark dnwpark changed the base branch from master to typeref-remove-intersection April 29, 2024 17:57
@dnwpark dnwpark force-pushed the typeref-remove-intersection branch 5 times, most recently from 5e114aa to 418b2d1 Compare May 6, 2024 17:38
@dnwpark dnwpark force-pushed the type-intersection-complex branch 4 times, most recently from 50e8b56 to 9db9b8d Compare May 7, 2024 22:59
@dnwpark dnwpark force-pushed the typeref-remove-intersection branch from 418b2d1 to 6a51455 Compare May 9, 2024 21:50
@dnwpark dnwpark force-pushed the type-intersection-complex branch 3 times, most recently from 3a3979a to a455852 Compare May 10, 2024 22:02
@dnwpark dnwpark force-pushed the typeref-remove-intersection branch from 6a51455 to c55425b Compare May 11, 2024 00:21
@dnwpark dnwpark force-pushed the type-intersection-complex branch from 0ac4824 to 371cbfa Compare May 11, 2024 00:25
@dnwpark dnwpark force-pushed the typeref-remove-intersection branch from 7a5f0c0 to 3be2fea Compare May 11, 2024 23:42
@dnwpark dnwpark force-pushed the type-intersection-complex branch from 371cbfa to 53858c7 Compare May 11, 2024 23:47
@dnwpark dnwpark force-pushed the typeref-remove-intersection branch from 3be2fea to a8de853 Compare May 12, 2024 00:20
@dnwpark dnwpark force-pushed the type-intersection-complex branch 3 times, most recently from 96edc3d to a71ed0f Compare May 12, 2024 00:34
@dnwpark dnwpark force-pushed the typeref-remove-intersection branch from a8de853 to 759123d Compare May 13, 2024 14:21
@dnwpark dnwpark force-pushed the type-intersection-complex branch from a71ed0f to 7ce8da7 Compare May 13, 2024 14:22
msullivan
msullivan reviewed May 14, 2024
View reviewed changes
edb/edgeql/compiler/schemactx.py Outdated
Comment on lines 562 to 566
if (
left.get_is_opaque_union(ctx.env.schema)
and (left_union := left.get_union_of(ctx.env.schema))
):
left = get_union_type(left_union.objects(ctx.env.schema), ctx=ctx)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The opaque union stuff really is dodgy in general. Does this represent a change of behavior, or are we doing basically what we were before for opaque unions?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In particular, I think that intersecting with an opaque union really ought to just produce the intersector type---but the reason we don't want that is for implementation purposes, IIRC?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Always producing intersections here causes issues when compiling the standard library because it tries to create an intersection TypeShell.

msullivan
msullivan approved these changes May 14, 2024
View reviewed changes
Copy link
Member

@msullivan msullivan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good, pending some follow-ups on opaque unions.

Let's make sure we understand exactly what the behavior is there, what we want, and have it documented where we are applying the intersections.

Great work on this!
Classic onboarding rabbit hole.
I'm happy I never did it during my first months :)

edb/schema/objects.py Outdated
@@ -3147,6 +3148,7 @@ def _issubclass(
self,
schema: s_schema.Schema,
parent: SubclassableObject,
**kwargs: Any,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should just pass through the arguments with defaults everywhere. (Sorry)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems unpacking opaque unions and views as thoroughly as I initially attempted is not necessary. I've removed the kwargs entirely.

@dnwpark dnwpark force-pushed the type-intersection-complex branch from 43a9a86 to a879d65 Compare May 14, 2024 17:47
@dnwpark dnwpark force-pushed the type-intersection-complex branch from a879d65 to fea2cc0 Compare May 14, 2024 17:52
Base automatically changed from typeref-remove-intersection to master May 14, 2024 18:17
@dnwpark dnwpark marked this pull request as ready for review May 14, 2024 18:17
@dnwpark dnwpark force-pushed the type-intersection-complex branch from 57e6d50 to fea2cc0 Compare May 14, 2024 18:46
@dnwpark dnwpark merged commit b642dfa into master May 14, 2024
23 checks passed
@dnwpark dnwpark deleted the type-intersection-complex branch May 14, 2024 20:58
@dnwpark dnwpark mentioned this pull request May 14, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@elprans elprans elprans left review comments

@msullivan msullivan msullivan approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@dnwpark @msullivan @elprans