refactor/simplify nullable inference code

refactor/simplify nullable inference code

Some of the high level changes are:
* visit catch body, fixes #463
* handle temps created by the compiler. These were in some cases treated incorrectly as non-null (see tests diff)
* compute nullable in the same pass as visiting assignments
* simplify visiting += and ++, fix ++ (it was dead code, #463)
* simplify _isNullable
* if we fail to see a variable declaration, treat it as nullable
* stack trace in catch is treated as non-null

R=vsm@google.com

Committed: https://github.com/dart-lang/dev_compiler/commit/507d3c541ae56afe4a5ad0f2304b3002981bb228

[Jennifer Messerly, 2016-03-01 21:58:33]

Description was changed from

==========
refactor/simplify nullable inference code

Some of the high level changes are:
* visit catch body, fixes #463
* handle temps
* compute nullable in the same pass as visiting assignments
* simplify visiting += and ++, fix ++ (was dead code)
* simplify _isNullable
* defensive: if we fail to see a variable declaration, treat it as nullable
* StackTrace is treated as non-null
==========

to

==========
refactor/simplify nullable inference code

Some of the high level changes are:
* visit catch body, fixes #463
* handle temps created by the compiler
* compute nullable in the same pass as visiting assignments
* simplify visiting += and ++, fix ++ (it was dead code, #463)
* simplify _isNullable
* if we fail to see a variable declaration, treat it as nullable
* stack trace in catch is treated as non-null
==========

[Jennifer Messerly, 2016-03-01 22:04:15]

Description was changed from

==========
refactor/simplify nullable inference code

Some of the high level changes are:
* visit catch body, fixes #463
* handle temps created by the compiler
* compute nullable in the same pass as visiting assignments
* simplify visiting += and ++, fix ++ (it was dead code, #463)
* simplify _isNullable
* if we fail to see a variable declaration, treat it as nullable
* stack trace in catch is treated as non-null
==========

to

==========
refactor/simplify nullable inference code

Some of the high level changes are:
* visit catch body, fixes #463
* handle temps created by the compiler. These were in some cases treated
incorrectly as non-null
(https://codereview.chromium.org/download/issue1751963002_20001_30008.diff)
* compute nullable in the same pass as visiting assignments
* simplify visiting += and ++, fix ++ (it was dead code, #463)
* simplify _isNullable
* if we fail to see a variable declaration, treat it as nullable
* stack trace in catch is treated as non-null
==========

[Jennifer Messerly, 2016-03-01 22:04:26]

Description was changed from

==========
refactor/simplify nullable inference code

Some of the high level changes are:
* visit catch body, fixes #463
* handle temps created by the compiler. These were in some cases treated
incorrectly as non-null
(https://codereview.chromium.org/download/issue1751963002_20001_30008.diff)
* compute nullable in the same pass as visiting assignments
* simplify visiting += and ++, fix ++ (it was dead code, #463)
* simplify _isNullable
* if we fail to see a variable declaration, treat it as nullable
* stack trace in catch is treated as non-null
==========

to

==========
refactor/simplify nullable inference code

Some of the high level changes are:
* visit catch body, fixes #463
* handle temps created by the compiler. These were in some cases treated
incorrectly as non-null (see tests diff)
* compute nullable in the same pass as visiting assignments
* simplify visiting += and ++, fix ++ (it was dead code, #463)
* simplify _isNullable
* if we fail to see a variable declaration, treat it as nullable
* stack trace in catch is treated as non-null
==========

[Jennifer Messerly, 2016-03-01 22:05:40]

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

[Jennifer Messerly, 2016-03-01 22:05:41]

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

https://codereview.chromium.org/1751963002/diff/20001/lib/src/codegen/nullabl...
lib/src/codegen/nullable_type_inference.dart:48: bool _isNullable(Expression
expr,
the diff didn't pick this up very well. But essentially, the logic in this
method is unchanged other than simplification around the `e is
LocalVariableElement` path

https://codereview.chromium.org/1751963002/diff/20001/lib/src/codegen/nullabl...
lib/src/codegen/nullable_type_inference.dart:150: class _NullableLocalInference
extends RecursiveAstVisitor {
this visitor is mostly unchanged from the AssignmentsIndex code, but it calls
_isNullable on the Expression right away, instead of collecting Expressions and
visiting them later.

https://codereview.chromium.org/1751963002/diff/20001/lib/src/codegen/nullabl...
lib/src/codegen/nullable_type_inference.dart:191: _nullableLocals.add(element);
this avoids building a `null` AST, then immediately visiting it and concluding
that yes, `null` is in fact nullable :)

https://codereview.chromium.org/1751963002/diff/20001/lib/src/codegen/nullabl...
lib/src/codegen/nullable_type_inference.dart:222:
_visitAssignment(node.leftOperand, node);
We treat this as `a = (a += 1)` ... this will have the right nullability
conclusions without requiring us to construct an AST node for `a + 1`.

Same trick is used for pre/postfix.

https://codereview.chromium.org/1751963002/diff/20001/tool/build_sdk.sh
File tool/build_sdk.sh (right):

https://codereview.chromium.org/1751963002/diff/20001/tool/build_sdk.sh#newco...
tool/build_sdk.sh:13: dart bin/dartdevc.dart --force-compile --no-source-maps
--sdk-check \
reverted this in patch set 3

[ochafik, 2016-03-01 23:29:57]

Sweet, thanks John!

Can't see the language-all diff, could you add some fixed problematic cases to
test/codegen/notnull.dart?

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

https://codereview.chromium.org/1751963002/diff/40001/lib/src/codegen/nullabl...
lib/src/codegen/nullable_type_inference.dart:24: final Func1<DartType, bool>
_isPrimitiveType;
Oh, cool!

https://codereview.chromium.org/1751963002/diff/40001/lib/src/codegen/nullabl...
lib/src/codegen/nullable_type_inference.dart:27: HashSet<LocalVariableElement>
_notNullLocals;
nit: final + computed in a factory constructor?

https://codereview.chromium.org/1751963002/diff/40001/lib/src/codegen/nullabl...
lib/src/codegen/nullable_type_inference.dart:153: /// Known local variables.
that field changes semantics after computeNotNullLocals is called, I'd reflect
this in the comment

[Jennifer Messerly, 2016-03-01 23:37:48]

On 2016/03/01 23:29:57, ochafik wrote:
> Sweet, thanks John!
> 
> Can't see the language-all diff, could you add some fixed problematic cases to
> test/codegen/notnull.dart?
> 
>
https://codereview.chromium.org/1751963002/diff/40001/lib/src/codegen/nullabl...
> File lib/src/codegen/nullable_type_inference.dart (right):
> 
>
https://codereview.chromium.org/1751963002/diff/40001/lib/src/codegen/nullabl...
> lib/src/codegen/nullable_type_inference.dart:24: final Func1<DartType, bool>
> _isPrimitiveType;
> Oh, cool!
> 
>
https://codereview.chromium.org/1751963002/diff/40001/lib/src/codegen/nullabl...
> lib/src/codegen/nullable_type_inference.dart:27: HashSet<LocalVariableElement>
> _notNullLocals;
> nit: final + computed in a factory constructor?
> 
>
https://codereview.chromium.org/1751963002/diff/40001/lib/src/codegen/nullabl...
> lib/src/codegen/nullable_type_inference.dart:153: /// Known local variables.
> that field changes semantics after computeNotNullLocals is called, I'd reflect
> this in the comment

fyi -- if you click the "download" link next to language-all.js, you can see the
diff (in text form). Not sure why it doesn't show that automatically.

[Jennifer Messerly, 2016-03-01 23:37:56]

On 2016/03/01 23:37:48, John Messerly wrote:
> On 2016/03/01 23:29:57, ochafik wrote:
> > Sweet, thanks John!
> > 
> > Can't see the language-all diff, could you add some fixed problematic cases
to
> > test/codegen/notnull.dart?
> > 
> >
>
https://codereview.chromium.org/1751963002/diff/40001/lib/src/codegen/nullabl...
> > File lib/src/codegen/nullable_type_inference.dart (right):
> > 
> >
>
https://codereview.chromium.org/1751963002/diff/40001/lib/src/codegen/nullabl...
> > lib/src/codegen/nullable_type_inference.dart:24: final Func1<DartType, bool>
> > _isPrimitiveType;
> > Oh, cool!
> > 
> >
>
https://codereview.chromium.org/1751963002/diff/40001/lib/src/codegen/nullabl...
> > lib/src/codegen/nullable_type_inference.dart:27:
HashSet<LocalVariableElement>
> > _notNullLocals;
> > nit: final + computed in a factory constructor?
> > 
> >
>
https://codereview.chromium.org/1751963002/diff/40001/lib/src/codegen/nullabl...
> > lib/src/codegen/nullable_type_inference.dart:153: /// Known local variables.
> > that field changes semantics after computeNotNullLocals is called, I'd
reflect
> > this in the comment
> 
> fyi -- if you click the "download" link next to language-all.js, you can see
the
> diff (in text form). Not sure why it doesn't show that automatically.

e.g. https://codereview.chromium.org/download/issue1751963002_40001_50008.diff

[ochafik, 2016-03-01 23:44:26]

Cool, thanks!

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

https://codereview.chromium.org/1751963002/diff/40001/lib/src/codegen/nullabl...
lib/src/codegen/nullable_type_inference.dart:259: if
(_nullInference._isNullable(right, visitLocal)) {
btw, what happens now when right is AssignmentExpression, isn't it going to
treat it as nullable?
(would be nice to cover these cases from #463 in test/codegen/notnull.dart)

[vsm, 2016-03-01 23:57:23]

lgtm

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

https://codereview.chromium.org/1751963002/diff/20001/lib/src/codegen/nullabl...
lib/src/codegen/nullable_type_inference.dart:79: return _isNullable(expr.target,
localIsNullable);
Could take advantage of the fact this:

x..foo()

must be non-null if it doesn't throw.  i.e., dart.notNull(x..foo()) is
unnecessary.  Doubt it's a common pattern though.

https://codereview.chromium.org/1751963002/diff/20001/lib/src/codegen/nullabl...
lib/src/codegen/nullable_type_inference.dart:159: /// Given a variable, tracks
all other variables that is is assigned to.
s/is is/it is/

[Jennifer Messerly, 2016-03-02 00:42:23]

Thanks guys!

I've got a few small tweaks that will make a bunch more Exprs infer as not null,
but I'll send that as a follow up.

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

https://codereview.chromium.org/1751963002/diff/20001/lib/src/codegen/nullabl...
lib/src/codegen/nullable_type_inference.dart:79: return _isNullable(expr.target,
localIsNullable);
On 2016/03/01 23:57:23, vsm wrote:
> Could take advantage of the fact this:
> 
> x..foo()
> 
> must be non-null if it doesn't throw.  i.e., dart.notNull(x..foo()) is
> unnecessary.  Doubt it's a common pattern though.

Done.

https://codereview.chromium.org/1751963002/diff/20001/lib/src/codegen/nullabl...
lib/src/codegen/nullable_type_inference.dart:159: /// Given a variable, tracks
all other variables that is is assigned to.
On 2016/03/01 23:57:23, vsm wrote:
> s/is is/it is/

Done.

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

https://codereview.chromium.org/1751963002/diff/40001/lib/src/codegen/nullabl...
lib/src/codegen/nullable_type_inference.dart:27: HashSet<LocalVariableElement>
_notNullLocals;
On 2016/03/01 23:29:56, ochafik wrote:
> nit: final + computed in a factory constructor?

The issue is that we pass `this` to the visitor (for accessing _isNullable), so
it's not possible to use a factory. But yeah it would be nice.

https://codereview.chromium.org/1751963002/diff/40001/lib/src/codegen/nullabl...
lib/src/codegen/nullable_type_inference.dart:153: /// Known local variables.
On 2016/03/01 23:29:56, ochafik wrote:
> that field changes semantics after computeNotNullLocals is called, I'd reflect
> this in the comment

Perhaps what I can do here is document that the Visitor itself is single use. In
other words: computeNotNullLocals must be called exactly once, and the visitor
discarded at that point, because all of the state is potentially invalid. Would
that help clarify it?

Another option is to make these non-final, and just null them out. Seemed
unnecessary as the whole visitor gets GC'd immediately.

(overall this is a fairly common pattern in compiler visitors, collect state,
compute something, discard. Reusing the visitor is erroneous & making it private
is a good idea in that case. Can't say I've ever seen a totally satisfying
pattern in OOP langs. Works a bit better in functional languages.)

https://codereview.chromium.org/1751963002/diff/40001/lib/src/codegen/nullabl...
lib/src/codegen/nullable_type_inference.dart:259: if
(_nullInference._isNullable(right, visitLocal)) {
On 2016/03/01 23:44:26, ochafik wrote:
> btw, what happens now when right is AssignmentExpression, isn't it going to
> treat it as nullable?
> (would be nice to cover these cases from #463 in test/codegen/notnull.dart)

Yeah, I didn't get to that part. Was satisfied to see that AssignmentExpr is not
handled by _isNullable, meaning we don't need to worry about the visit-twice
problems I was worried about. But we can certainly improve _isNullable, either
by adding cases over time, or perhaps going through all Expressions and thinking
systematically about what is and is not null. The latter might be better to do
as part of implementing nullable/non-nullable types in Analyzer. (and ultimately
a lot of this analysis is probably better in Analyzer.)

[Jennifer Messerly, 2016-03-02 00:44:09]

Description was changed from

==========
refactor/simplify nullable inference code

Some of the high level changes are:
* visit catch body, fixes #463
* handle temps created by the compiler. These were in some cases treated
incorrectly as non-null (see tests diff)
* compute nullable in the same pass as visiting assignments
* simplify visiting += and ++, fix ++ (it was dead code, #463)
* simplify _isNullable
* if we fail to see a variable declaration, treat it as nullable
* stack trace in catch is treated as non-null
==========

to

==========
refactor/simplify nullable inference code

Some of the high level changes are:
* visit catch body, fixes #463
* handle temps created by the compiler. These were in some cases treated
incorrectly as non-null (see tests diff)
* compute nullable in the same pass as visiting assignments
* simplify visiting += and ++, fix ++ (it was dead code, #463)
* simplify _isNullable
* if we fail to see a variable declaration, treat it as nullable
* stack trace in catch is treated as non-null

R=vsm@google.com

Committed:
https://github.com/dart-lang/dev_compiler/commit/507d3c541ae56afe4a5ad0f2304b...
==========

[Jennifer Messerly, 2016-03-02 00:44:11]

Committed patchset #4 (id:60001) manually as
507d3c541ae56afe4a5ad0f2304b3002981bb228 (presubmit successful).

[ochafik, 2016-03-02 02:01:15]

lgtm

[vsm, 2016-03-02 14:20:33]

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

https://codereview.chromium.org/1751963002/diff/60001/lib/src/codegen/nullabl...
lib/src/codegen/nullable_type_inference.dart:78: if (expr is CascadeExpression)
return false;
This would need to check for members supported by null.  E.g., x..toString() is
nullable.

[Jennifer Messerly, 2016-03-02 17:10:54]

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

https://codereview.chromium.org/1751963002/diff/60001/lib/src/codegen/nullabl...
lib/src/codegen/nullable_type_inference.dart:78: if (expr is CascadeExpression)
return false;
On 2016/03/02 14:20:33, vsm wrote:
> This would need to check for members supported by null.  E.g., x..toString()
is
> nullable.

Ah right. I just took your suggestion directly, didn't really think about it.
I'll add a note to my other nullable CL. :)