Add a few more known non-null cases

Add a few more known non-null cases

Improves handling of identifiers and known elements, the `identical` function, and generative constructors.

R=ochafik@google.com, vsm@google.com

Committed: https://github.com/dart-lang/dev_compiler/commit/82ebf76c4eca61bce4697ba2b7d2fb0132a0fdf6

[Jennifer Messerly, 2016-03-02 01:10:32]

Description was changed from

==========
Fix a few more nullable cases
==========

to

==========
Fix a few more nullable cases

Improves handling of identifiers and known elements, the `identical` function,
and generative constructors.
==========

[Jennifer Messerly, 2016-03-02 01:10:32]

jmesserly@google.com changed reviewers:
+ ochafik@google.com, vsm@google.com

[Jennifer Messerly, 2016-03-02 01:10:53]

Description was changed from

==========
Fix a few more nullable cases

Improves handling of identifiers and known elements, the `identical` function,
and generative constructors.
==========

to

==========
Add a few more known non-null cases

Improves handling of identifiers and known elements, the `identical` function,
and generative constructors.
==========

[ochafik, 2016-03-02 02:04:53]

Thanks a lot John, lgtm!

https://codereview.chromium.org/1752193002/diff/1/lib/src/codegen/nullable_ty...
File lib/src/codegen/nullable_type_inference.dart (right):

https://codereview.chromium.org/1752193002/diff/1/lib/src/codegen/nullable_ty...
lib/src/codegen/nullable_type_inference.dart:110: if
(e.library.source.isInSystemLibrary) return false;
Good point, sweet!

https://codereview.chromium.org/1752193002/diff/1/lib/src/codegen/nullable_ty...
lib/src/codegen/nullable_type_inference.dart:160: e.name == 'identical' &&
nit: e?.name == 'identical' to save the null check

[Jennifer Messerly, 2016-03-02 17:11:52]

https://codereview.chromium.org/1752193002/diff/1/lib/src/codegen/nullable_ty...
File lib/src/codegen/nullable_type_inference.dart (right):

https://codereview.chromium.org/1752193002/diff/1/lib/src/codegen/nullable_ty...
lib/src/codegen/nullable_type_inference.dart:87: if (expr is CascadeExpression)
return false;
Vijay pointed out in another CL that this is broken: we should check if it's an
Object member (like toString).

[Jennifer Messerly, 2016-03-02 20:50:57]

Thanks Olivier!
 
@Vijay -- I fixed the issue from the previous CL about cascades, if you want to
take a look at that?

[vsm, 2016-03-02 21:06:54]

https://codereview.chromium.org/1752193002/diff/20001/lib/src/codegen/nullabl...
File lib/src/codegen/nullable_type_inference.dart (right):

https://codereview.chromium.org/1752193002/diff/20001/lib/src/codegen/nullabl...
lib/src/codegen/nullable_type_inference.dart:99: if (e == null ||
isObjectProperty(e.name)) {
Shouldn't this be all instead of any?  E.g., if *all* are object properties,
return true.  If any are not, return false.

E.g.,
e..toString()..foo()

should return false.

[Jennifer Messerly, 2016-03-02 21:16:30]

https://codereview.chromium.org/1752193002/diff/20001/lib/src/codegen/nullabl...
File lib/src/codegen/nullable_type_inference.dart (right):

https://codereview.chromium.org/1752193002/diff/20001/lib/src/codegen/nullabl...
lib/src/codegen/nullable_type_inference.dart:99: if (e == null ||
isObjectProperty(e.name)) {
On 2016/03/02 21:06:54, vsm wrote:
> Shouldn't this be all instead of any?  E.g., if *all* are object properties,
> return true.  If any are not, return false.
> 
> E.g.,
> e..toString()..foo()
> 
> should return false.

Yeah that was a simplification, because I couldn't think of a valid reason to do
..toString(), ..hashCode, or ..runtimeType in a cascade, as none of them have
side effects.

So bailing on the first one is still technically correct. ;-)

If we want to be super precise even for methods on Object, then we should also
check if the target was already known to be non-null too.

But to be honest... I'm tempted to just revert to the old code:
https://codereview.chromium.org/1751963002/diff/20001/lib/src/codegen/nullabl...

I'm not convinced that any of this cascade support is worth it... seems a rare
code path to hit.

Let me know what you think is best.

[vsm, 2016-03-02 21:20:26]

On 2016/03/02 21:16:30, John Messerly wrote:
>
https://codereview.chromium.org/1752193002/diff/20001/lib/src/codegen/nullabl...
> File lib/src/codegen/nullable_type_inference.dart (right):
> 
>
https://codereview.chromium.org/1752193002/diff/20001/lib/src/codegen/nullabl...
> lib/src/codegen/nullable_type_inference.dart:99: if (e == null ||
> isObjectProperty(e.name)) {
> On 2016/03/02 21:06:54, vsm wrote:
> > Shouldn't this be all instead of any?  E.g., if *all* are object properties,
> > return true.  If any are not, return false.
> > 
> > E.g.,
> > e..toString()..foo()
> > 
> > should return false.
> 
> Yeah that was a simplification, because I couldn't think of a valid reason to
do
> ..toString(), ..hashCode, or ..runtimeType in a cascade, as none of them have
> side effects.
> 
> So bailing on the first one is still technically correct. ;-)
> 
> If we want to be super precise even for methods on Object, then we should also
> check if the target was already known to be non-null too.
> 
> But to be honest... I'm tempted to just revert to the old code:
>
https://codereview.chromium.org/1751963002/diff/20001/lib/src/codegen/nullabl...
> 
> I'm not convinced that any of this cascade support is worth it... seems a rare
> code path to hit.
> 
> Let me know what you think is best.

lgtm

Yeah, any or all of those are fine - up to you.  I doubt this case will ever
come up.

[Jennifer Messerly, 2016-03-02 21:27:08]

https://codereview.chromium.org/1752193002/diff/20001/lib/src/codegen/nullabl...
File lib/src/codegen/nullable_type_inference.dart (right):

https://codereview.chromium.org/1752193002/diff/20001/lib/src/codegen/nullabl...
lib/src/codegen/nullable_type_inference.dart:99: if (e == null ||
isObjectProperty(e.name)) {
On 2016/03/02 21:16:30, John Messerly wrote:
> On 2016/03/02 21:06:54, vsm wrote:
> > Shouldn't this be all instead of any?  E.g., if *all* are object properties,
> > return true.  If any are not, return false.
> > 
> > E.g.,
> > e..toString()..foo()
> > 
> > should return false.
> 
> Yeah that was a simplification, because I couldn't think of a valid reason to
do
> ..toString(), ..hashCode, or ..runtimeType in a cascade, as none of them have
> side effects.
> 
> So bailing on the first one is still technically correct. ;-)
> 
> If we want to be super precise even for methods on Object, then we should also
> check if the target was already known to be non-null too.
> 
> But to be honest... I'm tempted to just revert to the old code:
>
https://codereview.chromium.org/1751963002/diff/20001/lib/src/codegen/nullabl...
> 
> I'm not convinced that any of this cascade support is worth it... seems a rare
> code path to hit.
> 
> Let me know what you think is best.

Thanks. I went ahead and did the "precise" version. Already this far down the
rabbit hole, might as well. :)

[Jennifer Messerly, 2016-03-02 21:32:31]

Description was changed from

==========
Add a few more known non-null cases

Improves handling of identifiers and known elements, the `identical` function,
and generative constructors.
==========

to

==========
Add a few more known non-null cases

Improves handling of identifiers and known elements, the `identical` function,
and generative constructors.

R=ochafik@google.com, vsm@google.com

Committed:
https://github.com/dart-lang/dev_compiler/commit/82ebf76c4eca61bce4697ba2b7d2...
==========

[Jennifer Messerly, 2016-03-02 21:32:32]

Committed patchset #3 (id:40001) manually as
82ebf76c4eca61bce4697ba2b7d2fb0132a0fdf6 (presubmit successful).