Issue 932. Checks for various named arguments cases.

Issue 932. Checks for various named arguments cases.
http://code.google.com/p/dart/issues/detail?id=932

1. Clean up List<? extends Type>;

2. Generate compile time warnings/errors.

3. Generate run time errors.

4. Fix corresponding tests according to spec.


R=zundel@google.com
BUG=
TEST=

Committed: https://code.google.com/p/dart/source/detail?r=3511

[zundel, 2012-01-23 16:18:21]

https://chromiumcodereview.appspot.com/9270016/diff/1/compiler/java/com/googl...
File
compiler/java/com/google/dart/compiler/backend/js/GenerateJavascriptAST.java
(right):

https://chromiumcodereview.appspot.com/9270016/diff/1/compiler/java/com/googl...
compiler/java/com/google/dart/compiler/backend/js/GenerateJavascriptAST.java:2845:
// May be specified named arguments without corresponding named parameters.
I assume what this means:

// Caller specified a named argument that wasn't declared in the method
definition

https://chromiumcodereview.appspot.com/9270016/diff/1/compiler/java/com/googl...
File compiler/java/com/google/dart/compiler/resolver/TopLevelElementBuilder.java
(right):

https://chromiumcodereview.appspot.com/9270016/diff/1/compiler/java/com/googl...
compiler/java/com/google/dart/compiler/resolver/TopLevelElementBuilder.java:160:
Collections.<Type>unmodifiableList(typeVariables)));
Is the unmodifiable list just for safety or did you find a problem this way? 
Maybe Elements.makeTypeVariables should return an immutable list?

https://chromiumcodereview.appspot.com/9270016/diff/1/compiler/java/com/googl...
File compiler/java/com/google/dart/compiler/type/TypeAnalyzer.java (right):

https://chromiumcodereview.appspot.com/9270016/diff/1/compiler/java/com/googl...
compiler/java/com/google/dart/compiler/type/TypeAnalyzer.java:547:
Iterator<Entry<String, Type>> named = namedParameterTypes.entrySet().iterator();
I think this declaration should go outside your inner block, then you can use
the while loop instead of declaring a new block.  Or, if you want to protect
'named' from being used outside the loop, use a for loop here.

https://chromiumcodereview.appspot.com/9270016/diff/1/compiler/java/com/googl...
compiler/java/com/google/dart/compiler/type/TypeAnalyzer.java:584: }
What would happen with this kind of invocation:

foo(int i, [int j, int k]) {}

int main () {
  foo(1, j:2, 3);
}

I think this should be a compile-time error, but currently the code compiles
without errors and fails at runtime.  In theory, we could catch this at the
parser phase.  Currently,  I think it would print out that '3' is an extra
argument, but maybe we need to say that passing a positional after a named
argument is invalid.

https://chromiumcodereview.appspot.com/9270016/diff/1/tests/language/language...
File tests/language/language.status (right):

https://chromiumcodereview.appspot.com/9270016/diff/1/tests/language/language...
tests/language/language.status:132: #NamedParameters10NegativeTest: Fail  #
Implementation in progress.
go ahead and remove these lines before committing.

https://chromiumcodereview.appspot.com/9270016/diff/1/tests/language/src/Name...
File tests/language/src/NamedParameters10NegativeTest.dart (right):

https://chromiumcodereview.appspot.com/9270016/diff/1/tests/language/src/Name...
tests/language/src/NamedParameters10NegativeTest.dart:16: test(10, x:99);  // 1
positional arg, as expected. Param x does not exist.
"Furthermore, each qi, 1 <= i <= l,  must be a member of the set {pm+1, …, pm+k}
or a run time error occurs. "

Too bad it doesn't say what kind of error...  dartc throws a
NoSuchMethodException

I don't think this test should cause the compile to abort, so maybe a better way
to test this is


bool foundError = false;
try {
  test(10, x:99);
} expect (Exception e) {
  foundError = true;
}

Expect.equals(true, foundError);

https://chromiumcodereview.appspot.com/9270016/diff/1/tests/language/src/Name...
File tests/language/src/NamedParameters2NegativeTest.dart (right):

https://chromiumcodereview.appspot.com/9270016/diff/1/tests/language/src/Name...
tests/language/src/NamedParameters2NegativeTest.dart:15: F31(10, 25, b:25);  //
Parameter b passed twice, as positional and named.
same comment as in NamedParameters10NegativeTest. we expect the code to compile,
but to throw a some sort of error at runtime.

https://chromiumcodereview.appspot.com/9270016/diff/1/tests/language/src/Name...
tests/language/src/NamedParameters2NegativeTest.dart:19: main() {
you can get rid of the NamedParameters2NegativeTest class and move the
invocation into main() if you like.

https://chromiumcodereview.appspot.com/9270016/diff/1/tests/language/src/Name...
File tests/language/src/NamedParameters3NegativeTest.dart (right):

https://chromiumcodereview.appspot.com/9270016/diff/1/tests/language/src/Name...
tests/language/src/NamedParameters3NegativeTest.dart:20:
NamedParameters3NegativeTest.testMain();
You can get rid of the class NamedParameters3NegativeTest if you like and just
move the F31() invocation into main()

[scheglov, 2012-01-23 19:32:27]

https://chromiumcodereview.appspot.com/9270016/diff/1/compiler/java/com/googl...
File
compiler/java/com/google/dart/compiler/backend/js/GenerateJavascriptAST.java
(right):

https://chromiumcodereview.appspot.com/9270016/diff/1/compiler/java/com/googl...
compiler/java/com/google/dart/compiler/backend/js/GenerateJavascriptAST.java:2845:
// May be specified named arguments without corresponding named parameters.
On 2012/01/23 16:18:21, zundel wrote:
> I assume what this means:
> 
> // Caller specified a named argument that wasn't declared in the method
> definition

Done.

https://chromiumcodereview.appspot.com/9270016/diff/1/compiler/java/com/googl...
File compiler/java/com/google/dart/compiler/resolver/TopLevelElementBuilder.java
(right):

https://chromiumcodereview.appspot.com/9270016/diff/1/compiler/java/com/googl...
compiler/java/com/google/dart/compiler/resolver/TopLevelElementBuilder.java:160:
Collections.<Type>unmodifiableList(typeVariables)));
On 2012/01/23 16:18:21, zundel wrote:
> Is the unmodifiable list just for safety or did you find a problem this way? 
> Maybe Elements.makeTypeVariables should return an immutable list? 

Basically this allows us to pass List<TypeVariable>, i.e. List<? extends Type>,
into method which accepts List<Type>, because we can guarantee that no one will
add other subclass of Type, incompatible with TypeVariable.

https://chromiumcodereview.appspot.com/9270016/diff/1/compiler/java/com/googl...
File compiler/java/com/google/dart/compiler/type/TypeAnalyzer.java (right):

https://chromiumcodereview.appspot.com/9270016/diff/1/compiler/java/com/googl...
compiler/java/com/google/dart/compiler/type/TypeAnalyzer.java:547:
Iterator<Entry<String, Type>> named = namedParameterTypes.entrySet().iterator();
On 2012/01/23 16:18:21, zundel wrote:
> I think this declaration should go outside your inner block, then you can use
> the while loop instead of declaring a new block.  Or, if you want to protect
> 'named' from being used outside the loop, use a for loop here.

Done.

https://chromiumcodereview.appspot.com/9270016/diff/1/compiler/java/com/googl...
compiler/java/com/google/dart/compiler/type/TypeAnalyzer.java:584: }
On 2012/01/23 16:18:21, zundel wrote:
> What would happen with this kind of invocation:
> 
> foo(int i, [int j, int k]) {}
> 
> int main () {
>   foo(1, j:2, 3);
> }
> 
> I think this should be a compile-time error, but currently the code compiles
> without errors and fails at runtime.  In theory, we could catch this at the
> parser phase.  Currently,  I think it would print out that '3' is an extra
> argument, but maybe we need to say that passing a positional after a named
> argument is invalid.
> 

Done.

https://chromiumcodereview.appspot.com/9270016/diff/1/tests/language/language...
File tests/language/language.status (right):

https://chromiumcodereview.appspot.com/9270016/diff/1/tests/language/language...
tests/language/language.status:132: #NamedParameters10NegativeTest: Fail  #
Implementation in progress.
On 2012/01/23 16:18:21, zundel wrote:
> go ahead and remove these lines before committing.

Done.

https://chromiumcodereview.appspot.com/9270016/diff/1/tests/language/src/Name...
File tests/language/src/NamedParameters10NegativeTest.dart (right):

https://chromiumcodereview.appspot.com/9270016/diff/1/tests/language/src/Name...
tests/language/src/NamedParameters10NegativeTest.dart:16: test(10, x:99);  // 1
positional arg, as expected. Param x does not exist.
On 2012/01/23 16:18:21, zundel wrote:
> "Furthermore, each qi, 1 <= i <= l,  must be a member of the set {pm+1, …,
pm+k}
> or a run time error occurs. "
> 
> Too bad it doesn't say what kind of error...  dartc throws a
> NoSuchMethodException
> 
> I don't think this test should cause the compile to abort, so maybe a better
way
> to test this is
> 
> 
> bool foundError = false;
> try {
>   test(10, x:99);
> } expect (Exception e) {
>   foundError = true;
> }
> 
> Expect.equals(true, foundError);
> 

Done.

https://chromiumcodereview.appspot.com/9270016/diff/1/tests/language/src/Name...
File tests/language/src/NamedParameters2NegativeTest.dart (right):

https://chromiumcodereview.appspot.com/9270016/diff/1/tests/language/src/Name...
tests/language/src/NamedParameters2NegativeTest.dart:15: F31(10, 25, b:25);  //
Parameter b passed twice, as positional and named.
On 2012/01/23 16:18:21, zundel wrote:
> same comment as in NamedParameters10NegativeTest. we expect the code to
compile,
> but to throw a some sort of error at runtime.

Done.

https://chromiumcodereview.appspot.com/9270016/diff/1/tests/language/src/Name...
tests/language/src/NamedParameters2NegativeTest.dart:19: main() {
On 2012/01/23 16:18:21, zundel wrote:
> you can get rid of the NamedParameters2NegativeTest class and move the
> invocation into main() if you like. 

Done.

https://chromiumcodereview.appspot.com/9270016/diff/1/tests/language/src/Name...
File tests/language/src/NamedParameters3NegativeTest.dart (right):

https://chromiumcodereview.appspot.com/9270016/diff/1/tests/language/src/Name...
tests/language/src/NamedParameters3NegativeTest.dart:20:
NamedParameters3NegativeTest.testMain();
On 2012/01/23 16:18:21, zundel wrote:
> You can get rid of the class NamedParameters3NegativeTest if you like and just
> move the F31() invocation into main()

Done.

[zundel, 2012-01-23 19:39:19]

lgtm

https://chromiumcodereview.appspot.com/9270016/diff/75/tests/language/src/Nam...
File tests/language/src/NamedParameters3Test.dart (right):

https://chromiumcodereview.appspot.com/9270016/diff/75/tests/language/src/Nam...
tests/language/src/NamedParameters3Test.dart:18: Expect.equals(true,
foundError);
I think that NamedParameters10NegativeTest is slightly different, in that the
method being called had only one positional parameter and no named parameters. 
Could you just fold that into this test to make sure we don't lose coverage for
that corner case that NamedParameters10NegativeTest was added for?

[zundel, 2012-01-23 19:42:29]

Oh, I forgot, we need to update the year of the copyright notice on files that
are modified.

[scheglov, 2012-01-23 20:00:44]

I've updated year in changed source and test files.

https://chromiumcodereview.appspot.com/9270016/diff/75/tests/language/src/Nam...
File tests/language/src/NamedParameters3Test.dart (right):

https://chromiumcodereview.appspot.com/9270016/diff/75/tests/language/src/Nam...
tests/language/src/NamedParameters3Test.dart:18: Expect.equals(true,
foundError);
On 2012/01/23 19:39:19, zundel wrote:
> I think that NamedParameters10NegativeTest is slightly different, in that the
> method being called had only one positional parameter and no named parameters.

> Could you just fold that into this test to make sure we don't lose coverage
for
> that corner case that NamedParameters10NegativeTest was added for?

Actually NamedParameters10NegativeTest was replaced by NamedParameters4Test, not
by this test.