Bring type variables into static scope, but produce compile-time error when they are used.

Bring type variables into static scope, but produce compile-time error when they are used.

BUG=dartbug.com/5231
TEST=tests/language/type_variables_static_scoping_test.dart

[aam-me, 2012-09-27 03:42:46]

Peter, 

  I'm curious what you think about this changelist.
  Is this close to what you had in mind?
  ClassScope used to be the place to maintain information about whether context
is static or not, but it is doesn't seem to be useful now since we need to act
on the event that "type variable is used in static context" outside of Scope
lookup method.
  Also, several existing tests like
tests\language\type_variable_scope2_test.dart expect compile-time warning and
dynamic error for this kind of event, but according to the spec and 5231
description it is a compile-time error.

Thanks!

[ahe, 2012-10-02 07:50:25]

Hi Alexander,

Do you think i is possible to get rid of VariableScope?

Cheers,
Peter

https://chromiumcodereview.appspot.com/10996039/diff/1001/lib/compiler/implem...
File lib/compiler/implementation/resolver.dart (left):

https://chromiumcodereview.appspot.com/10996039/diff/1001/lib/compiler/implem...
lib/compiler/implementation/resolver.dart:2844: bool inStaticContext = false;
This is great.

https://chromiumcodereview.appspot.com/10996039/diff/1001/lib/compiler/implem...
File lib/compiler/implementation/resolver.dart (right):

https://chromiumcodereview.appspot.com/10996039/diff/1001/lib/compiler/implem...
lib/compiler/implementation/resolver.dart:2768: class VariableScope extends
Scope {
I think we should get rid of this one as well.

[ngeoffray, 2012-10-02 07:53:46]

Thanks Alexander for trying to fix this problem!

https://chromiumcodereview.appspot.com/10996039/diff/1001/tests/language/type...
File tests/language/type_variables_static_scoping_test.dart (right):

https://chromiumcodereview.appspot.com/10996039/diff/1001/tests/language/type...
tests/language/type_variables_static_scoping_test.dart:13: new Foo<int>().t = 1;
too many indentation

[Johnni Winther, 2012-10-02 09:07:29]

lgtm

https://chromiumcodereview.appspot.com/10996039/diff/1001/lib/compiler/implem...
File lib/compiler/implementation/elements/elements.dart (right):

https://chromiumcodereview.appspot.com/10996039/diff/1001/lib/compiler/implem...
lib/compiler/implementation/elements/elements.dart:1130: Scope buildScope() {
Remove this method. MethodScope is mutable and should only be created in the
ResolverVisitor.

https://chromiumcodereview.appspot.com/10996039/diff/1001/lib/compiler/implem...
File lib/compiler/implementation/resolver.dart (right):

https://chromiumcodereview.appspot.com/10996039/diff/1001/lib/compiler/implem...
lib/compiler/implementation/resolver.dart:2768: class VariableScope extends
Scope {
On 2012/10/02 07:50:25, ahe wrote:
> I think we should get rid of this one as well.

I agree. Remove this class.

[aam-me, 2012-10-02 12:50:39]

Thank you for the feedback, guys!
Please, review when you have a chance.

https://chromiumcodereview.appspot.com/10996039/diff/1001/lib/compiler/implem...
File lib/compiler/implementation/elements/elements.dart (right):

https://chromiumcodereview.appspot.com/10996039/diff/1001/lib/compiler/implem...
lib/compiler/implementation/elements/elements.dart:1130: Scope buildScope() {
Johnni,

  removing this method causes "Internal error: Cannot add an element in a class
scope" as in below. Did I miss something? Please, advise. Thanks!

===
C:\dart_bleeding\dart-repo.9\dart>tools\test.py -mrelease -cdart2js -rd8
co19/Language/03_Overview/1_Scoping_A02_t05
Test configuration: dart2js_d8_release_ia32
[00:00 |  --% | +    0 | -    0]
FAILED: dart2js-d8 release_ia32 co19/Language/03_Overview/1_Scoping_A02_t05
Expected: pass
Actual: crash
Unexpected compile-time error.

stdout:
lib/core/expect.dart:70:15: Error: The compiler crashed when compiling this
element.
  static void fail(String msg) {
              ^^^^
The compiler is broken.

...

Internal error: Cannot add an element in a class scope
#0      ClassScope.add
(file:///C:/dart_bleeding/dart-repo.9/dart/lib/compiler/implementation/resolver.dart:2854:5)
#1      ResolverVisitor.defineElement
(file:///C:/dart_bleeding/dart-repo.9/dart/lib/compiler/implementation/resolver.dart:1118:35)
#2      ResolverVisitor.setupFunction.<anonymous closure>
(file:///C:/dart_bleeding/dart-repo.9/dart/lib/compiler/implementation/resolver.dart:1167:22)
#3      FunctionSignature.forEachRequiredParameter
(file:///C:/dart_bleeding/dart-repo.9/dart/lib/compiler/implementation/elements/elements.dart:963:15)
#4      FunctionSignature.forEachParameter
(file:///C:/dart_bleeding/dart-repo.9/dart/lib/compiler/implementation/elements/elements.dart:988:29)
#5      ResolverVisitor.setupFunction
(file:///C:/dart_bleeding/dart-repo.9/dart/lib/compiler/implementation/resolver.dart:1154:40)
#6      ResolverTask.resolveMethodElement.<anonymous closure>
(file:///C:/dart_bleeding/dart-repo.9/dart/lib/compiler/implementation/resolver.dart:148:28)
#7      Compiler.withCurrentElement
(file:///C:/dart_bleeding/dart-repo.9/dart/lib/compiler/implementation/compiler.dart:135:15)
#8      Compiler.withCurrentElement
(file:///C:/dart_bleeding/dart-repo.9/dart/lib/compiler/implementation/compiler.dart:148:7)
#9      ResolverTask.resolveMethodElement
(file:///C:/dart_bleeding/dart-repo.9/dart/lib/compiler/implementation/resolver.dart:131:39)
#10     ResolverTask.resolve.<anonymous closure>
(file:///C:/dart_bleeding/dart-repo.9/dart/lib/compiler/implementation/resolver.dart:73:36)
...
===

https://chromiumcodereview.appspot.com/10996039/diff/1001/lib/compiler/implem...
File lib/compiler/implementation/resolver.dart (right):

https://chromiumcodereview.appspot.com/10996039/diff/1001/lib/compiler/implem...
lib/compiler/implementation/resolver.dart:2768: class VariableScope extends
Scope {
Thanks, done!

https://chromiumcodereview.appspot.com/10996039/diff/1001/tests/language/type...
File tests/language/type_variables_static_scoping_test.dart (right):

https://chromiumcodereview.appspot.com/10996039/diff/1001/tests/language/type...
tests/language/type_variables_static_scoping_test.dart:13: new Foo<int>().t = 1;
On 2012/10/02 07:53:46, ngeoffray wrote:
> too many indentation

Done.

[Johnni Winther, 2012-10-03 10:40:33]

https://chromiumcodereview.appspot.com/10996039/diff/1001/lib/compiler/implem...
File lib/compiler/implementation/elements/elements.dart (right):

https://chromiumcodereview.appspot.com/10996039/diff/1001/lib/compiler/implem...
lib/compiler/implementation/elements/elements.dart:1130: Scope buildScope() {
On 2012/10/02 12:50:39, aam wrote:
> Johnni,
> 
>   removing this method causes "Internal error: Cannot add an element in a
class
> scope" as in below. Did I miss something? Please, advise. Thanks!
> 
> ===
> C:\dart_bleeding\dart-repo.9\dart>tools\test.py -mrelease -cdart2js -rd8
> co19/Language/03_Overview/1_Scoping_A02_t05
> Test configuration: dart2js_d8_release_ia32
> [00:00 |  --% | +    0 | -    0]
> FAILED: dart2js-d8 release_ia32 co19/Language/03_Overview/1_Scoping_A02_t05
> Expected: pass
> Actual: crash
> Unexpected compile-time error.
> 
> stdout:
> lib/core/expect.dart:70:15: Error: The compiler crashed when compiling this
> element.
>   static void fail(String msg) {
>               ^^^^
> The compiler is broken.
> 
> ...
> 
> Internal error: Cannot add an element in a class scope
> #0      ClassScope.add
>
(file:///C:/dart_bleeding/dart-repo.9/dart/lib/compiler/implementation/resolver.dart:2854:5)
> #1      ResolverVisitor.defineElement
>
(file:///C:/dart_bleeding/dart-repo.9/dart/lib/compiler/implementation/resolver.dart:1118:35)
> #2      ResolverVisitor.setupFunction.<anonymous closure>
>
(file:///C:/dart_bleeding/dart-repo.9/dart/lib/compiler/implementation/resolver.dart:1167:22)
> #3      FunctionSignature.forEachRequiredParameter
>
(file:///C:/dart_bleeding/dart-repo.9/dart/lib/compiler/implementation/elements/elements.dart:963:15)
> #4      FunctionSignature.forEachParameter
>
(file:///C:/dart_bleeding/dart-repo.9/dart/lib/compiler/implementation/elements/elements.dart:988:29)
> #5      ResolverVisitor.setupFunction
>
(file:///C:/dart_bleeding/dart-repo.9/dart/lib/compiler/implementation/resolver.dart:1154:40)
> #6      ResolverTask.resolveMethodElement.<anonymous closure>
>
(file:///C:/dart_bleeding/dart-repo.9/dart/lib/compiler/implementation/resolver.dart:148:28)
> #7      Compiler.withCurrentElement
>
(file:///C:/dart_bleeding/dart-repo.9/dart/lib/compiler/implementation/compiler.dart:135:15)
> #8      Compiler.withCurrentElement
>
(file:///C:/dart_bleeding/dart-repo.9/dart/lib/compiler/implementation/compiler.dart:148:7)
> #9      ResolverTask.resolveMethodElement
>
(file:///C:/dart_bleeding/dart-repo.9/dart/lib/compiler/implementation/resolver.dart:131:39)
> #10     ResolverTask.resolve.<anonymous closure>
>
(file:///C:/dart_bleeding/dart-repo.9/dart/lib/compiler/implementation/resolver.dart:73:36)
> ...
> ===

OK. Just leave it there. I'll try to remove it in another CL.

[ahe, 2012-10-04 07:51:02]

I'm not sure about the test changes. Otherwise, this is awesome.

https://chromiumcodereview.appspot.com/10996039/diff/8002/lib/compiler/implem...
File lib/compiler/implementation/elements/elements.dart (right):

https://chromiumcodereview.appspot.com/10996039/diff/8002/lib/compiler/implem...
lib/compiler/implementation/elements/elements.dart:346: return modifiers !==
null && modifiers.isStatic();
Didn't Johnni land a change that ensures modifiers are non-null?

https://chromiumcodereview.appspot.com/10996039/diff/8002/lib/compiler/implem...
File lib/compiler/implementation/warnings.dart (right):

https://chromiumcodereview.appspot.com/10996039/diff/8002/lib/compiler/implem...
lib/compiler/implementation/warnings.dart:263: const MessageKind( 'cannot access
type variable #{1} in static context');
Extra space after (.

https://chromiumcodereview.appspot.com/10996039/diff/8002/tests/co19/co19-dar...
File tests/co19/co19-dart2js.status (right):

https://chromiumcodereview.appspot.com/10996039/diff/8002/tests/co19/co19-dar...
tests/co19/co19-dart2js.status:14: Language/03_Overview/1_Scoping_A02_t05: Fail
# Incorrect test - marked as static-warning, should be compile-error
Please file a bug at code.google.com/p/co19. Then move the line to line 360 and
follow the convention:

test/name: Fail, OK # co19 issue BUGID

https://chromiumcodereview.appspot.com/10996039/diff/8002/tests/language/type...
File tests/language/type_variables_static_scoping_test.dart (right):

https://chromiumcodereview.appspot.com/10996039/diff/8002/tests/language/type...
tests/language/type_variables_static_scoping_test.dart:1: class Foo<T> {
Missing copyright (see other files) and no test description.

[ahe, 2012-10-04 08:28:39]

On 2012/10/04 07:51:02, ahe wrote:
> I'm not sure about the test changes. Otherwise, this is awesome.

What I'm not sure about is the semantics. Gilad is in Aarhus right now, so I'll
talk to him and get it worked out.

[aam-me, 2012-10-10 02:48:40]

To support dynamic type error instead of compile-time error I introduced
malformed type and type element.
Please, let me know what you think.

Thank you!

https://chromiumcodereview.appspot.com/10996039/diff/8002/lib/compiler/implem...
File lib/compiler/implementation/elements/elements.dart (right):

https://chromiumcodereview.appspot.com/10996039/diff/8002/lib/compiler/implem...
lib/compiler/implementation/elements/elements.dart:346: return modifiers !==
null && modifiers.isStatic();
On 2012/10/04 07:51:02, ahe wrote:
> Didn't Johnni land a change that ensures modifiers are non-null?

Done.

https://chromiumcodereview.appspot.com/10996039/diff/8002/lib/compiler/implem...
File lib/compiler/implementation/warnings.dart (right):

https://chromiumcodereview.appspot.com/10996039/diff/8002/lib/compiler/implem...
lib/compiler/implementation/warnings.dart:263: const MessageKind( 'cannot access
type variable #{1} in static context');
On 2012/10/04 07:51:02, ahe wrote:
> Extra space after (.

Done.

[karlklose, 2012-10-10 07:19:33]

DBC:

Should we perhaps use the erroneous element as the element of malformed?

https://chromiumcodereview.appspot.com/10996039/diff/20001/lib/compiler/imple...
File lib/compiler/implementation/resolver.dart (right):

https://chromiumcodereview.appspot.com/10996039/diff/20001/lib/compiler/imple...
lib/compiler/implementation/resolver.dart:1056: DartType dtype = link.head;
dtype -> type. (The class is only called DartType because there already is a
class called type in the core library.)

https://chromiumcodereview.appspot.com/10996039/diff/20001/lib/compiler/imple...
lib/compiler/implementation/resolver.dart:1085: onFailure(node,
MessageKind.CANNOT_ACCESS_TYPE_VARIABLE_IN_STATIC_CONTEXT, [element.name]);
Long line.

https://chromiumcodereview.appspot.com/10996039/diff/20001/lib/compiler/imple...
lib/compiler/implementation/resolver.dart:2937: bool get inStaticContext =>
element.inStaticContext();//parent.inStaticContext();
Remove code in comment.

[Johnni Winther, 2012-10-18 06:19:32]

Please put this CL on hold as it conflicts with a scope clean-up in progress.

[Johnni Winther, 2012-11-21 14:39:57]

On 2012/10/18 06:19:32, Johnni Winther wrote:
> Please put this CL on hold as it conflicts with a scope clean-up in progress.

The new scoping rules are now implemented but the runtime behavior isn't. It
would be great if you could take another look at 'Produce warning and dynamic
type error instead of compile-time error.'. Probably done best in a clean CL.

[aam-me, 2012-11-21 21:43:19]

Okay, Johnni. I created https://codereview.chromium.org/11416144/ for runtime
behavior change. Closing this CL.