MalformedType used for all invalid type annotations.

MalformedType used for all invalid type annotations.

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

[ngeoffray, 2012-11-30 12:00:39]

LGTM!

https://codereview.chromium.org/11412245/diff/3001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/compile_time_constants.dart
(right):

https://codereview.chromium.org/11412245/diff/3001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/compile_time_constants.dart:133: &&
element.isField()) {
Maybe try to have this method call potentiallyCheckType, and pass to that method
the 'isConst' flag?

https://codereview.chromium.org/11412245/diff/3001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/lib/js_helper.dart (right):

https://codereview.chromium.org/11412245/diff/3001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:964:
throwMalformedSubtypeError(value, type, reasons) {
Why this helper and not inlining it line 1511?

https://codereview.chromium.org/11412245/diff/3001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/resolution/members.dart (right):

https://codereview.chromium.org/11412245/diff/3001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/resolution/members.dart:1140: node,
const Link(), enclosingElement,
Link<of what> ?

https://codereview.chromium.org/11412245/diff/3001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/ssa/bailout.dart (right):

https://codereview.chromium.org/11412245/diff/3001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/bailout.dart:117:
!sourceType.isMalformed &&
I wouldn't bother here. Let it have type guards. The program is bogus anyway.

https://codereview.chromium.org/11412245/diff/3001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/ssa/builder.dart (right):

https://codereview.chromium.org/11412245/diff/3001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:3474: return false;
// Visit the for malformed type only.
for -> first

Why the first only? for a type annotation it looks from codegen.dart that you're
looking for all malformed ones. Shouldn't you make it consistent?

https://codereview.chromium.org/11412245/diff/3001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/typechecker.dart (right):

https://codereview.chromium.org/11412245/diff/3001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/typechecker.dart:276: * [declaredType]
is [: Map<String> :].
The comment is making it more confusing :(

https://codereview.chromium.org/11412245/diff/3001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/typechecker.dart:292:
[this.typeArguments = null]);
fits in one line?

https://codereview.chromium.org/11412245/diff/3001/tests/language/type_variab...
File tests/language/type_variable_scope2_test.dart (right):

https://codereview.chromium.org/11412245/diff/3001/tests/language/type_variab...
tests/language/type_variable_scope2_test.dart:29: print(error);
Remove?

https://codereview.chromium.org/11412245/diff/3001/tests/language/wrong_numbe...
File tests/language/wrong_number_type_arguments_test.dart (right):

https://codereview.chromium.org/11412245/diff/3001/tests/language/wrong_numbe...
tests/language/wrong_number_type_arguments_test.dart:30: print(error);
Remove print?

[ahe, 2012-11-30 15:44:06]

Comments so far.  Overall, it really looks good.

https://codereview.chromium.org/11412245/diff/12001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/compiler.dart (right):

https://codereview.chromium.org/11412245/diff/12001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/compiler.dart:157:
reportDiagnostic(span, ex.message, api.Diagnostic.INFO);
Let's find another way to solve this.

https://codereview.chromium.org/11412245/diff/12001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/lib/js_helper.dart (right):

https://codereview.chromium.org/11412245/diff/12001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:964:
throwMalformedSubtypeError(value, type, reasons) {
Add a doc comment explaining who calls this.

https://codereview.chromium.org/11412245/diff/12001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/ssa/builder.dart (right):

https://codereview.chromium.org/11412245/diff/12001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:3496: return false;
// Visit the for malformed type only.
I'm not sure I understand this comment.

https://codereview.chromium.org/11412245/diff/12001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/ssa/codegen.dart (right):

https://codereview.chromium.org/11412245/diff/12001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/ssa/codegen.dart:2465: const
SourceString("malformedTypeCheck")
How about adding malformedTypeCast which gives a slightly different error
message.

https://codereview.chromium.org/11412245/diff/12001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/ssa/codegen.dart:2508: if
(parameterCount == 2) {
Could you document what is going on here?

https://codereview.chromium.org/11412245/diff/12001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/ssa/codegen.dart:2516:
arguments.add(new js.LiteralString("'$reasons)}'"));
How about using writeJsonEscapedCharsOn from util.dart?

https://codereview.chromium.org/11412245/diff/12001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/ssa/ssa.dart (right):

https://codereview.chromium.org/11412245/diff/12001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/ssa/ssa.dart:39: String
fetchReasonsFromMalformedType(DartType type) {
For consistency, please move this function to another file.

Also, it really isn't related to SSA, so perhaps it belongs in a different
library?

https://codereview.chromium.org/11412245/diff/12001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/ssa/ssa.dart:47: return
Strings.join(reasons, ', ');
I'm not sure it is a good idea to concatenate strings that are likely to be
translated.

https://codereview.chromium.org/11412245/diff/12001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/typechecker.dart (right):

https://codereview.chromium.org/11412245/diff/12001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/typechecker.dart:273: * [declaredType]
holds the type which declared this malformed type.
... holds the type which the user wrote in code.

https://codereview.chromium.org/11412245/diff/12001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/typechecker.dart:278: final DartType
declaredType;
Rename to userProvidedBadType?

https://codereview.chromium.org/11412245/diff/12001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/typechecker.dart:292:
[this.typeArguments = null]);
Need to be optional?

https://codereview.chromium.org/11412245/diff/12001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/typechecker.dart:362: bool get
isMalformed {
I'm worried about performance of this method.

https://codereview.chromium.org/11412245/diff/12001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/typechecker.dart:446: bool get
isMalformed {
Ditto.

https://codereview.chromium.org/11412245/diff/12001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/typechecker.dart:543: bool get
isMalformed {
Ditto.

https://codereview.chromium.org/11412245/diff/12001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/typechecker.dart:625: t.isMalformed ||
Ditto.

https://codereview.chromium.org/11412245/diff/12001/tests/language/type_annot...
File tests/language/type_annotation_test.dart (right):

https://codereview.chromium.org/11412245/diff/12001/tests/language/type_annot...
tests/language/type_annotation_test.dart:11: void<T> voidWithTypeArguments() {}
/// 01: compile-time error
void
<T> /// 01: compile-time error
voidWithTypeArguments() {}

And repeat below :-)

https://codereview.chromium.org/11412245/diff/12001/tests/language/type_annot...
tests/language/type_annotation_test.dart:43: c.voidWithTypeArguments(); /// 01:
continued
May be able to remove "continued" tag if you follow the style above.

[ahe, 2012-12-03 12:32:01]

Everything but resolution/members.dart reviewed and good.

https://codereview.chromium.org/11412245/diff/12001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/universe/partial_type_tree.dart
(right):

https://codereview.chromium.org/11412245/diff/12001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/universe/partial_type_tree.dart:65: *
Returns the [ClassElement] for the most specific supertype of the receiver
I don't understand this comment.  As far as I can tell, this method returns a
class element whose type upper bound on the receiver type.  Instead of using
"dynamic", it uses Object.

https://codereview.chromium.org/11412245/diff/12001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/universe/partial_type_tree.dart:71: if
(type == null) return compiler.objectClass;
What if type is dynamic?

https://codereview.chromium.org/11412245/diff/12001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/warnings.dart (right):

https://codereview.chromium.org/11412245/diff/12001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/warnings.dart:209: "incorrect number
of type arguments on #{1}");
Eventually, I'd like to split this into two messages:

"extra type argument" and "too few type arguments".

Consider Foo<T>.  If Foo takes two arguments, the position would be the NodeList
<T>.  If Foo takes no arguments, the position would be T.

https://codereview.chromium.org/11412245/diff/12001/tests/language/wrong_numb...
File tests/language/wrong_number_type_arguments_test.dart (right):

https://codereview.chromium.org/11412245/diff/12001/tests/language/wrong_numb...
tests/language/wrong_number_type_arguments_test.dart:6: Map<String> foo;  ///
00: static type warning
This should be fixed to be specific to <String>.

https://codereview.chromium.org/11412245/diff/12001/tests/language/wrong_numb...
tests/language/wrong_number_type_arguments_test.dart:7: Map<String> baz;  ///
02: static type warning
Ditto.

[ahe, 2012-12-03 12:45:31]

LGTM!

https://codereview.chromium.org/11412245/diff/12001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/resolution/members.dart (right):

https://codereview.chromium.org/11412245/diff/12001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/resolution/members.dart:1064: //
TODO(johnniwinther): Remove this case.
Absolutely :-)

https://codereview.chromium.org/11412245/diff/12001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/resolution/members.dart:1069: return
null;
Should we create a synthetic element in this case?

https://codereview.chromium.org/11412245/diff/12001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/resolution/members.dart:1243: bool
typeArgumentCountMismatch = false;
You could rename this to something like hasTypeArgumentMismatch.  That sounds
more like a bool than the current name which sounds like a count.

[Johnni Winther, 2012-12-04 10:07:17]

https://codereview.chromium.org/11412245/diff/3001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/compile_time_constants.dart
(right):

https://codereview.chromium.org/11412245/diff/3001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/compile_time_constants.dart:133: &&
element.isField()) {
On 2012/11/30 12:00:40, ngeoffray wrote:
> Maybe try to have this method call potentiallyCheckType, and pass to that
method
> the 'isConst' flag?

I found no clean way to refactor.

https://codereview.chromium.org/11412245/diff/3001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/lib/js_helper.dart (right):

https://codereview.chromium.org/11412245/diff/3001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:964:
throwMalformedSubtypeError(value, type, reasons) {
On 2012/11/30 12:00:40, ngeoffray wrote:
> Why this helper and not inlining it line 1511?

The builder will generate calls for it.

https://codereview.chromium.org/11412245/diff/3001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/resolution/members.dart (right):

https://codereview.chromium.org/11412245/diff/3001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/resolution/members.dart:1140: node,
const Link(), enclosingElement,
On 2012/11/30 12:00:40, ngeoffray wrote:
> Link<of what> ?

Done.

https://codereview.chromium.org/11412245/diff/3001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/ssa/bailout.dart (right):

https://codereview.chromium.org/11412245/diff/3001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/bailout.dart:117:
!sourceType.isMalformed &&
On 2012/11/30 12:00:40, ngeoffray wrote:
> I wouldn't bother here. Let it have type guards. The program is bogus anyway.

Done.

https://codereview.chromium.org/11412245/diff/3001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/ssa/builder.dart (right):

https://codereview.chromium.org/11412245/diff/3001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:3474: return false;
// Visit the for malformed type only.
On 2012/11/30 12:00:40, ngeoffray wrote:
> for -> first
> 
> Why the first only? for a type annotation it looks from codegen.dart that
you're
> looking for all malformed ones. Shouldn't you make it consistent?

Changed to use the newly added fetchReasonsFromMalformedType method.

https://codereview.chromium.org/11412245/diff/3001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/typechecker.dart (right):

https://codereview.chromium.org/11412245/diff/3001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/typechecker.dart:276: * [declaredType]
is [: Map<String> :].
On 2012/11/30 12:00:40, ngeoffray wrote:
> The comment is making it more confusing :(

Rewritten

https://codereview.chromium.org/11412245/diff/3001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/typechecker.dart:292:
[this.typeArguments = null]);
On 2012/11/30 12:00:40, ngeoffray wrote:
> fits in one line?

Done.

https://codereview.chromium.org/11412245/diff/3001/tests/language/type_variab...
File tests/language/type_variable_scope2_test.dart (right):

https://codereview.chromium.org/11412245/diff/3001/tests/language/type_variab...
tests/language/type_variable_scope2_test.dart:29: print(error);
On 2012/11/30 12:00:40, ngeoffray wrote:
> Remove?

I left it in because it's nice to have when the test fails.

https://codereview.chromium.org/11412245/diff/3001/tests/language/wrong_numbe...
File tests/language/wrong_number_type_arguments_test.dart (right):

https://codereview.chromium.org/11412245/diff/3001/tests/language/wrong_numbe...
tests/language/wrong_number_type_arguments_test.dart:30: print(error);
On 2012/11/30 12:00:40, ngeoffray wrote:
> Remove print?

Again, nice to have when the test fails.

https://codereview.chromium.org/11412245/diff/12001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/compiler.dart (right):

https://codereview.chromium.org/11412245/diff/12001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/compiler.dart:157:
reportDiagnostic(span, ex.message, api.Diagnostic.INFO);
On 2012/11/30 15:44:07, ahe wrote:
> Let's find another way to solve this.

Done.

https://codereview.chromium.org/11412245/diff/12001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/lib/js_helper.dart (right):

https://codereview.chromium.org/11412245/diff/12001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:964:
throwMalformedSubtypeError(value, type, reasons) {
On 2012/11/30 15:44:07, ahe wrote:
> Add a doc comment explaining who calls this.

Done.

https://codereview.chromium.org/11412245/diff/12001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/resolution/members.dart (right):

https://codereview.chromium.org/11412245/diff/12001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/resolution/members.dart:1069: return
null;
On 2012/12/03 12:45:31, ahe wrote:
> Should we create a synthetic element in this case?

The caller creates the ErroneousElement for the MalformedType.
Comment added.

https://codereview.chromium.org/11412245/diff/12001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/resolution/members.dart:1243: bool
typeArgumentCountMismatch = false;
On 2012/12/03 12:45:31, ahe wrote:
> You could rename this to something like hasTypeArgumentMismatch.  That sounds
> more like a bool than the current name which sounds like a count.

Done.

https://codereview.chromium.org/11412245/diff/12001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/ssa/builder.dart (right):

https://codereview.chromium.org/11412245/diff/12001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:3496: return false;
// Visit the for malformed type only.
On 2012/11/30 15:44:07, ahe wrote:
> I'm not sure I understand this comment.

Changed to include all errors.

https://codereview.chromium.org/11412245/diff/12001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/ssa/codegen.dart (right):

https://codereview.chromium.org/11412245/diff/12001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/ssa/codegen.dart:2465: const
SourceString("malformedTypeCheck")
On 2012/11/30 15:44:07, ahe wrote:
> How about adding malformedTypeCast which gives a slightly different error
> message.

Added a TODO.

https://codereview.chromium.org/11412245/diff/12001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/ssa/codegen.dart:2508: if
(parameterCount == 2) {
On 2012/11/30 15:44:07, ahe wrote:
> Could you document what is going on here?

Done.

https://codereview.chromium.org/11412245/diff/12001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/ssa/codegen.dart:2516:
arguments.add(new js.LiteralString("'$reasons)}'"));
On 2012/11/30 15:44:07, ahe wrote:
> How about using writeJsonEscapedCharsOn from util.dart?

Added a TODO.

https://codereview.chromium.org/11412245/diff/12001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/ssa/ssa.dart (right):

https://codereview.chromium.org/11412245/diff/12001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/ssa/ssa.dart:39: String
fetchReasonsFromMalformedType(DartType type) {
On 2012/11/30 15:44:07, ahe wrote:
> For consistency, please move this function to another file.
> 
> Also, it really isn't related to SSA, so perhaps it belongs in a different
> library?

Moved to typechecker.dart

https://codereview.chromium.org/11412245/diff/12001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/ssa/ssa.dart:47: return
Strings.join(reasons, ', ');
On 2012/11/30 15:44:07, ahe wrote:
> I'm not sure it is a good idea to concatenate strings that are likely to be
> translated.

Added a TODO.

https://codereview.chromium.org/11412245/diff/12001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/typechecker.dart (right):

https://codereview.chromium.org/11412245/diff/12001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/typechecker.dart:273: * [declaredType]
holds the type which declared this malformed type.
On 2012/11/30 15:44:07, ahe wrote:
> ... holds the type which the user wrote in code.

Done.

https://codereview.chromium.org/11412245/diff/12001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/typechecker.dart:278: final DartType
declaredType;
On 2012/11/30 15:44:07, ahe wrote:
> Rename to userProvidedBadType?

Done.

https://codereview.chromium.org/11412245/diff/12001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/typechecker.dart:292:
[this.typeArguments = null]);
On 2012/11/30 15:44:07, ahe wrote:
> Need to be optional?

I like it to be. It feels strange having to pass an additional null to a
constructor that is complete.

https://codereview.chromium.org/11412245/diff/12001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/typechecker.dart:362: bool get
isMalformed {
On 2012/11/30 15:44:07, ahe wrote:
> I'm worried about performance of this method.

Changed to computed a creation time.

https://codereview.chromium.org/11412245/diff/12001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/typechecker.dart:446: bool get
isMalformed {
On 2012/11/30 15:44:07, ahe wrote:
> Ditto.

Changed to computed a creation time.

https://codereview.chromium.org/11412245/diff/12001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/typechecker.dart:543: bool get
isMalformed {
On 2012/11/30 15:44:07, ahe wrote:
> Ditto.

Changed to computed a creation time.

https://codereview.chromium.org/11412245/diff/12001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/typechecker.dart:625: t.isMalformed ||
On 2012/11/30 15:44:07, ahe wrote:
> Ditto.

Done.

https://codereview.chromium.org/11412245/diff/12001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/universe/partial_type_tree.dart
(right):

https://codereview.chromium.org/11412245/diff/12001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/universe/partial_type_tree.dart:65: *
Returns the [ClassElement] for the most specific supertype of the receiver
On 2012/12/03 12:32:01, ahe wrote:
> I don't understand this comment.  As far as I can tell, this method returns a
> class element whose type upper bound on the receiver type.  Instead of using
> "dynamic", it uses Object.

Done.

https://codereview.chromium.org/11412245/diff/12001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/universe/partial_type_tree.dart:71: if
(type == null) return compiler.objectClass;
On 2012/12/03 12:32:01, ahe wrote:
> What if type is dynamic?

Added a TODO.

https://codereview.chromium.org/11412245/diff/12001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/warnings.dart (right):

https://codereview.chromium.org/11412245/diff/12001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/warnings.dart:209: "incorrect number
of type arguments on #{1}");
On 2012/12/03 12:32:01, ahe wrote:
> Eventually, I'd like to split this into two messages:
> 
> "extra type argument" and "too few type arguments".
> 
> Consider Foo<T>.  If Foo takes two arguments, the position would be the
NodeList
> <T>.  If Foo takes no arguments, the position would be T.

Added a TODO.

https://codereview.chromium.org/11412245/diff/12001/tests/language/type_annot...
File tests/language/type_annotation_test.dart (right):

https://codereview.chromium.org/11412245/diff/12001/tests/language/type_annot...
tests/language/type_annotation_test.dart:11: void<T> voidWithTypeArguments() {}
/// 01: compile-time error
On 2012/11/30 15:44:07, ahe wrote:
> void
> <T> /// 01: compile-time error
> voidWithTypeArguments() {}
> 
> And repeat below :-)

Done.

https://codereview.chromium.org/11412245/diff/12001/tests/language/type_annot...
tests/language/type_annotation_test.dart:43: c.voidWithTypeArguments(); /// 01:
continued
On 2012/11/30 15:44:07, ahe wrote:
> May be able to remove "continued" tag if you follow the style above.

Done.

https://codereview.chromium.org/11412245/diff/12001/tests/language/wrong_numb...
File tests/language/wrong_number_type_arguments_test.dart (right):

https://codereview.chromium.org/11412245/diff/12001/tests/language/wrong_numb...
tests/language/wrong_number_type_arguments_test.dart:6: Map<String> foo;  ///
00: static type warning
On 2012/12/03 12:32:01, ahe wrote:
> This should be fixed to be specific to <String>.

Done.

https://codereview.chromium.org/11412245/diff/12001/tests/language/wrong_numb...
tests/language/wrong_number_type_arguments_test.dart:7: Map<String> baz;  ///
02: static type warning
On 2012/12/03 12:32:01, ahe wrote:
> Ditto.

Done.

[ngeoffray, 2012-12-05 15:43:46]

Still LGTM

https://codereview.chromium.org/11412245/diff/12030/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/ssa/codegen.dart (right):

https://codereview.chromium.org/11412245/diff/12030/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/ssa/codegen.dart:1616:
!identical(receiverType.kind, TypeKind.MALFORMED_TYPE)) {
Is that important? If it's malformed, it will say yes to any subtype check,
right? So having a TypedSelector should be fine.

https://codereview.chromium.org/11412245/diff/12030/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/ssa/codegen.dart:1740: if (type !=
null && !identical(type.kind, TypeKind.MALFORMED_TYPE)) {
ditto

https://codereview.chromium.org/11412245/diff/12030/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/ssa/codegen.dart:1750: if (type !=
null && !identical(type.kind, TypeKind.MALFORMED_TYPE)) {
ditto

https://codereview.chromium.org/11412245/diff/12030/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/typechecker.dart (right):

https://codereview.chromium.org/11412245/diff/12030/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/typechecker.dart:345: bool
hasMalformed(Link<DartType> types) {
Can that be a static or instance method in DartType?

[Johnni Winther, 2012-12-05 18:33:50]

https://codereview.chromium.org/11412245/diff/12030/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/ssa/codegen.dart (right):

https://codereview.chromium.org/11412245/diff/12030/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/ssa/codegen.dart:1616:
!identical(receiverType.kind, TypeKind.MALFORMED_TYPE)) {
On 2012/12/05 15:43:46, ngeoffray wrote:
> Is that important? If it's malformed, it will say yes to any subtype check,
> right? So having a TypedSelector should be fine.

It is to make a distinction between List<SomeMalformedType> and
SomeMalformedType. In the first case we actually know as much as if the type was
List<dynamic> and we should use that, whereas in the latter case we know
nothing.

https://codereview.chromium.org/11412245/diff/12030/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/typechecker.dart (right):

https://codereview.chromium.org/11412245/diff/12030/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/typechecker.dart:345: bool
hasMalformed(Link<DartType> types) {
On 2012/12/05 15:43:46, ngeoffray wrote:
> Can that be a static or instance method in DartType?

It could be a static method.

[ngeoffray, 2012-12-05 18:37:37]

https://codereview.chromium.org/11412245/diff/12030/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/ssa/codegen.dart (right):

https://codereview.chromium.org/11412245/diff/12030/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/ssa/codegen.dart:1616:
!identical(receiverType.kind, TypeKind.MALFORMED_TYPE)) {
On 2012/12/05 18:33:51, Johnni Winther wrote:
> On 2012/12/05 15:43:46, ngeoffray wrote:
> > Is that important? If it's malformed, it will say yes to any subtype check,
> > right? So having a TypedSelector should be fine.
> 
> It is to make a distinction between List<SomeMalformedType> and
> SomeMalformedType. In the first case we actually know as much as if the type
was
> List<dynamic> and we should use that, whereas in the latter case we know
> nothing.

What I'm trying to understand is whether that will lead to bad JS code being
generated (because of tree shaking on typed selectors), or it will lead to code
being better optimized. If it's the latter, then I would remove the check, since
a malformed program is not something we care about optimizing.

[Johnni Winther, 2012-12-05 19:11:20]

https://codereview.chromium.org/11412245/diff/12030/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/ssa/codegen.dart (right):

https://codereview.chromium.org/11412245/diff/12030/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/ssa/codegen.dart:1616:
!identical(receiverType.kind, TypeKind.MALFORMED_TYPE)) {
On 2012/12/05 18:37:38, ngeoffray wrote:
> On 2012/12/05 18:33:51, Johnni Winther wrote:
> > On 2012/12/05 15:43:46, ngeoffray wrote:
> > > Is that important? If it's malformed, it will say yes to any subtype
check,
> > > right? So having a TypedSelector should be fine.
> > 
> > It is to make a distinction between List<SomeMalformedType> and
> > SomeMalformedType. In the first case we actually know as much as if the type
> was
> > List<dynamic> and we should use that, whereas in the latter case we know
> > nothing.
> 
> What I'm trying to understand is whether that will lead to bad JS code being
> generated (because of tree shaking on typed selectors), or it will lead to
code
> being better optimized. If it's the latter, then I would remove the check,
since
> a malformed program is not something we care about optimizing.

It's the latter.