Allow looking for properties inside the base of a mixin'd class

[orta]

Fixes #31426

I could see via a repl that:

getPropertyOfType(apparentType.getBaseTypes()[0], right.escapedText);

would return the right property, but at this point it was returning nothing. I couldn't find a way to convert my Type into an InterfaceType and so I built a helper to get towards having access to .getBaseTypes. From there it adds the extra checks.

[andrewbranch]

Assigned myself just because I’m interested in looking into this when I’m freed up a bit more and don’t want to forget about it

[weswigham]

Properties are supposed to get merged down into the types that inherit them - check out resolveObjectTypeMembers. You should never need to traverse "base types" to find a property like this, as base types are a syntactic construct only (we're a structural typesystem, after all). I would check out why the property isn't being included there, instead.

[andrewbranch]

I took a look at this, and the issue is that there’s a circularity resolving the base types IQuark while checking its declaration. As soon as we start resolving base types, we set type.resolvedBaseTypes to an empty array, which prevents actually crashing due to this circularity. During that resolution, something needs the resolved members of IQuark, which sees no base types because they’re still resolving, so it has its members set to an empty array.

Later, when we check a.value against IQuark, its base types have been resolved correctly, but because we’ve cached an empty members map, we don’t try to re-resolve them, so they appear empty.

The circularity that occurred was really deep (happened during the instantiation of the conditional InstanceType) and I didn’t put tons of effort into understanding exactly why it happened. It does seem like you should be able to substitute A in interface A extends B<T> {} for B<T>, so it seems like the best way forward is to see if the circularity can be eliminated. I was able to fix the error by clearing type.members if it changed while resolving base types since that seems like a good indicator that something went awry, but that feels pretty hacky.

[andrewbranch]

Oh, hang on—observe() returns an IQuark, so the circularity is real and clear! It’s even called out explicitly in #29872:

The workaround exists - to use different form of creating the type for the standalone mixin class - with interfaces... But this notation seems to drive crazy the IDE (the language server under the hood?). It stop finding usages of properties, show many false type errors etc.

Yep, because there’s a circularity that we falsely resolve and don’t report. I think the bug is that we don’t report a circularity error— the “workaround” is the bug.

And my earlier comment:

you should be able to substitute A in interface A extends B<T> {} for B<T>

It turns out the simple form B<T> (Mixin<typeof CQuark> in this test case) simply resolves to any in this case without triggering any errors, so that doesn’t actually work either, so I feel better about this interface example being an error. (I guess in the type reference case, we detect the circularity but fail to emit a noImplicitAny error on the way back out.)

[andrewbranch]

I think we should report circularity errors when resolving base types, but I would push this to 3.8 to avoid a last-minute breaking change.

[orta]

I think we should report circularity errors when resolving base types

I agree, I'd consider this one a bug and 29872 could be about supporting the actual feature

[andrewbranch]

#33050 originally fixed your case but it was scoped down due to performance reasons. You can see what I mean in the playground link from my comment above. SampleMixin1 and SampleMixin2 don’t qualify for deferred resolution of structure because they’re not tuple or array types. If you make them tuple or array types, your example no longer errors, but you’re stuck with tuple/array types where you didn’t want them.

[canonic-epicure]

I see.

Yup, how is the trick with tuples different from using the interface? I tried to continue your example, but indeed stuck when trying to access the nested property: link

Perhaps its possible to add some additional type, that will resolve the tuple type.. Something like this - which currently fails.

[andrewbranch]

It’s not possible. The trick with tuples/arrays is unique to tuples/arrays because they’re explicitly allowed to be recursive via #33050. If you pull the element out of the tuple/array, it’s no longer a tuple/array; it’s an indexed access type and it no longer falls into the special case set up by #33050. As far as I can tell, the only way to get what you’re really asking for is to expand the cases where recursive type aliases are allowed to include generic instantiations.

[canonic-epicure]

As far as I can tell, the only way to get what you’re really asking for is to expand the cases where recursive type aliases are allowed to include generic instantiations.

@andrewbranch Is this something that #33050 provides? If so, can it be enabled by some flag? #33050 says its not enabled globally, because of the 5% performance hit. I believe for many people, the value of the circular types is much more than those 5%.

[andrewbranch]

Is this something that #33050 provides?

No, it is not.

[andrewbranch]

Proposal:

1. Close this
2. Open an issue tracking the missing circularity error, but put it on the backlog and note that a preferable solution would be to defer resolution to avoid the circularity, as discussed in the thread above

[canonic-epicure]

@andrewbranch I'm a bit concerned of your comment in the conversation with the weswigham

I think the bug is that we don’t report a circularity error— the “workaround” is the bug.

If the circularity error will close the remaining escape route (workaround) for the circular references on mixin classes - thats going to break our codebase, which is already in production. Should be approached with the alternative solution for deferred types resolution.

[orta]

Closing this PR as discussed in the chat above, it's very likely that someone looking at this again will not be taking the same route