Skip to content

language reference entry for non-extending subtypes #6592

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.

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

Merged
merged 10 commits into from
Sep 12, 2021

Conversation

ginsbach
Copy link
Contributor

@ginsbach ginsbach commented Sep 2, 2021

@ginsbach ginsbach marked this pull request as ready for review September 2, 2021 14:28
Copy link
Contributor

@smowton smowton left a comment

Choose a reason for hiding this comment

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

Is this functionally identical to extending Foo's base types and writing this instanceof Foo in Bar's charpred? If so, that might be useful to mention.

ginsbach and others added 2 commits September 2, 2021 17:30
@ginsbach
Copy link
Contributor Author

ginsbach commented Sep 2, 2021

Is this functionally identical to extending Foo's base types and writing this instanceof Foo in Bar's charpred? If so, that might be useful to mention.

It isn't quite the same. To be specific, it goes beyond just adding it to the charpred in two ways:

  • the relationship is recorded in the type hierarchy, making type-based optimisations stronger
  • it is possible to access member predicates through the super keyword

Copy link
Contributor

@alexet alexet left a comment

Choose a reason for hiding this comment

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

I think being clearer about which things care about extends and which are instanceof earlier in this document may help.

I think the comparison to instanceof would actually be good saying something like
"Foo instanceof Baris reoughly equivalent to sayingthis instanceof Barin the charpred. The main differences are that you can call methods onBarviasuper` and you can get better optimisation.

class Bar extends Interface instanceof Foo {
override string foo() { result = "bar" }
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Again I think the overlapping instanceof/extends hierarchy is just confusing here. I would have a simpler example like the following:

    class Interface instanceof int {
      Interface() { this in [1 .. 100] }
      string foo() { result = "" }
   }

    class Foo extends int {
      Foo() { this in [1 .. 10] }
      string foo() { result = "foo" }
    }

    class Bar extends Interface instanceof Foo {
      override string foo() { result = "bar" }
    }

Here we don't need to understand the subtle differences between rootdefs and where exactly overriding happens.

Also your current example seems wrong select any(Foo b).foo() should call Interface::foo which should include "bar" as Bar::foo overrides Interface::foo. This seems an implementation bug.

Copy link
Contributor Author

@ginsbach ginsbach Sep 3, 2021

Choose a reason for hiding this comment

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

I have opened an issue (https://github.com/github/codeql-coreql-team/issues/1513) for the implementation bug.

I attempted to take your example, but the toString methods became messy. Does the following make sense to you (commit 9)?

    class Interface extends int {
      Interface() { this in [1 .. 10] }
      string foo() { result = "" }
   }

    class Foo extends int {
      Foo() { this in [1 .. 5] }
      string foo() { result = "foo" }
    }

    class Bar extends Interface instanceof Foo {
      override string foo() { result = "bar" }
    }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The implementation bug should be fixed now: https://github.com/github/semmle-code/pull/40188

@ginsbach
Copy link
Contributor Author

ginsbach commented Sep 3, 2021

Thank you for the detailed suggestions @alexet !

Foo instanceof Baris reoughly equivalent to sayingthis instanceof Barin the charpred. The main differences are that you can call methods onBarviasuper` and you can get better optimisation.

I have added a sentence like this in commit 8.

@ginsbach ginsbach requested a review from alexet September 3, 2021 15:15
Co-authored-by: Nick Rolfe <nickrolfe@github.com>
@ginsbach ginsbach added the ready-for-doc-review This PR requires and is ready for review from the GitHub docs team. label Sep 9, 2021
@ginsbach ginsbach merged commit 131d63c into main Sep 12, 2021
@ginsbach ginsbach deleted the ginsbach/instanceofDocs branch September 12, 2021 14:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation ready-for-doc-review This PR requires and is ready for review from the GitHub docs team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants