Skip to content

Conversation

@bbrehm
Copy link
Contributor

@bbrehm bbrehm commented Jul 15, 2021

This uses erased extensions to implement the operatorExtension API. It is an alternative to #1293 and relies on ShiftLeftSecurity/overflowdb-codegen#86

I guess we should move discussion of implementation details of the erased extensions (e.g. naming bikeshedding) to ShiftLeftSecurity/overflowdb-codegen#86 and move general discussion of this approach here.

So: Are we happy with erased extensions? The idea of erased extensions is that we add an unused covariant generic type parameter to our classes, and make a type-alias type oldClassName = newClassName[Any]. The parametric type gets erased, i.e. does not affect runtime behavior.

However, it participates in scala type inference and implicit conversions. Hence we can use it for DSL-style things like the operator extensions.

Since the operator extensions are supposed to be a simple example, I compressed it down to two files (make it easier for newcomers to grok the entire package).

Note that the erased extensions cannot be runtime-checked -- i.e. there can be no runtime type errors or x.isInstanceOf[Assignmant] checks.

Generally, I see three approaches to the underlying issue with subclassing the NodeRef classes:

  1. Give up on the almost-invariant that NodeRef JVM objects and logical graph nodes are in 1:1 correspondence. This is some work in odb, and constrains what we can do in an odb follow-up.
  2. Change the API for things like operator-extensions (something like Change operator extensions #1293)
  3. Use erased extensions (this PR)

cc @mpollmeier @fabsx00 @ml86 and @hubertp

@bbrehm bbrehm requested review from fabsx00, ml86 and mpollmeier July 15, 2021 11:25
@mpollmeier
Copy link
Contributor

Sorry but I don't think this is a big enough improvement from the status quo to justify adding more complexity to the generated code. Anyone who hasn't spent hours on this topic and is a also scala expert will have a hard time understand why we need these newly introduced covariant type parameters, which are even unused, so at first sight they look like bogus.

As you correctly point out, there's a bug in ODB where multiple NodeRefs for the same underlying NodeDb can lead to issues. This looks like it can be fixed in ODB, and the only reason we're hesitating to fix it is that in doesn't play any role in practice.
So in essence, from my perspective I'm voting for 1)

I'm guessing you're so persuasive on this topic because this "constrains what we can do in an odb follow-up", and that's fair enough. If the odb-followup is so good (fast, memory-efficient, ...) that we want to use it, then a change like this is not a deal-breaker. As it stands, while we're using ODB, I wouldn't want this change (especially the one for odb-codegen).

How about you carry on with the ODB-followup prototype and assume that we can (eventually) merge this PR. But for now we don't merge it.

@fabsx00 fabsx00 removed their request for review July 19, 2021 08:17
@fabsx00 fabsx00 closed this Aug 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants