Produce run-time error when type parameters are accessed from static context.

Produce run-time error when type parameters are accessed from static context.



BUG=dartbug.com/5231
TEST=

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

[aam-me, 2012-11-21 21:42:30]

Hi,

  per Johnni's suggestion I picked up last part of
https://chromiumcodereview.appspot.com/10996039/ as new changelist.
  Hopefully this is good starting point.

  I could not see enough synergies between ErroneousElement and what I needed
for MalformedType element, therefore created MalformedTypeElement.
  Also in previous CL I passed information to TypeResolver whether we are in
static context or not via Scope class, but this time - via explicit
inStaticContext parameter.

[ahe, 2012-11-22 12:41:30]

Hi Alexander,

Happy Thanksgiving.  I would like to say that I'm thankful for your
contributions to the Dart project :-)

I was just thinking that I hadn't heard from you in a while.  Good to have you
back!

LGTM!

Cheers,
Peter

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

https://codereview.chromium.org/11416144/diff/3001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/elements/elements.dart:1327: final
TypeAnnotation type;
This is an ast node.  Do we assume that enclosing is an element whose
compilation-unit contains this node?

Generally, I would like to avoid having a field called "type" that isn't a
DartType.  How about renaming this to typeNode?

https://codereview.chromium.org/11416144/diff/3001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/elements/elements.dart:1327: final
TypeAnnotation type;
Separate field from constructor with a new line.

https://codereview.chromium.org/11416144/diff/3001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/elements/elements.dart:1331:
enclosing);
Add newline.

https://codereview.chromium.org/11416144/diff/3001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/elements/elements.dart:1332: DartType
computeType(compiler) => compiler.types.malformedType;
Ditto.

https://codereview.chromium.org/11416144/diff/3001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/elements/elements.dart:1333: Node
parseNode(_) => type;
Ditto.

https://codereview.chromium.org/11416144/diff/3001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/elements/elements.dart:1744: static
bool isUnresolved(Element e) => e == null || e.isErroneous() || e.isMalformed();
Long line.  As a matter of style, I then prefer to move to the longer syntax,
that is, { return ... }.

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

https://codereview.chromium.org/11416144/diff/3001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/resolution/members.dart:1137: }
This for loop is a great candidate for a helper method.

https://codereview.chromium.org/11416144/diff/3001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/resolution/members.dart:1162: new
ResolutionWarning(MessageKind.TYPE_VARIABLE_WITHIN_STATIC_MEMBER,
Long line.

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

https://codereview.chromium.org/11416144/diff/3001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:3516: Message message
= new ResolutionWarning(MessageKind.TYPE_VARIABLE_WITHIN_STATIC_MEMBER,
Long line.

https://codereview.chromium.org/11416144/diff/3001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:3516: Message message
= new ResolutionWarning(MessageKind.TYPE_VARIABLE_WITHIN_STATIC_MEMBER,
Alternatively:

MessageKind.TYPE_VARIABLE_WITHIN_STATIC_MEMBER.message([element])

[aam-me, 2012-11-23 02:16:46]

Thank you, Peter!
I'm glad I got some vacation days so I can work on Dart.

As I was running some tests in checked mode I realized that malformed type
exception should be thrown in checked mode only. So besides addressing your
comments I made the change to convert malformed type arguments to dynamic if
running in production mode. Hope it makes sense.

https://chromiumcodereview.appspot.com/11416144/diff/3001/sdk/lib/_internal/c...
File sdk/lib/_internal/compiler/implementation/elements/elements.dart (right):

https://chromiumcodereview.appspot.com/11416144/diff/3001/sdk/lib/_internal/c...
sdk/lib/_internal/compiler/implementation/elements/elements.dart:1327: final
TypeAnnotation type;
Currently in both cases when MalformedTypEelement is created enclosing element
is result of type name resolution of this given type(typeNode). Not sure what is
definition of compilation-unit, but I think it is possible to get type from some
other library resolved into element from that other library(which is different
compilation unit, right?).
For the purposes of type parameter name resolution(which is what
MalformedTypeElement is currently used for) I think type parameter name can only
come current compilation unit.

What is the concern for situation when element's compilation-unit doesn't
contain the node?

https://chromiumcodereview.appspot.com/11416144/diff/3001/sdk/lib/_internal/c...
sdk/lib/_internal/compiler/implementation/elements/elements.dart:1331:
enclosing);
On 2012/11/22 12:41:30, ahe wrote:
> Add newline.

Done.

https://chromiumcodereview.appspot.com/11416144/diff/3001/sdk/lib/_internal/c...
sdk/lib/_internal/compiler/implementation/elements/elements.dart:1332: DartType
computeType(compiler) => compiler.types.malformedType;
On 2012/11/22 12:41:30, ahe wrote:
> Ditto.

Done.

https://chromiumcodereview.appspot.com/11416144/diff/3001/sdk/lib/_internal/c...
sdk/lib/_internal/compiler/implementation/elements/elements.dart:1333: Node
parseNode(_) => type;
On 2012/11/22 12:41:30, ahe wrote:
> Ditto.

Done.

https://chromiumcodereview.appspot.com/11416144/diff/3001/sdk/lib/_internal/c...
sdk/lib/_internal/compiler/implementation/elements/elements.dart:1744: static
bool isUnresolved(Element e) => e == null || e.isErroneous() || e.isMalformed();
On 2012/11/22 12:41:30, ahe wrote:
> Long line.  As a matter of style, I then prefer to move to the longer syntax,
> that is, { return ... }.

Done.

https://chromiumcodereview.appspot.com/11416144/diff/3001/sdk/lib/_internal/c...
File sdk/lib/_internal/compiler/implementation/resolution/members.dart (right):

https://chromiumcodereview.appspot.com/11416144/diff/3001/sdk/lib/_internal/c...
sdk/lib/_internal/compiler/implementation/resolution/members.dart:1137: }
On 2012/11/22 12:41:30, ahe wrote:
> This for loop is a great candidate for a helper method.

Done.

https://chromiumcodereview.appspot.com/11416144/diff/3001/sdk/lib/_internal/c...
sdk/lib/_internal/compiler/implementation/resolution/members.dart:1162: new
ResolutionWarning(MessageKind.TYPE_VARIABLE_WITHIN_STATIC_MEMBER,
On 2012/11/22 12:41:30, ahe wrote:
> Long line.

Done.

https://chromiumcodereview.appspot.com/11416144/diff/3001/sdk/lib/_internal/c...
File sdk/lib/_internal/compiler/implementation/ssa/builder.dart (right):

https://chromiumcodereview.appspot.com/11416144/diff/3001/sdk/lib/_internal/c...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:3516: Message message
= new ResolutionWarning(MessageKind.TYPE_VARIABLE_WITHIN_STATIC_MEMBER,
On 2012/11/22 12:41:30, ahe wrote:
> Long line.

Done.

[ahe, 2012-11-23 06:16:13]

https://chromiumcodereview.appspot.com/11416144/diff/3001/sdk/lib/_internal/c...
File sdk/lib/_internal/compiler/implementation/elements/elements.dart (right):

https://chromiumcodereview.appspot.com/11416144/diff/3001/sdk/lib/_internal/c...
sdk/lib/_internal/compiler/implementation/elements/elements.dart:1327: final
TypeAnnotation type;
On 2012/11/23 02:16:47, aam wrote:
> Currently in both cases when MalformedTypEelement is created enclosing element
> is result of type name resolution of this given type(typeNode). Not sure what
is
> definition of compilation-unit, but I think it is possible to get type from
some
> other library resolved into element from that other library(which is different
> compilation unit, right?).
> For the purposes of type parameter name resolution(which is what
> MalformedTypeElement is currently used for) I think type parameter name can
only
> come current compilation unit.
> 
> What is the concern for situation when element's compilation-unit doesn't
> contain the node?

All AST nodes and tokens record a character offset from the beginning of the
current file.  However, they don't know what the current file is.  This is
recorded by the element (via its reference to a compilation unit).

https://chromiumcodereview.appspot.com/11416144/diff/8003/sdk/lib/_internal/c...
File sdk/lib/_internal/compiler/implementation/resolution/members.dart (right):

https://chromiumcodereview.appspot.com/11416144/diff/8003/sdk/lib/_internal/c...
sdk/lib/_internal/compiler/implementation/resolution/members.dart:1201: argType
= compiler.types.dynamicType;
I would rather do this check in the backend.  Here is why: we try to keep the
resolution phase as close to the truth as possible.  In this case, the type
really is malformed, and if you were building, for example, an IDE based on
dart2js, it would be nice to have the details (that is the malformed type)
rather than the dynamic type.

[aam-me, 2012-11-23 14:02:27]

Thank you for the review, Peter!

PTAL

https://chromiumcodereview.appspot.com/11416144/diff/3001/sdk/lib/_internal/c...
File sdk/lib/_internal/compiler/implementation/elements/elements.dart (right):

https://chromiumcodereview.appspot.com/11416144/diff/3001/sdk/lib/_internal/c...
sdk/lib/_internal/compiler/implementation/elements/elements.dart:1327: final
TypeAnnotation type;
On 2012/11/23 06:16:13, ahe wrote:
> > What is the concern for situation when element's compilation-unit doesn't
> > contain the node?
> 
> All AST nodes and tokens record a character offset from the beginning of the
> current file.  However, they don't know what the current file is.  This is
> recorded by the element (via its reference to a compilation unit).

I see. In our case "enclosing" is either class element which has type variable
reference or the type variable reference itself(element.isTypeVariable()). In
both cases I believe element and node are part of the same compilation unit.

https://chromiumcodereview.appspot.com/11416144/diff/8003/sdk/lib/_internal/c...
File sdk/lib/_internal/compiler/implementation/resolution/members.dart (right):

https://chromiumcodereview.appspot.com/11416144/diff/8003/sdk/lib/_internal/c...
sdk/lib/_internal/compiler/implementation/resolution/members.dart:1201: argType
= compiler.types.dynamicType;
Okay, understand.
Started looking for a good place in the backend for this replacement logic, but
discovered that it is not needed at all.

[ahe, 2012-11-23 15:26:09]

LGTM!

https://chromiumcodereview.appspot.com/11416144/diff/3001/sdk/lib/_internal/c...
File sdk/lib/_internal/compiler/implementation/elements/elements.dart (right):

https://chromiumcodereview.appspot.com/11416144/diff/3001/sdk/lib/_internal/c...
sdk/lib/_internal/compiler/implementation/elements/elements.dart:1327: final
TypeAnnotation type;
On 2012/11/23 14:02:27, aam wrote:
> On 2012/11/23 06:16:13, ahe wrote:
> > > What is the concern for situation when element's compilation-unit doesn't
> > > contain the node?
> > 
> > All AST nodes and tokens record a character offset from the beginning of the
> > current file.  However, they don't know what the current file is.  This is
> > recorded by the element (via its reference to a compilation unit).
> 
> I see. In our case "enclosing" is either class element which has type variable
> reference or the type variable reference itself(element.isTypeVariable()). In
> both cases I believe element and node are part of the same compilation unit.

That makes sense.  But we have to watch out for Johnni who might extend this
class to other situations.  He may break that invariant :-)

[ngeoffray, 2012-11-27 09:37:34]

LGTM! Thanks Alexander, that's a very nice change.

https://chromiumcodereview.appspot.com/11416144/diff/8005/sdk/lib/_internal/c...
File
sdk/lib/_internal/compiler/implementation/dart_backend/placeholder_collector.dart
(right):

https://chromiumcodereview.appspot.com/11416144/diff/8005/sdk/lib/_internal/c...
sdk/lib/_internal/compiler/implementation/dart_backend/placeholder_collector.dart:368:
if (!Elements.isErroneousElement(constructor) &&
Can that be !Elements.isUnresolved(constructor) ?

https://chromiumcodereview.appspot.com/11416144/diff/8005/sdk/lib/_internal/c...
File sdk/lib/_internal/compiler/implementation/resolution/members.dart (right):

https://chromiumcodereview.appspot.com/11416144/diff/8005/sdk/lib/_internal/c...
sdk/lib/_internal/compiler/implementation/resolution/members.dart:1041: bool
anyMalformedTypesInThere(Link<DartType> list) {
Personal taste, but I would remove 'InThere'.

https://chromiumcodereview.appspot.com/11416144/diff/8005/sdk/lib/_internal/c...
sdk/lib/_internal/compiler/implementation/resolution/members.dart:1139: // In
checked mode malformed-ness of the type argument bubbles up.
Why not in non-checked mode? Is that an optimization because codegen does not
need it? For robutsness sake, I would still bubble it up in all cases.

https://chromiumcodereview.appspot.com/11416144/diff/8005/sdk/lib/_internal/c...
File sdk/lib/_internal/compiler/implementation/ssa/builder.dart (right):

https://chromiumcodereview.appspot.com/11416144/diff/8005/sdk/lib/_internal/c...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:3447:
!Elements.isMalformedElement(element)) {
!Elements.isUnresolved ?

https://chromiumcodereview.appspot.com/11416144/diff/8005/sdk/lib/_internal/c...
File sdk/lib/_internal/compiler/implementation/typechecker.dart (right):

https://chromiumcodereview.appspot.com/11416144/diff/8005/sdk/lib/_internal/c...
sdk/lib/_internal/compiler/implementation/typechecker.dart:167: bool operator
==(other) => other is MalformedType;
Not checking if elements are equal is kind of strange. Could you add a comment?

[aam-me, 2012-11-28 01:49:46]

Thank you, Nicolas, for the review! 
All good comments, but the CL is closed now as it was committed couple day ago.
What options do I have now? What is the best way to incorporate your feedback?

https://chromiumcodereview.appspot.com/11416144/diff/8005/sdk/lib/_internal/c...
File
sdk/lib/_internal/compiler/implementation/dart_backend/placeholder_collector.dart
(right):

https://chromiumcodereview.appspot.com/11416144/diff/8005/sdk/lib/_internal/c...
sdk/lib/_internal/compiler/implementation/dart_backend/placeholder_collector.dart:368:
if (!Elements.isErroneousElement(constructor) &&
Yes, good idea!

https://chromiumcodereview.appspot.com/11416144/diff/8005/sdk/lib/_internal/c...
File sdk/lib/_internal/compiler/implementation/resolution/members.dart (right):

https://chromiumcodereview.appspot.com/11416144/diff/8005/sdk/lib/_internal/c...
sdk/lib/_internal/compiler/implementation/resolution/members.dart:1041: bool
anyMalformedTypesInThere(Link<DartType> list) {
On 2012/11/27 09:37:35, ngeoffray wrote:
> Personal taste, but I would remove 'InThere'.
I understand. No objections to this.

https://chromiumcodereview.appspot.com/11416144/diff/8005/sdk/lib/_internal/c...
sdk/lib/_internal/compiler/implementation/resolution/members.dart:1139: // In
checked mode malformed-ness of the type argument bubbles up.
When we produce MalformedTypeElement here, it triggers runtime exception
generation in builder in codegen phase. We don't want to have runtime exception
in production mode.

https://chromiumcodereview.appspot.com/11416144/diff/8005/sdk/lib/_internal/c...
File sdk/lib/_internal/compiler/implementation/ssa/builder.dart (right):

https://chromiumcodereview.appspot.com/11416144/diff/8005/sdk/lib/_internal/c...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:3447:
!Elements.isMalformedElement(element)) {
Yes, agree. Good idea.

https://chromiumcodereview.appspot.com/11416144/diff/8005/sdk/lib/_internal/c...
File sdk/lib/_internal/compiler/implementation/typechecker.dart (right):

https://chromiumcodereview.appspot.com/11416144/diff/8005/sdk/lib/_internal/c...
sdk/lib/_internal/compiler/implementation/typechecker.dart:167: bool operator
==(other) => other is MalformedType;
Good point. I used VoidType above as a template.

But it makes sense to change it to something like this:
  bool operator ==(other) {
    if (other is !MalformedType) return false;
    if (!identical(element, other.element)) return false;
    return true;
  }

[ahe, 2012-11-28 02:08:50]

https://chromiumcodereview.appspot.com/11416144/diff/8005/sdk/lib/_internal/c...
File sdk/lib/_internal/compiler/implementation/resolution/members.dart (right):

https://chromiumcodereview.appspot.com/11416144/diff/8005/sdk/lib/_internal/c...
sdk/lib/_internal/compiler/implementation/resolution/members.dart:1139: // In
checked mode malformed-ness of the type argument bubbles up.
On 2012/11/28 01:49:47, aam wrote:
> When we produce MalformedTypeElement here, it triggers runtime exception
> generation in builder in codegen phase. We don't want to have runtime
exception
> in production mode.

I think what Nicolas is saying is that we generally try to keep resolution
independent of production mode or not. There are some long term benefits to
this, and short term, the benefits would be that our source mirror API always
behave the same.

So ideally, we would let the backend decide how to handle a malformed type,
depending on the mode.

[aam-me, 2012-11-29 05:18:06]

Thank you, Peter.

I picked up the comments and addressed them in
https://chromiumcodereview.appspot.com/11348294/.