Typecheck map and list literals

Typecheck map and list literals

We were not checking the elements of list and map literals against the declared type of the literal.

BUG=
R=vsm@google.com

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

[Leaf, 2015-02-25 20:42:56]

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

[vsm, 2015-02-25 20:52:29]

lgtm

https://codereview.chromium.org/957013002/diff/20001/lib/src/checker/checker....
File lib/src/checker/checker.dart (right):

https://codereview.chromium.org/957013002/diff/20001/lib/src/checker/checker....
lib/src/checker/checker.dart:390: var elements = node.elements;
You could lift this block into the nested if above.  It's a no-op unless type is
not dynamic, right?

https://codereview.chromium.org/957013002/diff/20001/lib/src/checker/checker....
lib/src/checker/checker.dart:392: elements[i] = checkArgument(elements[i],
type);
Are you already keeping track of whether we're in a const setting?  If so, we
might want to think whether to generate a DownCast when we are.  It'll be
invalid Dart output - but ok in JS though.

https://codereview.chromium.org/957013002/diff/20001/lib/src/checker/checker....
lib/src/checker/checker.dart:405: var entries = node.entries;
ditto

[Leaf, 2015-02-25 21:07:04]

https://codereview.chromium.org/957013002/diff/20001/lib/src/checker/checker....
File lib/src/checker/checker.dart (right):

https://codereview.chromium.org/957013002/diff/20001/lib/src/checker/checker....
lib/src/checker/checker.dart:390: var elements = node.elements;
On 2015/02/25 20:52:28, vsm wrote:
> You could lift this block into the nested if above.  It's a no-op unless type
is
> not dynamic, right?

Yeah, had it that way, then changed it just on the general principle that not
checking is kind of a latent bug if anybody later on adds non-trivial checks to
checkArgument when the type is dynamic.  I might be overthinking it though... 
:)

https://codereview.chromium.org/957013002/diff/20001/lib/src/checker/checker....
lib/src/checker/checker.dart:392: elements[i] = checkArgument(elements[i],
type);
On 2015/02/25 20:52:29, vsm wrote:
> Are you already keeping track of whether we're in a const setting?  If so, we
> might want to think whether to generate a DownCast when we are.  It'll be
> invalid Dart output - but ok in JS though.

I'm not checking for const currently.  I considered rejecting downcasts in const
context here, but wanted to leave us open to generating them in the JS backend. 
Could do it, but controlled by a flag.  Hmmm...

[vsm, 2015-02-25 21:14:08]

On 2015/02/25 21:07:04, leafp wrote:
>
https://codereview.chromium.org/957013002/diff/20001/lib/src/checker/checker....
> File lib/src/checker/checker.dart (right):
> 
>
https://codereview.chromium.org/957013002/diff/20001/lib/src/checker/checker....
> lib/src/checker/checker.dart:390: var elements = node.elements;
> On 2015/02/25 20:52:28, vsm wrote:
> > You could lift this block into the nested if above.  It's a no-op unless
type
> is
> > not dynamic, right?
> 
> Yeah, had it that way, then changed it just on the general principle that not
> checking is kind of a latent bug if anybody later on adds non-trivial checks
to
> checkArgument when the type is dynamic.  I might be overthinking it though... 
> :)

That's fine with me.  :-)

> 
>
https://codereview.chromium.org/957013002/diff/20001/lib/src/checker/checker....
> lib/src/checker/checker.dart:392: elements[i] = checkArgument(elements[i],
> type);
> On 2015/02/25 20:52:29, vsm wrote:
> > Are you already keeping track of whether we're in a const setting?  If so,
we
> > might want to think whether to generate a DownCast when we are.  It'll be
> > invalid Dart output - but ok in JS though.
> 
> I'm not checking for const currently.  I considered rejecting downcasts in
const
> context here, but wanted to leave us open to generating them in the JS
backend. 
> Could do it, but controlled by a flag.  Hmmm...

Yeah, that might be nice for experiments on existing code.

still lgtm

[Leaf, 2015-02-26 23:40:58]

Added a flag to allow rejecting casts in a constant context, with test.  PTAL.

[vsm, 2015-02-27 00:20:16]

lgtm

https://codereview.chromium.org/957013002/diff/40001/lib/src/checker/checker....
File lib/src/checker/checker.dart (right):

https://codereview.chromium.org/957013002/diff/40001/lib/src/checker/checker....
lib/src/checker/checker.dart:350: _visitMaybeConst(AstNode n, visitNode(AstNode
n)) {
Would be nice if we had generic methods.  :-)

https://codereview.chromium.org/957013002/diff/40001/lib/src/checker/checker....
lib/src/checker/checker.dart:405: _visitMaybeConst(node,
_visitConstructorDeclaration);
Perhaps just inline the closure here instead of a separate function?  Similar
below?

[Leaf, 2015-02-27 00:44:21]

https://codereview.chromium.org/957013002/diff/40001/lib/src/checker/checker....
File lib/src/checker/checker.dart (right):

https://codereview.chromium.org/957013002/diff/40001/lib/src/checker/checker....
lib/src/checker/checker.dart:350: _visitMaybeConst(AstNode n, visitNode(AstNode
n)) {
On 2015/02/27 00:20:16, vsm wrote:
> Would be nice if we had generic methods.  :-)

Acknowledged.

https://codereview.chromium.org/957013002/diff/40001/lib/src/checker/checker....
lib/src/checker/checker.dart:405: _visitMaybeConst(node,
_visitConstructorDeclaration);
On 2015/02/27 00:20:16, vsm wrote:
> Perhaps just inline the closure here instead of a separate function?  Similar
> below?

Done.

[Leaf, 2015-02-27 00:45:19]

Committed patchset #5 (id:80001) manually as
adcc118251d76ebbd00485f68f6e376e0f001ead (presubmit successful).