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

Confusing warning when improperly typed closure passed to map #26992

Closed
stereotype441 opened this issue Jul 30, 2016 · 16 comments

Comments

@stereotype441
Copy link
Member

commented Jul 30, 2016

The following ill-typed code:

    void f(List<String> x, List<String> y) {
      x.addAll(y.map((String z) => 1.0));
    }

produces the strong mode warning Unsound implicit cast from Iterable<dynamic> to Iterable<String>.

This is confusing because there's nothing dynamic in the above code. I would expect one of the following behaviors:

  • (String z) => 1.0 is inferred to have type (String) -> double, therefore y.map((String z) => 1.0) is inferred to have type Iterable<double>, but x.addAll requires Iterable<String>, therefore the message is Unsound implicit cast from Iterable<double> to Iterable<String>.
  • Since x.addAll requires Iterable<String>, the type argument to y.map is inferred to be String, therefore y.map requires type (String) -> String. But the type of (String z) => 1.0 is (String) -> double, therefore the message is unsound implicit cast from (String) -> double to (String) -> String.
@jmesserly

This comment has been minimized.

Copy link
Contributor

commented Aug 1, 2016

The failure mode of type inference with type parameters is to leave it with the implicit instantiation (the upper bound, usually dynamic but can be another type, e.g. num if Rectange<T extends num>).

Type of map is <S>(T -> S) -> Iterable<S>. Inference fails because double <: S and S <: String and no type can satisfy that. We know inference failed; if we wanted we could even list the constraints. But strong mode inference is not supposed to generate errors, in the current design. It only fixes them :)

We have discussed some kind of flag like "report inference failures" (#26781) that would change this behavior. But it's not clear if that's the right answer.

We also have no-implicit-casts flag now, which would make that cast failure into an error, and no-implicit-dynamic, which helps find implicit dynamic instantiations.

@jmesserly

This comment has been minimized.

Copy link
Contributor

commented Aug 1, 2016

CC @leafpetersen ... I guess the TL;DR from my message is, we want to make this better, but current behavior is by design. It might make sense to merge this into #26781 .

(Another related bug is #25490 to finish implementing inference.)

@bwilkerson

This comment has been minimized.

Copy link
Member

commented Aug 1, 2016

IMO, it would be much better from a user's perspective to pick one inference direction as the dominant direction and produce one of the two messages Paul was expecting. The current failure mode makes it hard for users to understand what the real problem is.

We have discussed some kind of flag like "report inference failures" (#26781) that would change this behavior. But it's not clear if that's the right answer.

I think it's the wrong answer. The tools should produce a clear and understandable error message without having to be run again with certain flags enabled.

We also have no-implicit-casts flag now, which would make that cast failure into an error, and no-implicit-dynamic, which helps find implicit dynamic instantiations.

My expectation is that those are either going to become the default behavior, or will be dropped, but in either case there should (eventually) only be one behavior.

@jmesserly

This comment has been minimized.

Copy link
Contributor

commented Aug 1, 2016

IMO, it would be much better from a user's perspective to pick one inference direction as the dominant direction and produce one of the two messages Paul was expecting. The current failure mode makes it hard for users to understand what the real problem is.

absolutely. The thing to understand is right now we don't issue an error, so we're talking about introducing an error where one does not currently exist. Right now you're totally allowed to write:

Iterable<dynamic> iter = y.map<dynamic>((String z) => 1.0));
List<String> list;
list.addAll(iter); // we suspect a cast failure here, but aren't sure

Does that make sense? There's a warning because we suspect (but don't know for certain) a cast failure at runtime.

But we're all in agreement this is suboptimal. It's quite an interesting balancing act trying to get to a stronger type system from where we started in Dart 1 :)

I think it's the wrong answer. The tools should produce a clear and understandable error message without having to be run again with certain flags enabled.

My expectation is that those are either going to become the default behavior, or will be dropped, but in either case there should (eventually) only be one behavior.

Absolutely, 200% agree! The flags aren't a long term solution. We are trying to find our way to a "stronger mode" without breaking folks. It's not easy to figure out how to get there from where we're at. It may seem obvious what to do in this particular example, but tightening the system can also cause working code to be rejected. That's why it's a balancing act and some exploration is needed...

@bwilkerson

This comment has been minimized.

Copy link
Member

commented Aug 1, 2016

To be clear, I wasn't suggesting that we not explore the possibilities, only that we not rely on a flag that might disappear as an answer to this problem. (Nor was I assuming that you were proposing doing so.)

Yes, it is hard to deal with backward compatibility. I find that sometimes it helps me do so if I know what the end goal is, hence my attempt to articulate the end goal. But I have no delusions that even knowing the end goal makes it easy to get there. :-)

@jmesserly

This comment has been minimized.

Copy link
Contributor

commented Aug 1, 2016

Makes sense. The end goal (from chats with @leafpetersen) is to not have implicit casts. In which case having a context type (here, Iterable<String>) will require us to either be able to type the expression as Iterable<String> or issue an error that we cannot.

Once we're finishing inference (#25490) we'll be able to make the message more precise as well. Ideally downwards inference is driving the error message (it can narrow the error better), but that'd be tricky to express in the current (incomplete) implementation.

@leafpetersen

This comment has been minimized.

Copy link
Member

commented Aug 2, 2016

The main issue here is about what to do when inference fails. We currently choose to simply fall back to dynamic. I think this is not ideal - this makes the results of inference extra confusing because it tends to just punt off the error to some other location. This is what happens here: we fall back to something that can't possibly work. Often the result will be runtime error - here you just get a confusing static warning.

I would prefer that we just issue an error in this case. This was the main thing that I wanted out of --no-implicit-dynamic. This one is extra bad, because it's an inference failure that results from being over-constrained rather than under-constrained. I think it should be the case that in all non-trivial (i.e. non-null cases), if inference is over-constrained you are guaranteed to get a runtime error. So we probably really should make at least that case always an error. The under-constrained case is less clear cut, since it's possible that dynamic will work.

Also, just to clarify - strong mode inference in general can introduce new static errors, but currently there are no errors that are specifically about inference (e.g. that inference has failed).

@jmesserly

This comment has been minimized.

Copy link
Contributor

commented Aug 2, 2016

yeah, if we're willing to go there with strong mode/dart2, it's easy to do. We have all the info in hand at the point of inference:

// Inference failed. Bail.

just need to flow in an error reporter. The calling code will typically have one in hand.

NOTE: unfortunately we call this during resolution. This means it will hit the existing Analyzer bug, where errors from resolution are dropped on the floor if you call context.computeErrors and context.resolveCompilationUnit in the wrong order :( :( :(

@jmesserly

This comment has been minimized.

Copy link
Contributor

commented Aug 4, 2016

I found a somewhat related issue, wanted to document it here. Consider a case like:

int x = new Future.value('hi');

Here, we will think inference "succeeded" and infer <String>. Maybe that's okay, because the code will indeed produce an error (that Future<String> is not assignable to int) but it surprised me. It happens because in the "inferring type system", we call isSubtypeOf to record constraints, and we don't bother to check if the subtype test can be satisfied at all (in other words, if isSubtypeOf returns false). IIRC, I tried that initially but it's a tad tricky, as we'd need to answer "maybe" for type parameters we're inferring, which gets into some tri-state logic.

@jmesserly jmesserly added the blocked label Aug 9, 2016

@jmesserly

This comment has been minimized.

Copy link
Contributor

commented Aug 9, 2016

Hey @stereotype441 @scheglov @bwilkerson -- do y'all have a bug for errors getting dropped during resolution? What we have seen in DDC is, if we call context.resolveCompilationUnit followed by context.computeErrors, we are missing errors from resolution in some cases. This can be seen by stepping through the code, it will have a null error reporter. We notice this in strong mode because our "inferred type" hints get dropped sometimes. But for that reason, we don't report any warnings/errors from ResolverVisitor.

The reason I ask is: fixing this bug is essentially blocked on that issue being addressed.

edit: oh, and it's also reproduced in checker_test/inferred_type_test, because it resolves stuff in a similar order as DDC

@scheglov

This comment has been minimized.

Copy link
Contributor

commented Aug 10, 2016

@jmesserly Do you know which test in checker_test/inferred_type_test has this problem, and which error is dropped?

@jmesserly

This comment has been minimized.

Copy link
Contributor

commented Aug 10, 2016

here's a patch that will print the dropped inference messages:

diff --git a/pkg/analyzer/lib/src/generated/error.dart b/pkg/analyzer/lib/src/generated/error.dart
index bedfdd8..b88658f 100644
--- a/pkg/analyzer/lib/src/generated/error.dart
+++ b/pkg/analyzer/lib/src/generated/error.dart
@@ -3198,6 +3198,19 @@ class ErrorReporter {

   Source get source => _source;

+  bool get isNullListener =>
+      _errorListener == AnalysisErrorListener.NULL_LISTENER;
+
+  void printErrorForNode(ErrorCode errorCode, AstNode node,
+      [List<Object> arguments]) {
+    printErrorForOffset(errorCode, node.offset, node.length, arguments);
+  }
+
+  void printErrorForOffset(ErrorCode errorCode, int offset, int length,
+      [List<Object> arguments]) {
+    print(new AnalysisError(_source, offset, length, errorCode, arguments));
+  }
+
   /**
    * Set the source to be used when reporting errors to the given [source].
    * Setting the source to `null` will cause the default source to be used.
diff --git a/pkg/analyzer/lib/src/generated/resolver.dart b/pkg/analyzer/lib/src/generated/resolver.dart
index 3f9d3d8..2c9c221 100644
--- a/pkg/analyzer/lib/src/generated/resolver.dart
+++ b/pkg/analyzer/lib/src/generated/resolver.dart
@@ -4792,7 +4792,11 @@ class InferenceContext {
       error = StrongModeCode.INFERRED_TYPE;
     }

-    _errorReporter.reportErrorForNode(error, node, [node, type]);
+    if (_errorReporter.isNullListener) {
+      _errorReporter.printErrorForNode(error, node, [node, type]);
+    } else {
+      _errorReporter.reportErrorForNode(error, node, [node, type]);
+    }
   }

   List<DartType> _matchTypes(InterfaceType t1, InterfaceType t2) {

then run either of these:

dart -c pkg/analyzer/test/src/task/strong/checker_test.dart
dart -c pkg/analyzer/test/src/task/strong/inferred_type_test.dart
@jmesserly

This comment has been minimized.

Copy link
Contributor

commented Aug 10, 2016

ah here's a simple one:

var x = <String>[].map((x) => "");

top-level/static field initializers might be the culprit.

@jmesserly

This comment has been minimized.

Copy link
Contributor

commented Aug 10, 2016

ah yes, from InferStaticVariableTypeTask:

      ResolutionContext resolutionContext = ResolutionContextBuilder.contextFor(
          initializer, AnalysisErrorListener.NULL_LISTENER);
      ResolverVisitor visitor = new ResolverVisitor(variable.library,
          variable.source, typeProvider, AnalysisErrorListener.NULL_LISTENER,
          nameScope: resolutionContext.scope);
@jmesserly

This comment has been minimized.

Copy link
Contributor

commented Aug 11, 2016

FYI, I filed that one as #27053

@jmesserly

This comment has been minimized.

Copy link
Contributor

commented Aug 27, 2016

Taking this up now as it's blocking some other things.

jmesserly added a commit that referenced this issue Aug 29, 2016
Simplify the bailout test for generic inference
Just tidies up two checks that should be one check, and some vestigial substitution. Noticed this while working on #26992

R=leafp@google.com

Review URL: https://codereview.chromium.org/2288393002 .

@jmesserly jmesserly closed this in f57ed4d Sep 1, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.