Issue 28027. Move Null to the bottom in the Analyzer.

Issue 28027. Move Null to the bottom in the Analyzer.

R=brianwilkerson@google.com, paulberry@google.com
BUG= https://github.com/dart-lang/sdk/issues/28027

Committed: https://github.com/dart-lang/sdk/commit/0160332713fb955916deac2b216f2e872f3b479b

[Brian Wilkerson, 2017-01-17 21:53:01]

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

https://codereview.chromium.org/2638183002/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/error_verifier.dart:5492:
staticReturnType.isBottom ||
I don't understand when we need to check for bottom (such as here) and when we
don't (such as line 6430).

[scheglov, 2017-01-17 21:54:18]

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

https://codereview.chromium.org/2638183002/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/error_verifier.dart:5492:
staticReturnType.isBottom ||
On 2017/01/17 21:53:01, Brian Wilkerson wrote:
> I don't understand when we need to check for bottom (such as here) and when we
> don't (such as line 6430).

This covers:
void foo() => throw 42;

[Brian Wilkerson, 2017-01-17 21:59:29]

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

https://codereview.chromium.org/2638183002/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/error_verifier.dart:5492:
staticReturnType.isBottom ||
Is it true, then, that bottom can only appear because of a throw expression?

[scheglov, 2017-01-17 22:07:23]

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

https://codereview.chromium.org/2638183002/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/error_verifier.dart:5492:
staticReturnType.isBottom ||
On 2017/01/17 21:59:29, Brian Wilkerson wrote:
> Is it true, then, that bottom can only appear because of a throw expression?

As I understand the latest specification - yes.

[Paul Berry, 2017-01-17 22:32:09]

https://codereview.chromium.org/2638183002/diff/1/tests/language/null_bottom_...
File tests/language/null_bottom_test.dart (right):

https://codereview.chromium.org/2638183002/diff/1/tests/language/null_bottom_...
tests/language/null_bottom_test.dart:116: fun2(null).abs();
Am I reading correctly that this no longer gives a static warning nor a runtime
error?  No longer giving a static warning seems undesirable--was this intended? 
No longer giving a runtime error just seems wrong.

[Brian Wilkerson, 2017-01-17 22:41:24]

In the places where my expectations are wrong (we can't actually see or don't
need to explicitly handle 'bottom'), a comment in the code explaining why would
be helpful later on.

Other than that, lgtm.

https://codereview.chromium.org/2638183002/diff/1/pkg/analysis_server/lib/src...
File pkg/analysis_server/lib/src/services/correction/assist_internal.dart
(right):

https://codereview.chromium.org/2638183002/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/services/correction/assist_internal.dart:339: if
(type == null || type.isDynamic || type.isDartCoreNull) {
What about:

int x = throw '';

Seems like the initializer can have a static type of 'bottom'.

https://codereview.chromium.org/2638183002/diff/1/pkg/analysis_server/lib/src...
File pkg/analysis_server/lib/src/services/correction/fix_internal.dart (right):

https://codereview.chromium.org/2638183002/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/services/correction/fix_internal.dart:2406: if (type
== null || type.isDartCoreNull) {
What about:

f(throw '');

Seems like an argument could have a type of 'bottom'.

https://codereview.chromium.org/2638183002/diff/1/pkg/analysis_server/lib/src...
File pkg/analysis_server/lib/src/services/correction/util.dart (right):

https://codereview.chromium.org/2638183002/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/services/correction/util.dart:1133: if
(type.isDartCoreNull) {
Are we sure 'type' cannot be 'bottom'?

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

https://codereview.chromium.org/2638183002/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/resolver.dart:5335: if (potentialType == null ||
potentialType.isDartCoreNull) {
Are we sure 'potentialType' cannot be 'bottom'?

https://codereview.chromium.org/2638183002/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/resolver.dart:5393: if (strongMode || type ==
null || type.isDynamic || type.isDartCoreNull) {
Are we sure 'type' cannot be 'bottom'?

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

https://codereview.chromium.org/2638183002/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/static_type_analyzer.dart:984: //
TODO(jmesserly): in strong mode, should we just use the context type?
I'm guessing this comment can be removed.

https://codereview.chromium.org/2638183002/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/static_type_analyzer.dart:2113:
(!resolutionMap.staticTypeForExpression(initializer).isDartCoreNull)) {
I think the initializer could have a static type of 'bottom', and that we ought
to not infer 'bottom'.

https://codereview.chromium.org/2638183002/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/static_type_analyzer.dart:2272: if (!_strongMode
&& type != null && !type.isDynamic && !type.isDartCoreNull) {
Seems like we should guard against propagating 'bottom'.

https://codereview.chromium.org/2638183002/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/static_type_analyzer.dart:2297: if
(propagatedReturnType.isDartCoreNull) {
Seems like we should guard against propagating 'bottom'.

https://codereview.chromium.org/2638183002/diff/1/pkg/analyzer/lib/src/summar...
File pkg/analyzer/lib/src/summary/link.dart (right):

https://codereview.chromium.org/2638183002/diff/1/pkg/analyzer/lib/src/summar...
pkg/analyzer/lib/src/summary/link.dart:250: if (type == null ||
type.isDartCoreNull || type.isVoid) {
Seems like we should still guard against 'bottom'.

https://codereview.chromium.org/2638183002/diff/1/pkg/analyzer/lib/src/task/d...
File pkg/analyzer/lib/src/task/dart.dart (right):

https://codereview.chromium.org/2638183002/diff/1/pkg/analyzer/lib/src/task/d...
pkg/analyzer/lib/src/task/dart.dart:3748: if (newType == null ||
newType.isDartCoreNull) {
Seems like we should still replace 'bottom' with dynamic.

https://codereview.chromium.org/2638183002/diff/1/pkg/analyzer/lib/src/task/s...
File pkg/analyzer/lib/src/task/strong_mode.dart (right):

https://codereview.chromium.org/2638183002/diff/1/pkg/analyzer/lib/src/task/s...
pkg/analyzer/lib/src/task/strong_mode.dart:371: if (newType == null ||
newType.isDartCoreNull) {
Seems like we should still replace 'bottom' with dynamic.

[Leaf, 2017-01-18 00:48:21]

leafp@google.com changed reviewers:
+ leafp@google.com

[Leaf, 2017-01-18 00:48:22]

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

https://codereview.chromium.org/2638183002/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/error_verifier.dart:5492:
staticReturnType.isBottom ||
On 2017/01/17 22:07:23, scheglov wrote:
> On 2017/01/17 21:59:29, Brian Wilkerson wrote:
> > Is it true, then, that bottom can only appear because of a throw expression?
> 
> As I understand the latest specification - yes.

This seems likely true in spec mode Dart.  In Strong Mode, we may wish to allow
inference to fill in bottom.

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

https://codereview.chromium.org/2638183002/diff/1/pkg/analyzer/test/generated...
pkg/analyzer/test/generated/static_type_warning_code_test.dart:1928:
[StaticTypeWarningCode.UNDEFINED_METHOD]);
Now I'm confused - this seems different from the language test case.  Does this
implementation allow all methods to be called on Null or not?

https://codereview.chromium.org/2638183002/diff/1/tests/language/null_bottom_...
File tests/language/null_bottom_test.dart (right):

https://codereview.chromium.org/2638183002/diff/1/tests/language/null_bottom_...
tests/language/null_bottom_test.dart:116: fun2(null).abs();
On 2017/01/17 22:32:09, Paul Berry wrote:
> Am I reading correctly that this no longer gives a static warning nor a
runtime
> error?  No longer giving a static warning seems undesirable--was this
intended? 
> No longer giving a runtime error just seems wrong.

The lack of runtime error seems wrong to me as well, unless I'm misunderstanding
what that means.

The static warning question is interesting.  I agree that the natural extension
of the current Dart treatment of null/bottom is to not warn on this, but I think
that's likely to be a bad user experience.  Bob points out the ubiquity of
Future<Null> as a way of saying "don't use the result of this Future".  I will
open a language issue to discuss whether we should change the spec on this
point.

[scheglov, 2017-01-18 02:10:51]

PTAL

https://codereview.chromium.org/2638183002/diff/1/pkg/analysis_server/lib/src...
File pkg/analysis_server/lib/src/services/correction/assist_internal.dart
(right):

https://codereview.chromium.org/2638183002/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/services/correction/assist_internal.dart:339: if
(type == null || type.isDynamic || type.isDartCoreNull) {
On 2017/01/17 22:41:24, Brian Wilkerson wrote:
> What about:
> 
> int x = throw '';
> 
> Seems like the initializer can have a static type of 'bottom'.

Right.
But later we check that only InterfaceType and FunctionType are allowed.
And Bottom is neither.
But Null is InterfaceType.
I changed the main condition, it would be easier to understand.

https://codereview.chromium.org/2638183002/diff/1/pkg/analysis_server/lib/src...
File pkg/analysis_server/lib/src/services/correction/fix_internal.dart (right):

https://codereview.chromium.org/2638183002/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/services/correction/fix_internal.dart:2406: if (type
== null || type.isDartCoreNull) {
On 2017/01/17 22:41:24, Brian Wilkerson wrote:
> What about:
> 
> f(throw '');
> 
> Seems like an argument could have a type of 'bottom'.

Done.

https://codereview.chromium.org/2638183002/diff/1/pkg/analysis_server/lib/src...
File pkg/analysis_server/lib/src/services/correction/util.dart (right):

https://codereview.chromium.org/2638183002/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/services/correction/util.dart:1133: if
(type.isDartCoreNull) {
On 2017/01/17 22:41:24, Brian Wilkerson wrote:
> Are we sure 'type' cannot be 'bottom'?

I think yes, but we could keep the check for isBottom too.

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

https://codereview.chromium.org/2638183002/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/resolver.dart:5335: if (potentialType == null ||
potentialType.isDartCoreNull) {
On 2017/01/17 22:41:24, Brian Wilkerson wrote:
> Are we sure 'potentialType' cannot be 'bottom'?

Given that only ThrowExpression has 'bottom' type, it is possible, but
impractical.
I've added a new test case.
f() {
  var v = 1;
  v = throw 2;
  return v;
}

https://codereview.chromium.org/2638183002/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/resolver.dart:5393: if (strongMode || type ==
null || type.isDynamic || type.isDartCoreNull) {
On 2017/01/17 22:41:24, Brian Wilkerson wrote:
> Are we sure 'type' cannot be 'bottom'?

I cannot think an example for this.

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

https://codereview.chromium.org/2638183002/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/static_type_analyzer.dart:984: //
TODO(jmesserly): in strong mode, should we just use the context type?
On 2017/01/17 22:41:24, Brian Wilkerson wrote:
> I'm guessing this comment can be removed.

Done.

https://codereview.chromium.org/2638183002/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/static_type_analyzer.dart:2113:
(!resolutionMap.staticTypeForExpression(initializer).isDartCoreNull)) {
On 2017/01/17 22:41:24, Brian Wilkerson wrote:
> I think the initializer could have a static type of 'bottom', and that we
ought
> to not infer 'bottom'.

Acknowledged.
I've added a new test case.

https://codereview.chromium.org/2638183002/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/static_type_analyzer.dart:2272: if (!_strongMode
&& type != null && !type.isDynamic && !type.isDartCoreNull) {
On 2017/01/17 22:41:24, Brian Wilkerson wrote:
> Seems like we should guard against propagating 'bottom'.

Done.

https://codereview.chromium.org/2638183002/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/static_type_analyzer.dart:2297: if
(propagatedReturnType.isDartCoreNull) {
On 2017/01/17 22:41:24, Brian Wilkerson wrote:
> Seems like we should guard against propagating 'bottom'.

Done.

https://codereview.chromium.org/2638183002/diff/1/pkg/analyzer/lib/src/summar...
File pkg/analyzer/lib/src/summary/link.dart (right):

https://codereview.chromium.org/2638183002/diff/1/pkg/analyzer/lib/src/summar...
pkg/analyzer/lib/src/summary/link.dart:250: if (type == null ||
type.isDartCoreNull || type.isVoid) {
On 2017/01/17 22:41:24, Brian Wilkerson wrote:
> Seems like we should still guard against 'bottom'.

Done.

https://codereview.chromium.org/2638183002/diff/1/pkg/analyzer/lib/src/task/d...
File pkg/analyzer/lib/src/task/dart.dart (right):

https://codereview.chromium.org/2638183002/diff/1/pkg/analyzer/lib/src/task/d...
pkg/analyzer/lib/src/task/dart.dart:3748: if (newType == null ||
newType.isDartCoreNull) {
On 2017/01/17 22:41:24, Brian Wilkerson wrote:
> Seems like we should still replace 'bottom' with dynamic.

Yes!
Again with throw.
Sigh...
Fixed.

https://codereview.chromium.org/2638183002/diff/1/pkg/analyzer/lib/src/task/s...
File pkg/analyzer/lib/src/task/strong_mode.dart (right):

https://codereview.chromium.org/2638183002/diff/1/pkg/analyzer/lib/src/task/s...
pkg/analyzer/lib/src/task/strong_mode.dart:371: if (newType == null ||
newType.isDartCoreNull) {
On 2017/01/17 22:41:24, Brian Wilkerson wrote:
> Seems like we should still replace 'bottom' with dynamic.

Done.

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

https://codereview.chromium.org/2638183002/diff/1/pkg/analyzer/test/generated...
pkg/analyzer/test/generated/static_type_warning_code_test.dart:1928:
[StaticTypeWarningCode.UNDEFINED_METHOD]);
On 2017/01/18 00:48:22, Leaf wrote:
> Now I'm confused - this seems different from the language test case.  Does
this
> implementation allow all methods to be called on Null or not?

I added TODO with the new issue reference.

https://codereview.chromium.org/2638183002/diff/1/tests/language/null_bottom_...
File tests/language/null_bottom_test.dart (right):

https://codereview.chromium.org/2638183002/diff/1/tests/language/null_bottom_...
tests/language/null_bottom_test.dart:116: fun2(null).abs();
On 2017/01/17 22:32:09, Paul Berry wrote:
> Am I reading correctly that this no longer gives a static warning nor a
runtime
> error?  No longer giving a static warning seems undesirable--was this
intended? 
> No longer giving a runtime error just seems wrong.

See my message to Lasse about language/null_bottom_test/03 and not generating a
static warning.
You're CCed IIRC.

You're right about the runtime error.
Restored.

https://codereview.chromium.org/2638183002/diff/1/tests/language/null_bottom_...
tests/language/null_bottom_test.dart:116: fun2(null).abs();
On 2017/01/18 00:48:22, Leaf wrote:
> On 2017/01/17 22:32:09, Paul Berry wrote:
> > Am I reading correctly that this no longer gives a static warning nor a
> runtime
> > error?  No longer giving a static warning seems undesirable--was this
> intended? 
> > No longer giving a runtime error just seems wrong.
> 
> The lack of runtime error seems wrong to me as well, unless I'm
misunderstanding
> what that means.

  Agree, fixed.

> 
> The static warning question is interesting.  I agree that the natural
extension
> of the current Dart treatment of null/bottom is to not warn on this, but I
think
> that's likely to be a bad user experience.  Bob points out the ubiquity of
> Future<Null> as a way of saying "don't use the result of this Future".  I will
> open a language issue to discuss whether we should change the spec on this
> point.

  I added your to the messages I sent Lasse earlier today.
  It's a bit different situation here than the one your opened a new issue
about.

[Brian Wilkerson, 2017-01-18 14:51:10]

lgtm

[scheglov, 2017-01-18 19:15:17]

Description was changed from

==========
Issue 28027. Move Null to the bottom in the Analyzer.

R=brianwilkerson@google.com, paulberry@google.com
BUG= https://github.com/dart-lang/sdk/issues/28027
==========

to

==========
Issue 28027. Move Null to the bottom in the Analyzer.

R=brianwilkerson@google.com, paulberry@google.com
BUG= https://github.com/dart-lang/sdk/issues/28027

Committed:
https://github.com/dart-lang/sdk/commit/0160332713fb955916deac2b216f2e872f3b479b
==========

[scheglov, 2017-01-18 19:15:18]

Committed patchset #2 (id:20001) manually as
0160332713fb955916deac2b216f2e872f3b479b (presubmit successful).