[CFE] computeTypeShapeCheckSufficiency is too conservative with nullability mismatches

[fishythefish]

I think returning .insufficient here is too conservative:

sdk/pkg/kernel/lib/type_environment.dart

Lines 296 to 299 in 7349ef5

	if (!IsSubtypeOf.basedSolelyOnNullabilities(
	expressionStaticType, checkTargetType)
	.inMode(subtypeCheckMode)) {
	return TypeShapeCheckSufficiency.insufficient;

(I assume SubtypeCheckMode.withNullabilities throughout this issue.)

Suppose we have

class Base<T> {}

class C<T> extends Base<T> {}

and suppose the type test in question is e is C<T> where e has static type Base<T>?. Based solely on nullabilities, Base<T>? is not a subtype of C<T>, so we get .insufficient and must do the full type test. However, it seems that .interfaceShape should suffice here, since e is C would still correctly handle the case where e is null.

[chloestefantsova]

Thanks for bringing attention to this, @fishythefish! Honestly, I don't remember why the nullability-based subtype check was introduced as the overall filter of the procedure, and I agree that it can be removed. I'm adding @mkustermann, who originally requested the helper procedure, just in case if the nullability check was meaningful for the original use case.

[mkustermann]

The main question we (dart2wasm) want answered whether it's sufficient to check for the nullability and type class or whether also type arguments have to be checked. We use it like this

  bool _staticTypesEnsureTypeArgumentsMatch(
      InterfaceType testedAgainstType, InterfaceType operandType) {
    assert(testedAgainstType.typeArguments.isNotEmpty);

    // If the operand type doesn't have any type arguments it will not be able
    // to tell us anything about the type arguments of testedAgainstType.
    if (operandType.typeArguments.isEmpty) return false;

    final sufficiency = typeEnvironment.computeTypeShapeCheckSufficiency(
        expressionStaticType: operandType,
        checkTargetType:
            testedAgainstType.withDeclaredNullability(Nullability.nullable),
        subtypeCheckMode: type_env.SubtypeCheckMode.withNullabilities);

    // If `true` the caller only needs to check nullabillity and the actual
    // concrete class, no need to check [testedAgainstType] arguments.
    return sufficiency == type_env.TypeShapeCheckSufficiency.interfaceShape;
  }

[fishythefish]

Thanks for the quick turnaround, @chloestefantsova!

And thanks for the info, @mkustermann! We've started doing something similar in dart2js: https://dart-review.googlesource.com/c/sdk/+/408124
Our main question is the same as yours. Having to check type arguments is more costly than having to handle null, so it's a shame if we have to perform a full type check due to a nullability issue even though the type arguments needn't be checked. I ran into this issue while trying to figure out why .withDeclaredNullability was needed.

Looking at the updated test expectations, I think there are still some more cases where we're unnecessarily conservative around null operands. Would it be reasonable to amend

sdk/pkg/kernel/lib/type_environment.dart

Lines 414 to 417 in a78fb5e

	InterfaceType? testedAgainstTypeAsOperandClass = hierarchy
	.getInterfaceTypeAsInstanceOfClass(
	checkTargetType, expressionStaticType.classNode)
	?.withDeclaredNullability(checkTargetType.declaredNullability);

to pass Nullability.nullable instead, as the backends end up doing anyway?

(Alternatively, maybe the right thing is to use expressionStaticType.toNonNull() here:

sdk/pkg/kernel/lib/type_environment.dart

Lines 470 to 475 in a78fb5e

	return isSubtypeOf(
	expressionStaticType,
	testedAgainstTypeAsOperandClass,
	SubtypeCheckMode.withNullabilities)
	? TypeShapeCheckSufficiency.interfaceShape
	: TypeShapeCheckSufficiency.insufficient;

I haven't thought carefully about the difference, if any.)

[chloestefantsova]

I haven't thought carefully about the difference, if any.)

Well, we basically want the nullabilities of the two types to not mater. Using SubtypeCheckMode.ignoreNullabilities in line 473 should do exactly that. Using Nullability.nonNullable in line 417 might do the opposite: since testedAgainstTypeAsOperandClass is tested to be the supertype of the check, making it non-nullable makes the same or less amount of tests pass in lines 470-475.

I've prepared the CL to verify the idea (https://dart-review.googlesource.com/c/sdk/+/408800), and surprisingly, the VM test were passing while some Dart2js tests started to fail. @fishythefish, could it be due to Dart2js code requiring some adjustment for the optimization?

[fishythefish]

I'm not sure we want .ignoringNullabilities since that gives us pre-NNBD semantics. We want to relax the case where the operand is null, but we don't want to ignore nullabilities at all levels of the check. For example:

class C<T> {}

class D<T> extends C<T> {}

bool foo(C<int?> x) => x is D<int>;

We should get .insufficient here. For example, x = D<int?>() would pass the interface shape check, but not the full subtype check. But with .ignoringNullabilities, we would get .interfaceShape. (Note that type_schema_environment_nnbd_test seems to be missing test cases of this variety - all the D/E cases only test variations in outermost nullability.)

I made a typo above (which I've now edited). I wrote .nonNullable, but of course that should be .nullable. From local testing, both this approach and the .toNonNull() approach I mentioned previously seem to match the expectations from your CL.

[chloestefantsova]

I'm not sure we want .ignoringNullabilities since that gives us pre-NNBD semantics.

Good point! I'm forgetting that the type arguments will still matter for the insufficient case. Then, making testedAgainstTypeAsOperandClass nullable should do the trick. PTAL: https://dart-review.googlesource.com/c/sdk/+/408800.