fix #25183, add support to ErrorVerifier for generic methods

fix #25183, add support to ErrorVerifier for generic methods

R=brianwilkerson@google.com, leafp@google.com

Committed: https://github.com/dart-lang/sdk/commit/0e09465f0c093f707a84e93264cea3ff3fb6d3ae

[Jennifer Messerly, 2015-12-09 21:18:11]

jmesserly@google.com changed reviewers:
+ brianwilkerson@google.com, leafp@google.com

[Jennifer Messerly, 2015-12-09 21:18:11]

Here's the fix for https://github.com/dart-lang/sdk/issues/25183. I'm not super
happy with the code duplication (see TODOs), but that seems like the existing
pattern.

I think there's probably some ways to fold this into TypeSystem, so we could
ideally just compute errors when we see the subtype check fail.

Another idea is to instead rely on CodeChecker, and just skip the code here if
we're in strong mode. However I feel like longer term we need to integrate the
two. And generic methods should be designed to work without strong mode.
Speaking of that -- it'd be pretty easy to remove the "is StrongTypeSystemImpl"
check, if we create a common base class for the spec type system and the strong
one, so I can move the instantiateToBounds helper there.

Thoughts?

[Brian Wilkerson, 2015-12-09 21:58:44]

LGTM

> I'm not super happy with the code duplication ...

Me either.

> I think there's probably some ways to fold this into TypeSystem,
> so we could ideally just compute errors when we see the subtype
> check fail.

One problem we've encountered before (which suggests that this isn't a good
approach) is that when a method like that starts getting called from multiple
places we start producing duplicated errors, which isn't good for users.

Perhaps the TypeSystem method could return enough information to allow the
creation of an error.

> Another idea is to instead rely on CodeChecker, and just skip the
> code here if we're in strong mode. However I feel like longer term
> we need to integrate the two. And generic methods should be
> designed to work without strong mode.

Agreed and agreed.

> Speaking of that -- it'd be pretty easy to remove the
> "is StrongTypeSystemImpl" check, if we create a common base
> class for the spec type system and the strong one, so I can
> move the instantiateToBounds helper there.

We already have the interface TypeSystem to which such a method could be added.
If the implementation is the same between the two concrete classes then, yes, a
common base class makes sense.

[Leaf, 2015-12-09 22:02:29]

lgtm, thanks!

https://codereview.chromium.org/1514683002/diff/1/pkg/analyzer/lib/src/genera...
File pkg/analyzer/lib/src/generated/error_verifier.dart (right):

https://codereview.chromium.org/1514683002/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/error_verifier.dart:1353: // however
"instantiateToBounds" is currently on StrongTypeSystemImpl.
The non-strong mode semantics might be different, so I suspect that
instantiateToBounds will end up with two different implementations (I think the
non-strong mode POR is that it instantiates with dynamic).  So it might make
sense to put it on the interface and have two different versions.  On the other
hand, I think in non-strong mode you may be able to implement this simply by
leaving the variables un-instantiated (since you're checking assignability
instead of subtyping).  I don't think this works here because we do subtyping
with proper variance.

https://codereview.chromium.org/1514683002/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/error_verifier.dart:1358: // an error message
once it's ready to "return false".
Yeah, this seems like it would work out well.  Maybe just by passing in an
optional callback to call before failing which yields the error.

https://codereview.chromium.org/1514683002/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/error_verifier.dart:1413: // Proceed with the
rest of the checks, using instantiated types.
The error messages that we get from the rest of the checks may be less than
ideal in the case that the programmer used a different name for a type parameter
in the overriding method.  Maybe add a TODO to consider how to present this
nicely?

https://codereview.chromium.org/1514683002/diff/1/pkg/analyzer/test/generated...
File pkg/analyzer/test/generated/resolver_test.dart (right):

https://codereview.chromium.org/1514683002/diff/1/pkg/analyzer/test/generated...
pkg/analyzer/test/generated/resolver_test.dart:13008: void
test_genericMethod_override_invalidIypeParamBounds() {
InvalidIype -> InvalidType

[Jennifer Messerly, 2015-12-09 22:08:11]

On 2015/12/09 21:58:44, Brian Wilkerson wrote:
> LGTM
> 
> > I'm not super happy with the code duplication ...
> 
> Me either.
> 
> > I think there's probably some ways to fold this into TypeSystem,
> > so we could ideally just compute errors when we see the subtype
> > check fail.
> 
> One problem we've encountered before (which suggests that this isn't a good
> approach) is that when a method like that starts getting called from multiple
> places we start producing duplicated errors, which isn't good for users.

Oh yeah, definitely! The ability to produce errors would be turned on in some
very restricted set of circumstances (like right here in ErrorVerifier).

e.g.

class StrongTypeSystemImpl /*or SpecTypeSystem... */ {
   ... isSubtypeOf(..., {errorReporter}) {
    if (... return type mismatch ...) {
      errorReporter?.wrongReturnType(...);
      return false;
    }
  }
}

That's one way to structure it... return value would definitely work too. A pair
of methods might do it:

Error checkSubtypeOf(...) {
}

bool isSubtypeOf(...) {
  return checkSubtypeOf(...) == null;
}

the possible downside is we'll always build the error object, whether or not
it's needed.

> 
> Perhaps the TypeSystem method could return enough information to allow the
> creation of an error.
> 
> > Another idea is to instead rely on CodeChecker, and just skip the
> > code here if we're in strong mode. However I feel like longer term
> > we need to integrate the two. And generic methods should be
> > designed to work without strong mode.
> 
> Agreed and agreed.
> 
> > Speaking of that -- it'd be pretty easy to remove the
> > "is StrongTypeSystemImpl" check, if we create a common base
> > class for the spec type system and the strong one, so I can
> > move the instantiateToBounds helper there.
> 
> We already have the interface TypeSystem to which such a method could be
added.
> If the implementation is the same between the two concrete classes then, yes,
a
> common base class makes sense.

[Jennifer Messerly, 2015-12-09 22:37:41]

Thanks! PTAL at changes, especially TypeSystem

https://codereview.chromium.org/1514683002/diff/1/pkg/analyzer/lib/src/genera...
File pkg/analyzer/lib/src/generated/error_verifier.dart (right):

https://codereview.chromium.org/1514683002/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/error_verifier.dart:1353: // however
"instantiateToBounds" is currently on StrongTypeSystemImpl.
On 2015/12/09 22:02:29, Leaf wrote:
> The non-strong mode semantics might be different, so I suspect that
> instantiateToBounds will end up with two different implementations (I think
the
> non-strong mode POR is that it instantiates with dynamic).  So it might make
> sense to put it on the interface and have two different versions.  On the
other
> hand, I think in non-strong mode you may be able to implement this simply by
> leaving the variables un-instantiated (since you're checking assignability
> instead of subtyping).  I don't think this works here because we do subtyping
> with proper variance.

Done. Fixed this TODO by implementing the right logic in the spec type system

https://codereview.chromium.org/1514683002/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/error_verifier.dart:1358: // an error message
once it's ready to "return false".
On 2015/12/09 22:02:29, Leaf wrote:
> Yeah, this seems like it would work out well.  Maybe just by passing in an
> optional callback to call before failing which yields the error.

Sounds good. I don't want to try that restructure yet but as we fold in
CodeChecker I think it would be a great time.

https://codereview.chromium.org/1514683002/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/error_verifier.dart:1413: // Proceed with the
rest of the checks, using instantiated types.
On 2015/12/09 22:02:29, Leaf wrote:
> The error messages that we get from the rest of the checks may be less than
> ideal in the case that the programmer used a different name for a type
parameter
> in the overriding method.  Maybe add a TODO to consider how to present this
> nicely?

Eeeep good catch. I've switch it to use the "overridingFT" name, in other words,
p1.name. Added test too!


With that fixed, I *think* this will be okay, since the errors are in terms of
the method you're looking at. The phrasing of Analyzer's existing error messages
kind of takes that into account. For example:

abstract class B<T> {
  Iterable<T> method();
}

abstract class D<S> extends B<S> {
  String method();
}
// [warning] The return type 'String' is not assignable to 'Iterable<S>' as
required by the method it is overriding from 'B'
(/Users/jmesserly/scratch/test.dart, line 6, col 10)

... it's in terms of your generic parameter 'S' even though if you go over to
'B' you'll see 'T'. So it tells you what you can put in there to make it work.

https://codereview.chromium.org/1514683002/diff/1/pkg/analyzer/test/generated...
File pkg/analyzer/test/generated/resolver_test.dart (right):

https://codereview.chromium.org/1514683002/diff/1/pkg/analyzer/test/generated...
pkg/analyzer/test/generated/resolver_test.dart:13008: void
test_genericMethod_override_invalidIypeParamBounds() {
On 2015/12/09 22:02:29, Leaf wrote:
> InvalidIype -> InvalidType

Done.

[Brian Wilkerson, 2015-12-09 23:02:28]

LGTM

[Jennifer Messerly, 2015-12-09 23:16:03]

Description was changed from

==========
fix #25183, add support to ErrorVerifier for generic methods
==========

to

==========
fix #25183, add support to ErrorVerifier for generic methods

R=brianwilkerson@google.com, leafp@google.com

Committed:
https://github.com/dart-lang/sdk/commit/0e09465f0c093f707a84e93264cea3ff3fb6d3ae
==========

[Jennifer Messerly, 2015-12-09 23:16:04]

Committed patchset #3 (id:40001) manually as
0e09465f0c093f707a84e93264cea3ff3fb6d3ae (presubmit successful).