Handle type variable in static member.

Handle type variable in static member.

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

[ahe, 2012-10-19 11:58:04]

LGTM, if you deal with the is check.

https://codereview.chromium.org/11229002/diff/1/lib/compiler/implementation/r...
File lib/compiler/implementation/resolution/members.dart (right):

https://codereview.chromium.org/11229002/diff/1/lib/compiler/implementation/r...
lib/compiler/implementation/resolution/members.dart:1341: if (type is
TypeVariableType && enclosingElement.isInStaticMember()) {
I would prefer using .kind over is tests. I would like to have a DynamicType
which implements all other types.

[karlklose, 2012-10-19 12:05:38]

LGTM.

https://codereview.chromium.org/11229002/diff/1/lib/compiler/implementation/e...
File lib/compiler/implementation/elements/elements.dart (right):

https://codereview.chromium.org/11229002/diff/1/lib/compiler/implementation/e...
lib/compiler/implementation/elements/elements.dart:164: bool isInStaticMember()
{
I find it confusing, that getEnclosingMember may return the element itself.
Please add a comment.

Can we change the behavior of getEnclosingMember?

https://codereview.chromium.org/11229002/diff/1/lib/compiler/implementation/r...
File lib/compiler/implementation/resolution/members.dart (right):

https://codereview.chromium.org/11229002/diff/1/lib/compiler/implementation/r...
lib/compiler/implementation/resolution/members.dart:1089: DartType
resolveTypeAnnotation(TypeAnnotation node, Scope scope,
Maybe put the first two arguments on their on lines, too?

https://codereview.chromium.org/11229002/diff/1/lib/compiler/implementation/r...
lib/compiler/implementation/resolution/members.dart:1337: DartType
useType(TypeAnnotation annotation, DartType type) {
I think you should rename this helper.

https://codereview.chromium.org/11229002/diff/1/lib/compiler/implementation/r...
lib/compiler/implementation/resolution/members.dart:1343: [type]);
Indentation is wrong.

[ahe, 2012-10-19 12:22:40]

https://codereview.chromium.org/11229002/diff/1/lib/compiler/implementation/e...
File lib/compiler/implementation/elements/elements.dart (right):

https://codereview.chromium.org/11229002/diff/1/lib/compiler/implementation/e...
lib/compiler/implementation/elements/elements.dart:164: bool isInStaticMember()
{
On 2012/10/19 12:05:38, karlklose wrote:
> I find it confusing, that getEnclosingMember may return the element itself.
> Please add a comment.
> 
> Can we change the behavior of getEnclosingMember?

I find the behavior really useful. So I would rather investigate better names.

[Johnni Winther, 2012-10-22 09:03:32]

TypeKind added for DartType. PTAL

https://codereview.chromium.org/11229002/diff/1/lib/compiler/implementation/e...
File lib/compiler/implementation/elements/elements.dart (right):

https://codereview.chromium.org/11229002/diff/1/lib/compiler/implementation/e...
lib/compiler/implementation/elements/elements.dart:164: bool isInStaticMember()
{
On 2012/10/19 12:05:38, karlklose wrote:
> I find it confusing, that getEnclosingMember may return the element itself.
> Please add a comment.
> 
> Can we change the behavior of getEnclosingMember?

[getEnclosingMember] is only used by [isInStaticMember]. I've put a doc on
[getEnclosingMember] saying that it can return itself. I cannot think of a
better name for [getEnclosingMember].

https://codereview.chromium.org/11229002/diff/1/lib/compiler/implementation/r...
File lib/compiler/implementation/resolution/members.dart (right):

https://codereview.chromium.org/11229002/diff/1/lib/compiler/implementation/r...
lib/compiler/implementation/resolution/members.dart:1089: DartType
resolveTypeAnnotation(TypeAnnotation node, Scope scope,
On 2012/10/19 12:05:38, karlklose wrote:
> Maybe put the first two arguments on their on lines, too?

Done.

https://codereview.chromium.org/11229002/diff/1/lib/compiler/implementation/r...
lib/compiler/implementation/resolution/members.dart:1337: DartType
useType(TypeAnnotation annotation, DartType type) {
On 2012/10/19 12:05:38, karlklose wrote:
> I think you should rename this helper.

Renamed and doc added.

https://codereview.chromium.org/11229002/diff/1/lib/compiler/implementation/r...
lib/compiler/implementation/resolution/members.dart:1341: if (type is
TypeVariableType && enclosingElement.isInStaticMember()) {
On 2012/10/19 11:58:04, ahe wrote:
> I would prefer using .kind over is tests. I would like to have a DynamicType
> which implements all other types.

TypeKind added.

https://codereview.chromium.org/11229002/diff/1/lib/compiler/implementation/r...
lib/compiler/implementation/resolution/members.dart:1343: [type]);
On 2012/10/19 12:05:38, karlklose wrote:
> Indentation is wrong.

Done.

[Johnni Winther, 2012-10-22 12:18:56]

PTAL

[Johnni Winther, 2012-10-23 09:36:58]

Ping.

[ahe, 2012-10-23 11:34:58]

I think you should land the tests changes in a separate CL.

https://codereview.chromium.org/11229002/diff/7001/tests/language/language_da...
File tests/language/language_dart2js.status (right):

https://codereview.chromium.org/11229002/diff/7001/tests/language/language_da...
tests/language/language_dart2js.status:9: type_parameter_test: Skip # Deprecated
test of issue 4932
Please do not skip tests. If the test is bad, delete it. If the test fails, mark
it as failing.

Also, this line does not fit the comment on line 6.

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

https://codereview.chromium.org/11229002/diff/7001/tests/language/type_variab...
tests/language/type_variable_scope2_test.dart:20: Foo<T> /// 01: compile-time
error
I would like to see a reference to the specification that defend this test.

https://codereview.chromium.org/11229002/diff/7001/tests/language/type_variab...
File tests/language/type_variable_scope_test.dart (right):

https://codereview.chromium.org/11229002/diff/7001/tests/language/type_variab...
tests/language/type_variable_scope_test.dart:11: Foo<T> /// 00: compile-time
error
Same as previous test, I would like to see a specification quote.

[Johnni Winther, 2012-10-23 12:09:13]

Test update moved to another CL. PTAL.

[ahe, 2012-10-23 16:17:13]

LGTM if you address my comment about language_dart2js.status.

https://codereview.chromium.org/11229002/diff/12002/lib/compiler/implementati...
File lib/compiler/implementation/resolution/members.dart (right):

https://codereview.chromium.org/11229002/diff/12002/lib/compiler/implementati...
lib/compiler/implementation/resolution/members.dart:1962: void
checkAndUseType(TypeAnnotation annotation, DartType type) {
Add TODO to say we should get rid of this.

https://codereview.chromium.org/11229002/diff/12002/tests/language/language_d...
File tests/language/language_dart2js.status (right):

https://codereview.chromium.org/11229002/diff/12002/tests/language/language_d...
tests/language/language_dart2js.status:9: type_parameter_test: Skip # Deprecated
test of issue 4932
Please do not skip tests, also, this test does not belong in this section.

[Johnni Winther, 2012-10-24 14:28:35]

https://codereview.chromium.org/11229002/diff/12002/lib/compiler/implementati...
File lib/compiler/implementation/resolution/members.dart (right):

https://codereview.chromium.org/11229002/diff/12002/lib/compiler/implementati...
lib/compiler/implementation/resolution/members.dart:1962: void
checkAndUseType(TypeAnnotation annotation, DartType type) {
On 2012/10/23 16:17:13, ahe wrote:
> Add TODO to say we should get rid of this.

Done.

https://codereview.chromium.org/11229002/diff/12002/tests/language/language_d...
File tests/language/language_dart2js.status (right):

https://codereview.chromium.org/11229002/diff/12002/tests/language/language_d...
tests/language/language_dart2js.status:9: type_parameter_test: Skip # Deprecated
test of issue 4932
On 2012/10/23 16:17:13, ahe wrote:
> Please do not skip tests, also, this test does not belong in this section.

Line removed.

[Johnni Winther, 2012-10-26 08:12:19]

PTAL

[ahe, 2012-10-26 08:18:40]

LGTM

https://codereview.chromium.org/11229002/diff/16001/lib/compiler/implementati...
File lib/compiler/implementation/typechecker.dart (right):

https://codereview.chromium.org/11229002/diff/16001/lib/compiler/implementati...
lib/compiler/implementation/typechecker.dart:37: static const TypeKind VOID =
const TypeKind('void');
How about adding a "toString" mehod?

[Johnni Winther, 2012-10-29 09:20:14]

https://codereview.chromium.org/11229002/diff/16001/lib/compiler/implementati...
File lib/compiler/implementation/typechecker.dart (right):

https://codereview.chromium.org/11229002/diff/16001/lib/compiler/implementati...
lib/compiler/implementation/typechecker.dart:37: static const TypeKind VOID =
const TypeKind('void');
On 2012/10/26 08:18:40, ahe wrote:
> How about adding a "toString" mehod?

Done.