Generate a warning and a runtime error for calls to nonexistent static calls, getters and setters.

Generate a warning and a runtime error for calls to nonexistent static calls, getters and setters.

R=ngeoffray@google.com
BUG=3086, 3089, 3095
Committed: https://code.google.com/p/dart/source/detail?r=12011

[kasperl, 2012-09-05 09:29:45]

DBC:

https://chromiumcodereview.appspot.com/10920089/diff/1/lib/compiler/implement...
File lib/compiler/implementation/resolver.dart (right):

https://chromiumcodereview.appspot.com/10920089/diff/1/lib/compiler/implement...
lib/compiler/implementation/resolver.dart:1419: ResolutionWarning warning =
This looks ripe for refactoring. Couldn't we share the code that creates a new
resolution warning, reports it and returns a new fake, erroneous element?

https://chromiumcodereview.appspot.com/10920089/diff/1/lib/compiler/implement...
File lib/compiler/implementation/ssa/builder.dart (right):

https://chromiumcodereview.appspot.com/10920089/diff/1/lib/compiler/implement...
lib/compiler/implementation/ssa/builder.dart:2012: // An erroneous element
indicates an unresolved static getter.
Introduce new helper for this? Moar code sharing for the win.

https://chromiumcodereview.appspot.com/10920089/diff/1/lib/compiler/implement...
lib/compiler/implementation/ssa/builder.dart:3087: } else if
(variable.isErroneous()) {
Add a comment in the empty braces to make it clear that this is intentional.

[karlklose, 2012-09-05 11:10:47]

Thanks Kasper.

https://chromiumcodereview.appspot.com/10920089/diff/1/lib/compiler/implement...
File lib/compiler/implementation/resolver.dart (right):

https://chromiumcodereview.appspot.com/10920089/diff/1/lib/compiler/implement...
lib/compiler/implementation/resolver.dart:1419: ResolutionWarning warning =
On 2012/09/05 09:29:45, kasperl wrote:
> This looks ripe for refactoring. Couldn't we share the code that creates a new
> resolution warning, reports it and returns a new fake, erroneous element?

Done.

https://chromiumcodereview.appspot.com/10920089/diff/1/lib/compiler/implement...
File lib/compiler/implementation/ssa/builder.dart (right):

https://chromiumcodereview.appspot.com/10920089/diff/1/lib/compiler/implement...
lib/compiler/implementation/ssa/builder.dart:2012: // An erroneous element
indicates an unresolved static getter.
On 2012/09/05 09:29:45, kasperl wrote:
> Introduce new helper for this? Moar code sharing for the win.

Done.

https://chromiumcodereview.appspot.com/10920089/diff/1/lib/compiler/implement...
lib/compiler/implementation/ssa/builder.dart:3087: } else if
(variable.isErroneous()) {
On 2012/09/05 09:29:45, kasperl wrote:
> Add a comment in the empty braces to make it clear that this is intentional.

Done.

[kasperl, 2012-09-05 11:16:01]

I think the refactoring made the code much easier to understand. Thanks! Two
more comments and I'll let Nicolas deal with the real review :-)

https://chromiumcodereview.appspot.com/10920089/diff/1011/lib/compiler/implem...
File lib/compiler/implementation/ssa/builder.dart (right):

https://chromiumcodereview.appspot.com/10920089/diff/1011/lib/compiler/implem...
lib/compiler/implementation/ssa/builder.dart:2699: argumentValues =
<HInstruction>[];
Could this be done using argumentNodes.map?

https://chromiumcodereview.appspot.com/10920089/diff/1011/tests/language/call...
File tests/language/call_nonexistent_static_test.dart (right):

https://chromiumcodereview.appspot.com/10920089/diff/1011/tests/language/call...
tests/language/call_nonexistent_static_test.dart:10: try {
Consider using Expect.throws for these cases. Something like:

   Expect.throws(() => hest = 1, (e) => e is NoSuchMethodException)

should do the trick.

[ngeoffray, 2012-09-05 11:46:44]

LGTM

http://codereview.chromium.org/10920089/diff/10001/lib/compiler/implementatio...
File lib/compiler/implementation/elements/elements.dart (right):

http://codereview.chromium.org/10920089/diff/10001/lib/compiler/implementatio...
lib/compiler/implementation/elements/elements.dart:321:
ErroneousElement(this.errorMessage, Element enclosing)
As discussed, you might want to pass a node to this element, for error
reporting.

http://codereview.chromium.org/10920089/diff/10001/lib/compiler/implementatio...
lib/compiler/implementation/elements/elements.dart:334: getLibrary() =>
enclosingElement.getLibrary();
I think you don't need this.

http://codereview.chromium.org/10920089/diff/10001/lib/compiler/implementatio...
File lib/compiler/implementation/resolved_visitor.dart (right):

http://codereview.chromium.org/10920089/diff/10001/lib/compiler/implementatio...
lib/compiler/implementation/resolved_visitor.dart:22: if (element == null ||
element.isInstanceMember()) {
How can the isInstanceMember happen?

http://codereview.chromium.org/10920089/diff/10001/lib/compiler/implementatio...
File lib/compiler/implementation/resolver.dart (right):

http://codereview.chromium.org/10920089/diff/10001/lib/compiler/implementatio...
lib/compiler/implementation/resolver.dart:1350:
warnAndCreateErroneousElement(node.selector,
Move warnAndcreateErroneouselement one up, and put the parameters on the same
line (4 space indent).

http://codereview.chromium.org/10920089/diff/10001/lib/compiler/implementatio...
lib/compiler/implementation/resolver.dart:1423:
MessageKind.CANNOT_RESOLVE_GETTER);
ditto

http://codereview.chromium.org/10920089/diff/10001/lib/compiler/implementatio...
File lib/compiler/implementation/ssa/builder.dart (right):

http://codereview.chromium.org/10920089/diff/10001/lib/compiler/implementatio...
lib/compiler/implementation/ssa/builder.dart:1968: String targetName =
error.errorMessage.arguments[0].toString();
I would put the information in the ErroneousElement instead. This dependency on
the message doesn't look right.

http://codereview.chromium.org/10920089/diff/10001/lib/compiler/implementatio...
lib/compiler/implementation/ssa/builder.dart:2064: } else if (element != null &&
Element.isInvalid(element)) {
This null check feels quite annoying. Could we have isInvalid not return true
for null? A null element on a node is not necesseraly 'invalid' so the name is
quite confusing already.

http://codereview.chromium.org/10920089/diff/10001/tests/language/getter_no_s...
File tests/language/getter_no_setter2_test.dart (right):

http://codereview.chromium.org/10920089/diff/10001/tests/language/getter_no_s...
tests/language/getter_no_setter2_test.dart:30: nextVar++;  /// 01: static type
warning, runtime error
Why a type warning? It looks like a compile-time error to not have a setter
here.

http://codereview.chromium.org/10920089/diff/10001/tests/language/setter_no_g...
File tests/language/setter_no_getter_test.dart (right):

http://codereview.chromium.org/10920089/diff/10001/tests/language/setter_no_g...
tests/language/setter_no_getter_test.dart:5: /*    /// 01: static type warning,
runtime error
static type warning -> compile-time error?

http://codereview.chromium.org/10920089/diff/10001/tests/standalone/standalon...
File tests/standalone/standalone.status (right):

http://codereview.chromium.org/10920089/diff/10001/tests/standalone/standalon...
tests/standalone/standalone.status:61: io/process_exit_negative_test: Fail #
Exit handler is not called.
Is that a Fail, OK?

[karlklose, 2012-09-05 14:53:42]

PTAL.

Unresolved static calls, gets and sets without a receiver are still compile time
errors, because they are equivalent to writing this before them.

I reverted the corresponding changes and implemented the feature for calls to
class statics.

https://chromiumcodereview.appspot.com/10920089/diff/1011/lib/compiler/implem...
File lib/compiler/implementation/ssa/builder.dart (right):

https://chromiumcodereview.appspot.com/10920089/diff/1011/lib/compiler/implem...
lib/compiler/implementation/ssa/builder.dart:2699: argumentValues =
<HInstruction>[];
No, this is a Link, which does not have map.

https://chromiumcodereview.appspot.com/10920089/diff/1011/tests/language/call...
File tests/language/call_nonexistent_static_test.dart (right):

https://chromiumcodereview.appspot.com/10920089/diff/1011/tests/language/call...
tests/language/call_nonexistent_static_test.dart:10: try {
On 2012/09/05 11:16:01, kasperl wrote:
> Consider using Expect.throws for these cases. Something like:
> 
>    Expect.throws(() => hest = 1, (e) => e is NoSuchMethodException)
> 
> should do the trick.

Done.

https://chromiumcodereview.appspot.com/10920089/diff/10001/lib/compiler/imple...
File lib/compiler/implementation/elements/elements.dart (right):

https://chromiumcodereview.appspot.com/10920089/diff/10001/lib/compiler/imple...
lib/compiler/implementation/elements/elements.dart:321:
ErroneousElement(this.errorMessage, Element enclosing)
For now, I changed internalError to report on the enclosing element.

https://chromiumcodereview.appspot.com/10920089/diff/10001/lib/compiler/imple...
lib/compiler/implementation/elements/elements.dart:334: getLibrary() =>
enclosingElement.getLibrary();
It is called from the Element constructor in an assert.

https://chromiumcodereview.appspot.com/10920089/diff/10001/lib/compiler/imple...
File lib/compiler/implementation/resolved_visitor.dart (right):

https://chromiumcodereview.appspot.com/10920089/diff/10001/lib/compiler/imple...
lib/compiler/implementation/resolved_visitor.dart:22: if (element == null ||
element.isInstanceMember()) {
It cannot.

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

https://chromiumcodereview.appspot.com/10920089/diff/10001/lib/compiler/imple...
lib/compiler/implementation/resolver.dart:1350:
warnAndCreateErroneousElement(node.selector,
This change is reverted.

https://chromiumcodereview.appspot.com/10920089/diff/10001/lib/compiler/imple...
lib/compiler/implementation/resolver.dart:1423:
MessageKind.CANNOT_RESOLVE_GETTER);
On 2012/09/05 11:46:45, ngeoffray wrote:
> ditto

ditto.

https://chromiumcodereview.appspot.com/10920089/diff/10001/lib/compiler/imple...
File lib/compiler/implementation/ssa/builder.dart (right):

https://chromiumcodereview.appspot.com/10920089/diff/10001/lib/compiler/imple...
lib/compiler/implementation/ssa/builder.dart:1968: String targetName =
error.errorMessage.arguments[0].toString();
On 2012/09/05 11:46:45, ngeoffray wrote:
> I would put the information in the ErroneousElement instead. This dependency
on
> the message doesn't look right.

Done.

https://chromiumcodereview.appspot.com/10920089/diff/10001/lib/compiler/imple...
lib/compiler/implementation/ssa/builder.dart:2064: } else if (element != null &&
Element.isInvalid(element)) {
I renamed isInvalid to isUnresolved and added isErroneousElement to get rid of
this test.

https://chromiumcodereview.appspot.com/10920089/diff/10001/tests/language/get...
File tests/language/getter_no_setter2_test.dart (right):

https://chromiumcodereview.appspot.com/10920089/diff/10001/tests/language/get...
tests/language/getter_no_setter2_test.dart:30: nextVar++;  /// 01: static type
warning, runtime error
This change is reverted.

https://chromiumcodereview.appspot.com/10920089/diff/10001/tests/language/set...
File tests/language/setter_no_getter_test.dart (right):

https://chromiumcodereview.appspot.com/10920089/diff/10001/tests/language/set...
tests/language/setter_no_getter_test.dart:5: /*    /// 01: static type warning,
runtime error
This change was reverted.

https://chromiumcodereview.appspot.com/10920089/diff/10001/tests/standalone/s...
File tests/standalone/standalone.status (right):

https://chromiumcodereview.appspot.com/10920089/diff/10001/tests/standalone/s...
tests/standalone/standalone.status:61: io/process_exit_negative_test: Fail #
Exit handler is not called.
This change was reverted.

[ngeoffray, 2012-09-07 08:12:46]

LGTM

https://chromiumcodereview.appspot.com/10920089/diff/10001/lib/compiler/imple...
File lib/compiler/implementation/elements/elements.dart (right):

https://chromiumcodereview.appspot.com/10920089/diff/10001/lib/compiler/imple...
lib/compiler/implementation/elements/elements.dart:334: getLibrary() =>
enclosingElement.getLibrary();
On 2012/09/05 14:53:42, karlklose wrote:
> It is called from the Element constructor in an assert.

Bu how is this implementation different than Element.getLibrary?

https://chromiumcodereview.appspot.com/10920089/diff/6004/lib/compiler/implem...
File lib/compiler/implementation/compiler.dart (right):

https://chromiumcodereview.appspot.com/10920089/diff/6004/lib/compiler/implem...
lib/compiler/implementation/compiler.dart:827: if
(Element.isErroneousElement(element)) {
Instead of special casing here, could you implement position in ErroneousElement
instead?

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

https://chromiumcodereview.appspot.com/10920089/diff/6004/lib/compiler/implem...
lib/compiler/implementation/elements/elements.dart:299: static bool
isErroneousElement(Element e) => e != null && e.isErroneous();
Why aren't these methods on Element*s* class?

https://chromiumcodereview.appspot.com/10920089/diff/6004/lib/compiler/implem...
File lib/compiler/implementation/ssa/builder.dart (right):

https://chromiumcodereview.appspot.com/10920089/diff/6004/lib/compiler/implem...
lib/compiler/implementation/ssa/builder.dart:1992: String
getTargetName(ErroneousElement error, [String prefix]) {
How about

String getTargetName(error, [String prefix = '']) =>
'$prefix${error.targetName.slowToString}'

? Shorter :)

[karlklose, 2012-09-07 09:24:01]

https://chromiumcodereview.appspot.com/10920089/diff/10001/lib/compiler/imple...
File lib/compiler/implementation/elements/elements.dart (right):

https://chromiumcodereview.appspot.com/10920089/diff/10001/lib/compiler/imple...
lib/compiler/implementation/elements/elements.dart:334: getLibrary() =>
enclosingElement.getLibrary();
It does not check for the kind of itself, because that is undefined. Instead, it
looks at the enclosing.

https://chromiumcodereview.appspot.com/10920089/diff/6004/lib/compiler/implem...
File lib/compiler/implementation/compiler.dart (right):

https://chromiumcodereview.appspot.com/10920089/diff/6004/lib/compiler/implem...
lib/compiler/implementation/compiler.dart:827: if
(Element.isErroneousElement(element)) {
I wanted it to be similar to the case for the other synthetic elements (just
below), because there is no real position associated with this element.

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

https://chromiumcodereview.appspot.com/10920089/diff/6004/lib/compiler/implem...
lib/compiler/implementation/elements/elements.dart:299: static bool
isErroneousElement(Element e) => e != null && e.isErroneous();
Done, moved. I thought it reads better, but it is more consistent to have them
on Elements.

https://chromiumcodereview.appspot.com/10920089/diff/6004/lib/compiler/implem...
File lib/compiler/implementation/ssa/builder.dart (right):

https://chromiumcodereview.appspot.com/10920089/diff/6004/lib/compiler/implem...
lib/compiler/implementation/ssa/builder.dart:1992: String
getTargetName(ErroneousElement error, [String prefix]) {
On 2012/09/07 08:12:46, ngeoffray wrote:
> How about
> 
> String getTargetName(error, [String prefix = '']) =>
> '$prefix${error.targetName.slowToString}'
> 
> ? Shorter :)

Done.

[ahe, 2012-09-07 12:18:53]

LGTM!

https://chromiumcodereview.appspot.com/10920089/diff/6004/lib/compiler/implem...
File lib/compiler/implementation/compiler.dart (right):

https://chromiumcodereview.appspot.com/10920089/diff/6004/lib/compiler/implem...
lib/compiler/implementation/compiler.dart:827: if
(Element.isErroneousElement(element)) {
I don't think you're supposed to call spanFromElement if it is erroneous. If the
element is erroneous, there's no need to report an error.

https://chromiumcodereview.appspot.com/10920089/diff/6004/lib/compiler/implem...
File lib/compiler/implementation/resolved_visitor.dart (right):

https://chromiumcodereview.appspot.com/10920089/diff/6004/lib/compiler/implem...
lib/compiler/implementation/resolved_visitor.dart:21: if
(Element.isUnresolved(element)) {
If the element is unresolved, I think you should call visitUnresolved.

Also, I think this is clearer:

if (element === null) {
  return visitDynamicSend(node);
} else ...
....
} else if (Element.isUnresolved(element)) {
  return visitUnresolvedSend(node);
} else {
...

https://chromiumcodereview.appspot.com/10920089/diff/6004/tests/language/lang...
File tests/language/language_dart2js.status (right):

https://chromiumcodereview.appspot.com/10920089/diff/6004/tests/language/lang...
tests/language/language_dart2js.status:47: class_literal_test/03: Fail # Calling
a nonexistent static is not a compile time error anymore.
Fail, OK