Skip to content
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

dart2js: bad optimization of "x is Null" -- what does TypeMask.isEmpty mean? #16407

Closed
rakudrama opened this issue Jan 30, 2014 · 2 comments
Closed
Labels
P2 A bug or feature request we're likely to work on type-bug Incorrect behavior (everything from a crash to more subtle misbehavior) web-dart2js

Comments

@rakudrama
Copy link
Member

main() {
  foo('null', null);
  foo('"x"', 'x');
  print('null is Null => ${null is Null}');
}

var foo = (s, v) {
  print('$s is Null = ${v is Null}');
};

This program prints:
null is Null = false
"x" is Null = false
null is Null => true

The problem is that 'v is Null' is replaced with 'false'.

SsaInstructionSimplifier.visitIs:

    } else if (!RuntimeTypes.hasTypeArguments(type)) {
      TypeMask expressionMask = expression.instructionType;
      TypeMask typeMask = (element == compiler.nullClass)
          ? new TypeMask.subtype(element)
          : new TypeMask.nonNullSubtype(element);
      if (expressionMask.union(typeMask, compiler) == typeMask) {
        return graph.addConstantBool(true, compiler);
      } else if (expressionMask.intersection(typeMask, compiler).isEmpty) {
                      ^^^
        return graph.addConstantBool(false, compiler);
      }
    }

expressionMask.intersection(typeMask, compiler) evaluates to a type that prints as '[null]'.
This type returns true for .isEmpty.

I would expect .isEmpty to return true only for the type representing a completely empty set of values, that does not include a null value.
However, '.isEmpty' seems to be orthogonal to '.isNullable'.
There are several places in the intersection, union and other algorithms that seem to assume that the type representing the singleton set of values {null} is also .isEmpty
So it is not trivial to strengthen .isEmpty to my expected meaning.

I think we should strength isEmpty to make t1.intersection(t2).isEmpty a reliable test, instead of having to also check the result is non-nullable like this code from types_propagation.dart:

    TypeMask outputType = checkedType.intersection(inputType, compiler);
    if (outputType.isEmpty && !outputType.isNullable) {

If we keep isEmpty orthogonal to isNullable, TypeMask.isEmpty really needs to be documented!

@sigmundch
Copy link
Member

Note that the test above will stop failing when I land a change that calculates isDisjoint explicitly (I modified the line in SSA with the problem illustrated by Stephen above). However, the issue still exists in the type mask abstraction: it would be nice if isEmpty really means empty, not empty-or-null

@kevmoo kevmoo added P2 A bug or feature request we're likely to work on type-bug Incorrect behavior (everything from a crash to more subtle misbehavior) and removed triaged labels Mar 1, 2016
@rakudrama
Copy link
Member Author

This is fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P2 A bug or feature request we're likely to work on type-bug Incorrect behavior (everything from a crash to more subtle misbehavior) web-dart2js
Projects
None yet
Development

No branches or pull requests

3 participants