Is it actually safe to just remove these, or do we need to go through a deprecation period?

Is it actually safe to just remove these, or do we need to go through a deprecation period?

Good question. I just wanted to put up the diff for suggestion. The PR actually removes a whole bunch of other predicates as well that were previously inherited from Element - changing the type hierarchy like this is always tricky in terms of potential breakage. Maybe all of the removed predicates should be re-added as deprecated, but no longer inherited nor overriding.

It's certainly breaking some tests. Do you perhaps want to take it from here? I don't feel strongly enough about this change to spend a lot of time on it.

It's certainly breaking some tests. Do you perhaps want to take it from here? I don't feel strongly enough about this change to spend a lot of time on it.

I'll have a closer look.

It's also perfectly fine to re-add

Location getLocation() {
    result.getContainer() = this and
    result.hasLocationInfo(_, 0, 0, 0, 0)
  }

on File without extending Locatable. A class doesn't need to extend Locatable to be able to provide a getLocation predicate.

It's certainly breaking some tests.

I think it's breaking almost all the tests.

I've been looking at this a bit. I think File will still need to extend Element. Not having this just breaks too much code (code I'm not really willing to touch) and that code will need special cases for Files if I do touch it. There might still be consequences of Folder not having a getLocation, but those somewhat hidden for me due to all test regressions related to File not extending Element.

I think File will still need to extend Element.

Sounds fine. In that case it might as well extend Locatable, since that doesn't add any member predicates, but merely indicates that getLocation can be expected to give something reasonable (which it does).

I think File will still need to extend Element.

Sounds fine. In that case it might as well extend Locatable, since that doesn't add any member predicates, but merely indicates that getLocation can be expected to give something reasonable (which it does).

This fixes all the tests (see the internal PR that now links to this one).

Given the nature of this change, I'm tempted to make this one breaking instead of having a deprecation period. However, I'll need to check with some people that this reasonable.

The changes LGTM! We should maybe run DCA on this to ensure that we didn't perturb any join orders, though?

Ran on the internal PR.

DCA seems somewhat wobbly, but analysis times are unaffected.

🎉 Thanks for taking over and getting this merged.

🎉 Thanks for taking over and getting this merged.

You're welcome!