Clean up error handling.

Clean up error handling.

BUG=http://dartbug.com/12039
R=kustermann@google.com

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

[kustermann, 2013-07-27 09:50:29]

lgtm

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

https://codereview.chromium.org/20742002/diff/3001/dart/sdk/lib/_internal/com...
dart/sdk/lib/_internal/compiler/implementation/resolution/members.dart:647:
{'className': mixin.name});
Indentation.

https://codereview.chromium.org/20742002/diff/3001/dart/sdk/lib/_internal/com...
File dart/sdk/lib/_internal/compiler/implementation/warnings.dart (right):

https://codereview.chromium.org/20742002/diff/3001/dart/sdk/lib/_internal/com...
dart/sdk/lib/_internal/compiler/implementation/warnings.dart:461: 'Error: Cannot
use class "#{className}" as a mixin because it uses super.');
Move it to the end of the previous line.

https://codereview.chromium.org/20742002/diff/3001/dart/tests/compiler/dart2j...
File dart/tests/compiler/dart2js/compiler_test.dart (right):

https://codereview.chromium.org/20742002/diff/3001/dart/tests/compiler/dart2j...
dart/tests/compiler/dart2js/compiler_test.dart:43: compiler.setOnWarning((c, n,
m) => Expect.equals(foo, compiler.currentElement));
long line

https://codereview.chromium.org/20742002/diff/3001/dart/tests/compiler/dart2j...
File dart/tests/compiler/dart2js/private_test.dart (right):

https://codereview.chromium.org/20742002/diff/3001/dart/tests/compiler/dart2j...
dart/tests/compiler/dart2js/private_test.dart:88: analyze('new
_PrivateClass.publicConstructor();', MessageKind.CANNOT_RESOLVE.warning);
long line

[ahe, 2013-07-28 09:05:51]

Thank you, Martin.

Before submitting, I added a number of guide lines for error messages to
warning.dart. Would you mind proof-reading them at your convenience?

Cheers,
Peter

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

https://codereview.chromium.org/20742002/diff/3001/dart/sdk/lib/_internal/com...
dart/sdk/lib/_internal/compiler/implementation/resolution/members.dart:647:
{'className': mixin.name});
On 2013/07/27 09:50:29, kustermann wrote:
> Indentation.

Done.

https://codereview.chromium.org/20742002/diff/3001/dart/sdk/lib/_internal/com...
File dart/sdk/lib/_internal/compiler/implementation/warnings.dart (right):

https://codereview.chromium.org/20742002/diff/3001/dart/sdk/lib/_internal/com...
dart/sdk/lib/_internal/compiler/implementation/warnings.dart:461: 'Error: Cannot
use class "#{className}" as a mixin because it uses super.');
On 2013/07/27 09:50:29, kustermann wrote:
> Move it to the end of the previous line.

Done.

https://codereview.chromium.org/20742002/diff/3001/dart/tests/compiler/dart2j...
File dart/tests/compiler/dart2js/compiler_test.dart (right):

https://codereview.chromium.org/20742002/diff/3001/dart/tests/compiler/dart2j...
dart/tests/compiler/dart2js/compiler_test.dart:43: compiler.setOnWarning((c, n,
m) => Expect.equals(foo, compiler.currentElement));
On 2013/07/27 09:50:29, kustermann wrote:
> long line

Done.

https://codereview.chromium.org/20742002/diff/3001/dart/tests/compiler/dart2j...
File dart/tests/compiler/dart2js/private_test.dart (right):

https://codereview.chromium.org/20742002/diff/3001/dart/tests/compiler/dart2j...
dart/tests/compiler/dart2js/private_test.dart:88: analyze('new
_PrivateClass.publicConstructor();', MessageKind.CANNOT_RESOLVE.warning);
On 2013/07/27 09:50:29, kustermann wrote:
> long line

Done.

[ahe, 2013-07-28 09:07:22]

Committed patchset #4 manually as r25546 (presubmit successful).

[ahe, 2013-07-28 09:19:36]

https://codereview.chromium.org/20742002/diff/17001/dart/sdk/lib/_internal/co...
File dart/sdk/lib/_internal/compiler/implementation/warnings.dart (right):

https://codereview.chromium.org/20742002/diff/17001/dart/sdk/lib/_internal/co...
dart/sdk/lib/_internal/compiler/implementation/warnings.dart:14: * other
languages than English, and that it is therefor not possible to
therefore

https://codereview.chromium.org/20742002/diff/17001/dart/sdk/lib/_internal/co...
dart/sdk/lib/_internal/compiler/implementation/warnings.dart:36: * "iff", that
level of precision is unnecessary.
is *often* unnecessary.

[Johnni Winther, 2013-07-29 06:22:41]

lgtm

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

https://codereview.chromium.org/20742002/diff/17001/dart/sdk/lib/_internal/co...
dart/sdk/lib/_internal/compiler/implementation/resolution/members.dart:1260:
'internal error: Unhandled node: ${node.getObjectDescription()}');
'internal error' -> 'Internal Error'.

https://codereview.chromium.org/20742002/diff/17001/dart/sdk/lib/_internal/co...
File dart/sdk/lib/_internal/compiler/implementation/warnings.dart (right):

https://codereview.chromium.org/20742002/diff/17001/dart/sdk/lib/_internal/co...
dart/sdk/lib/_internal/compiler/implementation/warnings.dart:10: * 1. The
message should start with exactly one of "Error", "Internal error",
"Internal error" -> "Internal Error".

https://codereview.chromium.org/20742002/diff/17001/dart/sdk/lib/_internal/co...
dart/sdk/lib/_internal/compiler/implementation/warnings.dart:51: * lie. Prefer
messages like: 'Internal error: This function should not be
'Internal error' -> 'Internal Error'.

[kustermann, 2013-07-29 06:26:40]

It's a good idea to define rules for the messages.

https://codereview.chromium.org/20742002/diff/17001/dart/sdk/lib/_internal/co...
File dart/sdk/lib/_internal/compiler/implementation/warnings.dart (right):

https://codereview.chromium.org/20742002/diff/17001/dart/sdk/lib/_internal/co...
dart/sdk/lib/_internal/compiler/implementation/warnings.dart:27: * 4. Do not try
to compose messages, as it can make translating them hard.
I don't get this rule.

https://codereview.chromium.org/20742002/diff/17001/dart/sdk/lib/_internal/co...
dart/sdk/lib/_internal/compiler/implementation/warnings.dart:39: * is again to
benefit readers to whom English is not their first language.
You could simplify to something like: "This is again beneficial for non-English
users.

https://codereview.chromium.org/20742002/diff/17001/dart/sdk/lib/_internal/co...
dart/sdk/lib/_internal/compiler/implementation/warnings.dart:53: */
Since the rules above apply to the whole file => new line.

[ahe, 2013-07-30 14:31:35]

Feedback from Luke.

https://codereview.chromium.org/20742002/diff/17001/dart/sdk/lib/_internal/co...
File dart/sdk/lib/_internal/compiler/implementation/warnings.dart (right):

https://codereview.chromium.org/20742002/diff/17001/dart/sdk/lib/_internal/co...
dart/sdk/lib/_internal/compiler/implementation/warnings.dart:22: * 'Error: The
class "#{className}" cannot use "super".' Notice that the word
Luke:

Using "cannot", "must not", "shall not", etc. is off-putting to people new to
programming.
Prefer contractions when they are in common use.

General rule: three sentences: 1. what is wrong, 2. why is it wrong, 3. how do I
fix it.

The class "#{className}" can't use "super".
"#{className}" doesn't have a superclass.
Try removing "super".

Prefer not imperative tone.

The computer should describe the problem, not criticize for violating the
specification.

Message {
  String errorBecauseExplanation;
  String howDoIFixIt;
  var uniqueIdentifier;
  List examples = [...];
}

  enum kind = {error, warning, hint, ...};
position

emitMessage(kind, position, message, arguments)

[sra1, 2013-07-30 21:28:19]

https://codereview.chromium.org/20742002/diff/17001/dart/sdk/lib/_internal/co...
File dart/sdk/lib/_internal/compiler/implementation/warnings.dart (right):

https://codereview.chromium.org/20742002/diff/17001/dart/sdk/lib/_internal/co...
dart/sdk/lib/_internal/compiler/implementation/warnings.dart:20: * 3. Reserved
words and embedded identifiers should be in quotes (double
We should be using single quotes.

1. Double quotes are inconsistent with the VM.  Example (an introduced error):

../../sdk/lib/collection/iterable.dart:20:25: expected ')', but got 'element'
  bool contains(Ob ject element) {

2. It is also inconsistent with javac, gcc, Python, Microsoft's compilers.

(I also think double quotes look peculiar - probably because of years of
experience with all the successful language implementations that use single
quotes.)

3. Single quotes are also a better choice when the whole message needs to be
quoted, example, google.com gives the same result for
  red wine
  'red wine'
but a different result for
  "red wine"
because double quotes are understood to be a search phrase.

A diagnostic like
   Cannot subclass 'int'
can be quoted as a phrase ("Cannot sublcass 'int'") but searching for
   Cannot sublcass "int"
(via "Cannot sublcass "int"" or 'Cannot subclass "int"') will be less
successful.

[ahe, 2013-07-31 15:58:38]

I'm currently adding some new diagnostics in a CL.

I'll circulate that CL a bit wider than normally so we can discuss how the
messages look and then I'll update the guide lines.

https://codereview.chromium.org/20742002/diff/17001/dart/sdk/lib/_internal/co...
File dart/sdk/lib/_internal/compiler/implementation/warnings.dart (right):

https://codereview.chromium.org/20742002/diff/17001/dart/sdk/lib/_internal/co...
dart/sdk/lib/_internal/compiler/implementation/warnings.dart:20: * 3. Reserved
words and embedded identifiers should be in quotes (double
On 2013/07/30 21:28:20, sra1 wrote:
> We should be using single quotes.
...
> 3. Single quotes are also a better choice when the whole message needs to be
> quoted, example, http://google.com gives the same result for

You convinced me.