Unsound implicit cast when using conditional expression

[jakobr-google]

dartanalyzer version 1.21.0-dev.3.0

The analyzer complains about the conditional expression in the code below: [warning] Unsound implicit cast from 'Object' to 'Hest<dynamic, Fisk>'.

No complaints for the if-else.

class Fisk<D> {}
class LinearFisk extends Fisk<double> {}
class OrdinalFisk<D> extends Fisk<D> {}

class Hest<D, S extends Fisk<D>> {}

class OrdinalHest<D> extends Hest<D, OrdinalFisk<D>> {}
class NumericHest extends Hest<double, LinearFisk> {}

void main() {
  final check = true;

  // Fails: Unsound implicit cast from Object to Hest<dynamic, Fisk<dynamic>>
  Hest hest = check ? new OrdinalHest() : new NumericHest();

  // No errors reported in the code below.
  if (check) {
    hest = new OrdinalHest();
  } else {
    hest = new NumericHest();
  }
}

[floitschG]

The problem here is that the least upper ound of OrdinalHest and NumericHest is computed as Object.

@leafpetersen, @lrhn, @eernstg: is this a duplicate of our LUB bug #27525, or does this fall into the partial/raw type category, or is this just a bug in the analyzer?

[floitschG]

forgot @munificent

[lrhn]

I can't see if this is strong-mode or not. I'm guessing where this is coming from, so it probably is.

If not, then the actual types of the conditional expression are:

• Ordinalhest : Hest<Dynamic, OrdinalFisk : Fisk> : Object
• NumericHest : Hest<double, LinearFisk : Fisk> : Object

These two type expansions have nothing in common except Object.
There are generic supertypes in common, but that's not what LUB looks at (and I doubt it ever will).

If it is strong mode, I'm not absolutely sure how types will be inferred for the OrdinalHest. There is no context for the Hest type parameter, and we don't want to infer from one branch to another. so new OrdinalHest() must still either give up or infer dynamic or Object.
Then we are back to unifying Hest<double, ...> with Hest<dynamic, ...>.

I don't think it's an analyzer bug.

[jakobr-google]

Yeah, I forgot to mention. This is strong mode. No complaints from the analyzer in "weak" mode.

I can bind the type parameters further (in an attempt to please the analyzer -- an OrdinalHest clearly shouldn't be a Hest<double, ...>):

class Fisk<D> {}
class LinearFisk extends Fisk<double> {}
class OrdinalFisk extends Fisk<double> {}

class Hest<D, S extends Fisk<D>> {}

class OrdinalHest extends Hest<double, OrdinalFisk> {}
class NumericHest extends Hest<double, LinearFisk> {}

void main() {
  final check = true;

  // Fails: Unsound implicit cast from Object to Hest<double, Fisk<double>>
  Hest<double, Fisk<double>> hest = check ? new OrdinalHest() : new NumericHest();

  // No errors reported in the code below.
  if (check) {
    hest = new OrdinalHest();
  } else {
    hest = new NumericHest();
  }
}

Still no luck. The problem (or feature as designed?) is the second type parameter. If I put in Fisk<double> instead of OrdinalFisk and LinearFisk, the analyzer stops complaining.

I can probably structure the code in another way to work around this, but it goes against my intuition that the conditional expression and if-else should behave the same way in this case, so I thought I'd file this issue.

[eernstg]

Just trying it out shows that you only get the message (with dartanalyzer version 1.21.0-dev.9.0) in strong mode, and the message comes from _implicitCastMessage in sdk/pkg/analyzer/lib/src/error/codes.dart which is declared in a class with This class has Strong Mode specific error codes. in its dartdoc comment. So this is strong-mode only.

That message is used for the errors DOWN_CAST_COMPOSITE, DOWN_CAST_IMPLICIT, DOWN_CAST_IMPLICIT_ASSIGN, DYNAMIC_CAST, and ASSIGNMENT_CAST, and it turned out to be DOWN_CAST_COMPOSITE in this case. That case is chosen by the following code in lib/src/task/strong/checker.dart:

    // Composite cast: these are more likely to fail.
    bool downCastComposite = false;
    if (!rules.isGroundType(to)) {
      // This cast is (probably) due to our different treatment of dynamic.
      // It may be more likely to fail at runtime.
      if (from is InterfaceType) {
        // For class types, we'd like to allow non-generic down casts, e.g.,
        // Iterable<T> to List<T>.  The intuition here is that raw (generic)
        // casts are problematic, and we should complain about those.
        var typeArgs = from.typeArguments;
        downCastComposite =
            typeArgs.isEmpty || typeArgs.any((t) => t.isDynamic);
      } else {
        downCastComposite = !from.isDynamic;
      }
    }

In this case we have Hest<dynamic, Fisk<dynamic>> as to and Object as from, so to is a non-ground type and from is an InterfaceType. Being a ground type is detected here:

  bool isGroundType(DartType t) {
    // TODO(leafp): Revisit this.
    if (t is TypeParameterType) {
      return false;
    }
    if (_isTop(t)) {
      return true;
    }

    if (t is FunctionType) {
      if (!_isTop(t.returnType) ||
          anyParameterType(t, (pt) => !_isBottom(pt, dynamicIsBottom: true))) {
        return false;
      } else {
        return true;
      }
    }

    if (t is InterfaceType) {
      List<DartType> typeArguments = t.typeArguments;
      for (DartType typeArgument in typeArguments) {
        if (!_isTop(typeArgument)) return false;
      }
      return true;
    }

    // We should not see any other type aside from malformed code.
    return false;
  }

So Hest<dynamic, Fisk<dynamic>> is non-ground because it has a non-top type argument (Fisk..).

I guess dartanalyzer --strong is a bit too cautious about the typing situation here, because every instance of Hest should have type Hest<Object, Fisk<Object>> or a subtype thereof, and even that annotation (stated explicitly in the program) doesn't make the warning go away. The warning persists with Hest<Object, Fisk<Object>> hest = null as Object so OrdinalHest and NumericHest have nothing to do with the whole thing (as Lasse mentioned): Anything of type Object will do.

[eernstg]

If I put in Fisk instead of OrdinalFisk and LinearFisk, the analyzer stops complaining.

With the 2nd version of the program you gave, the conditional expression will then have type Hest<double, Fisk> so there is no downcast at all with Hest<double, Fisk> hest = .. and the downcast doesn't have to "invent" type arguments with Hest hest = .. so that goes without hints as well.

[leafpetersen]

This is a least upper bound issue. Strong mode (in this case) is using the spec version of least upper bound, and the spec definition gives Object as the least upper bound of OrdinalHest and LinearHest. As @jakobr-google mentions, if you use Fisk instead of OrdinalFisk and LinearFisk:

class OrdinalHest extends Hest<double, Fisk> {}
class NumericHest extends Hest<double, Fisk> {}

then the spec mode (and hence the strong mode as well) least upper bound of OrdinalHest and LinearHest is Hest<double, Fisk> as expected.

I believe that the proposal that I made for replacing spec mode least upper bound in strong mode would do the right thing for this example (both variants).

[eernstg]

I'd think that there is an issue which has nothing to do with least upper bounds. The program doesn't need to contain any conditional expressions at all, and we can still get "Unsafe implicit cast from 'Object' to 'Hest<Object, Fisk>'" from dartanalyzer --strong ..:

class Fisk<D> {}
class Hest<D, S extends Fisk<D>> {}

void main() {
  Hest<Object, Fisk<Object>> hest = null as Object; // Warning here.
}

Of course, it is always safe to warn about a problem that may or may not be there. But I believe that we cannot have an instance of Hest which does not have type Hest<Object, Fisk<Object>> or a subtype thereof, so this particular downcast doesn't have to be characterized as unsafe. I'm not quite sure how difficult it would be, in the given context, to see that it is indeed safe.

[leafpetersen]

The unsafe implicit downcast warning exists in strong mode to help users transition from non-strong mode platforms. As strong mode becomes more broadly supported, I think we should just eliminate this warning.

[leafpetersen]

The implicit cast warning is no longer on by default, and the least upper bound issues are tracked separately. Closing this one.