Implement type checking of map literals (issue 221).

Implement type checking of map literals (issue 221).
Support proper syntax for map literals (at most one type argument is allowed)
and give a warning when legacy syntax is used.
Add tests.
Fix tests.
Committed: https://code.google.com/p/dart/source/detail?r=1764

[regis, 2011-11-22 17:38:32]

I will update the various test status files before submitting, so that dartc,
frog, and friends remain happy.
List literals (issue 220) will be taken care of in another cl.

Thanks,
Regis

[hausner, 2011-11-22 18:37:34]

LGTM with a couple of Qs.

http://codereview.chromium.org/8637018/diff/10001/runtime/lib/literal_map_fac...
File runtime/lib/literal_map_factory.dart (right):

http://codereview.chromium.org/8637018/diff/10001/runtime/lib/literal_map_fac...
runtime/lib/literal_map_factory.dart:12: var map = new LinkedHashMap<K, V>();
Why don't you declare the map as <String, V> ?

http://codereview.chromium.org/8637018/diff/10001/runtime/lib/literal_map_fac...
runtime/lib/literal_map_factory.dart:16: if (elements[i] is !V) {
Why do you have to explicitly implement the type check? Would the assignment in
line 22 below not throw a type error if elements[i] isn't of type V? (because
map is declared to be of type <K, V>)

http://codereview.chromium.org/8637018/diff/10001/tests/language/src/MapLiter...
File tests/language/src/MapLiteral1NegativeTest.dart (right):

http://codereview.chromium.org/8637018/diff/10001/tests/language/src/MapLiter...
tests/language/src/MapLiteral1NegativeTest.dart:8: class
MapLiteral1NegativeTest<T> {
Why does this class need a type parameter?

[regis, 2011-11-22 20:15:19]

Thanks

http://codereview.chromium.org/8637018/diff/10001/runtime/lib/literal_map_fac...
File runtime/lib/literal_map_factory.dart (right):

http://codereview.chromium.org/8637018/diff/10001/runtime/lib/literal_map_fac...
runtime/lib/literal_map_factory.dart:12: var map = new LinkedHashMap<K, V>();
On 2011/11/22 18:37:35, hausner wrote:
> Why don't you declare the map as <String, V> ?

Done.

http://codereview.chromium.org/8637018/diff/10001/runtime/lib/literal_map_fac...
runtime/lib/literal_map_factory.dart:16: if (elements[i] is !V) {
On 2011/11/22 18:37:35, hausner wrote:
> Why do you have to explicitly implement the type check? Would the assignment
in
> line 22 below not throw a type error if elements[i] isn't of type V? (because
> map is declared to be of type <K, V>)

Yes, we would get a type error here, but the user would not know how to deal
with it. So I intercept the error and report it with a more useful error
message.

http://codereview.chromium.org/8637018/diff/10001/tests/language/src/MapLiter...
File tests/language/src/MapLiteral1NegativeTest.dart (right):

http://codereview.chromium.org/8637018/diff/10001/tests/language/src/MapLiter...
tests/language/src/MapLiteral1NegativeTest.dart:8: class
MapLiteral1NegativeTest<T> {
On 2011/11/22 18:37:35, hausner wrote:
> Why does this class need a type parameter?

It does not. Copy and paste error. Thanks. Done.

[srdjan, 2011-11-23 03:43:42]

DBC

http://codereview.chromium.org/8637018/diff/14001/runtime/lib/literal_map_fac...
File runtime/lib/literal_map_factory.dart (right):

http://codereview.chromium.org/8637018/diff/14001/runtime/lib/literal_map_fac...
runtime/lib/literal_map_factory.dart:22: map[elements[i-1]] = elements[i];
space between -> 'i - 1'

http://codereview.chromium.org/8637018/diff/14001/runtime/vm/parser.cc
File runtime/vm/parser.cc (right):

http://codereview.chromium.org/8637018/diff/14001/runtime/vm/parser.cc#newcode29
runtime/vm/parser.cc:29: DECLARE_FLAG(bool, expose_core_impl);
Future CL: Since we are using this flag only for de-privatizing coreimpl
classes, maybe we could rename it to expose_private_core_impl.

[hausner, 2011-11-23 15:40:10]

Additional Q.

http://codereview.chromium.org/8637018/diff/10001/runtime/lib/literal_map_fac...
File runtime/lib/literal_map_factory.dart (right):

http://codereview.chromium.org/8637018/diff/10001/runtime/lib/literal_map_fac...
runtime/lib/literal_map_factory.dart:16: if (elements[i] is !V) {
At the cost of duplicated type checks?  Why not declare map without type
arguments then to avoid the second (redundant) check? The user cannot add
anything to that map later, since it is immutable, so I'm not sure how the the
type arguments would ever be helpful.
 
On 2011/11/22 20:15:20, regis wrote:
> On 2011/11/22 18:37:35, hausner wrote:
> > Why do you have to explicitly implement the type check? Would the assignment
> in
> > line 22 below not throw a type error if elements[i] isn't of type V?
(because
> > map is declared to be of type <K, V>)
> 
> Yes, we would get a type error here, but the user would not know how to deal
> with it. So I intercept the error and report it with a more useful error
> message.

[regis, 2011-11-23 15:51:32]

Thanks guys.
I will address this feedback in the currently pending cl fixing the list
literals.

http://codereview.chromium.org/8637018/diff/10001/runtime/lib/literal_map_fac...
File runtime/lib/literal_map_factory.dart (right):

http://codereview.chromium.org/8637018/diff/10001/runtime/lib/literal_map_fac...
runtime/lib/literal_map_factory.dart:16: if (elements[i] is !V) {
On 2011/11/23 15:40:10, hausner wrote:
> At the cost of duplicated type checks?  Why not declare map without type
> arguments then to avoid the second (redundant) check? The user cannot add
> anything to that map later, since it is immutable, so I'm not sure how the the
> type arguments would ever be helpful.
>  
> On 2011/11/22 20:15:20, regis wrote:
> > On 2011/11/22 18:37:35, hausner wrote:
> > > Why do you have to explicitly implement the type check? Would the
assignment
> > in
> > > line 22 below not throw a type error if elements[i] isn't of type V?
> (because
> > > map is declared to be of type <K, V>)
> > 
> > Yes, we would get a type error here, but the user would not know how to deal
> > with it. So I intercept the error and report it with a more useful error
> > message.
> 

Actually, this code has changed now. See the other pending cl fixing issue 220.
This was wrong, as the test was done in non checked mode as well.

http://codereview.chromium.org/8637018/diff/14001/runtime/lib/literal_map_fac...
File runtime/lib/literal_map_factory.dart (right):

http://codereview.chromium.org/8637018/diff/14001/runtime/lib/literal_map_fac...
runtime/lib/literal_map_factory.dart:22: map[elements[i-1]] = elements[i];
On 2011/11/23 03:43:44, srdjan wrote:
> space between -> 'i - 1'

Will do.

http://codereview.chromium.org/8637018/diff/14001/runtime/vm/parser.cc
File runtime/vm/parser.cc (right):

http://codereview.chromium.org/8637018/diff/14001/runtime/vm/parser.cc#newcode29
runtime/vm/parser.cc:29: DECLARE_FLAG(bool, expose_core_impl);
On 2011/11/23 03:43:44, srdjan wrote:
> Future CL: Since we are using this flag only for de-privatizing coreimpl
> classes, maybe we could rename it to expose_private_core_impl.

Will do.