Fix crash when this is typed as a generic T with no type constraints

[jihndai]

Fixes #47965

While checking a protected property's accessibility, the crash occurs because getConstraintOfTypeParameter can return undefined when this is typed as the generic T and T has no type constraints on it. The fix for this was to check for undefined and refuse access if it was, as we can't guarantee that an instantiation of a generic T can access protected properties when T isn't constrained to any class/type.

Additionally, the explicit this type was also skipping through derivation checks done by isClassDerivedFromDeclaringClasses, which led to some inconsistent behavior shown below, so changes were also made to address this, but please do correct me if I'm mistaken.

class D {
  derived1(val: D1) {
    val.d1(); // currently shows an error, as expected
  }
  derived1ThisD(this: D, val: D1) {
    val.d1(); // should show the same error as above, but doesn't
  }
}
class D1 extends D {
  protected d1() {}
}

function thisString(this: string, d: D1) {
  /**
   * Error message seems misleading:
   * 
   * "Property 'd1' is protected and only accessible through an instance of class 'any'. This is an instance of class 'D1'"
   */
  d.d1();
}

[jihndai]

Nevermind, found the issues talking about private's behavior and protected's behavior in relation to this.

[sandersn]

I think the basic fixes are good. Just some minor style requests.

[sandersn]

This looks like an unrelated driveby cleanup, not part of either fix. Is that correct?

[jihndai]

correct just a cleanup, shouldn't change the behavior at all.

[sandersn]

I think I would inline this since it's only two lines and only called once.

[sandersn]

If it's OK to run the checks even for static methods (and I think it is), then the below is much less surprising to read:

Suggested change

	if (
	flags & ModifierFlags.Static
	|| !(enclosingClass = getEnclosingClassFromThisParameter(location))
	|| !(enclosingClass = isClassDerivedFromDeclaringClasses(enclosingClass, prop, writing))
	) {
	enclosingClass = getEnclosingClassFromThisParameter(location)) || isClassDerivedFromDeclaringClasses(enclosingClass, prop, writing);
	if (flags & ModifierFlags.Static || !enclosingClass) {

[jihndai]

Yeah should be ok, at best it was just a very minor optimization to avoid fetching this's parameter if the field was already static.

[jihndai]

getEnclosingClassFromThisParameter can return undefined while isClassDerivedFromDeclaringClasses doesn't accept undefined, so I'll have to do some type checking with this style instead:

enclosingClass = getEnclosingClassFromThisParameter(location);
enclosingClass = enclosingClass && isClassDerivedFromDeclaringClasses(enclosingClass, prop, writing);
if (flags & ModifierFlags.Static || !enclosingClass) {

[jihndai]

Also noticed that the name isClassDerivedFromDeclaringClasses may be a bit misleading, as it doesn't return a boolean, but instead it returns the first parameter if checks succeeds, otherwise it returns undefined.

Not sure if we'd want to change the name or not.

[sandersn]

Maybe as a follow-up, I'd rather get this change in.

[sandersn]

extremely minor style preference: I think the blank lines in this function should be deleted.