Make some ErrorCode-s compile-time errors and some just type warnings

Make some ErrorCode-s compile-time errors and some just type warnings


R=zundel@google.com
BUG=
TEST=

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

[zundel, 2011-10-31 19:00:28]

http://codereview.chromium.org/8384012/diff/1/compiler/java/com/google/dart/c...
File compiler/java/com/google/dart/compiler/backend/js/RuntimeTypeInjector.java
(right):

http://codereview.chromium.org/8384012/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/backend/js/RuntimeTypeInjector.java:723:
TypeKind typeKind = TypeKind.of(type); // XXX
what is // XXX for?

http://codereview.chromium.org/8384012/diff/1/compiler/java/com/google/dart/c...
File compiler/java/com/google/dart/compiler/resolver/TypeErrorCode.java (right):

http://codereview.chromium.org/8384012/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/resolver/TypeErrorCode.java:30:
NO_SUCH_TYPE_GENERICS_ARGUMENT("no such type \"%s\" in type arguments"),
nit: could you add the name of the type at the end

no such type "foo" in type arguments for "bar"

http://codereview.chromium.org/8384012/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/resolver/TypeErrorCode.java:31:
NO_SUCH_TYPE_PARAMETER("no such type \"%s\" in method parameter"),
nit: again, could you add the name of the method parameter?  I won't keep
asking, just look at these two and add them if its easy to do.  The reason I ask
is that it might be helpful to test error messages.

http://codereview.chromium.org/8384012/diff/1/tests/stub-generator/src/MintMa...
File tests/stub-generator/src/MintMakerFullyIsolatedTest-generatedTest.dart
(right):

http://codereview.chromium.org/8384012/diff/1/tests/stub-generator/src/MintMa...
tests/stub-generator/src/MintMakerFullyIsolatedTest-generatedTest.dart:7:
#import("../../isolate/src/TestFramework.dart");
you need to fully revert this file before committing.

http://codereview.chromium.org/8384012/diff/1/tests/stub-generator/src/MintMa...
File tests/stub-generator/src/MintMakerPromiseWithStubsTest-generatedTest.dart
(right):

http://codereview.chromium.org/8384012/diff/1/tests/stub-generator/src/MintMa...
tests/stub-generator/src/MintMakerPromiseWithStubsTest-generatedTest.dart:8: 
you need to fully revert this file before committing.

[scheglov, 2011-10-31 20:48:05]

http://codereview.chromium.org/8384012/diff/1/compiler/java/com/google/dart/c...
File compiler/java/com/google/dart/compiler/resolver/TypeErrorCode.java (right):

http://codereview.chromium.org/8384012/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/resolver/TypeErrorCode.java:31:
NO_SUCH_TYPE_PARAMETER("no such type \"%s\" in method parameter"),
On 2011/10/31 19:00:28, zundel wrote:
> nit: again, could you add the name of the method parameter?  I won't keep
> asking, just look at these two and add them if its easy to do.  The reason I
ask
> is that it might be helpful to test error messages.

  I think now that may be having specific NO_SUCH_TYPE_* is mistake.
  For sure we need separate ErrorCode-s for warning and error, but not
return/parameter/variable/etc. This does not happen for Java in Eclipse. This is
not required at all for IDE - you just see where is problem. And more words in
error message is worse, not better. As of tests, I think that line number is
enough narrow location of problem.

[zundel, 2011-11-01 13:54:37]

http://codereview.chromium.org/8384012/diff/1/compiler/java/com/google/dart/c...
File compiler/java/com/google/dart/compiler/resolver/ResolutionContext.java
(right):

http://codereview.chromium.org/8384012/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/resolver/ResolutionContext.java:202: Type
type = resolveType(typeNode, isStatic,
TypeErrorCode.NO_SUCH_TYPE_GENERICS_ARGUMENT);
Gilad clarified that instantiating a type requires resolved type parameters, so
this should be a Resolver error.

http://codereview.chromium.org/8384012/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/resolver/ResolutionContext.java:217:
TypeErrorCode.NO_SUCH_TYPE_GENERICS_ARGUMENT);
ditto.

http://codereview.chromium.org/8384012/diff/1/compiler/java/com/google/dart/c...
File compiler/java/com/google/dart/compiler/resolver/TypeErrorCode.java (right):

http://codereview.chromium.org/8384012/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/resolver/TypeErrorCode.java:31:
NO_SUCH_TYPE_PARAMETER("no such type \"%s\" in method parameter"),
On 2011/10/31 20:48:06, scheglov wrote:
> On 2011/10/31 19:00:28, zundel wrote:
> > nit: again, could you add the name of the method parameter?  I won't keep
> > asking, just look at these two and add them if its easy to do.  The reason I
> ask
> > is that it might be helpful to test error messages.
> 
>   I think now that may be having specific NO_SUCH_TYPE_* is mistake.
>   For sure we need separate ErrorCode-s for warning and error, but not
> return/parameter/variable/etc. This does not happen for Java in Eclipse. This
is
> not required at all for IDE - you just see where is problem. And more words in
> error message is worse, not better. As of tests, I think that line number is
> enough narrow location of problem.

As a counter point, if you look at NO_SUCH_TYPE_GENERICS_ARGUMENT error message
when it is printed out from the compiler, the error is highlighted on the
generic type, not the type parameter. We should probably fix that, but such
terse errors have confounded me many a time.  

I think the more precise errors are of benefit to the command line compiler,
especially for unit testing, and in at least some cases in the IDE, especially
if we expanded them still further to include the name of the entity they
reference.  Looking at the structure of the code, I see that is kind of
difficult, so you can ignore these comments.

http://codereview.chromium.org/8384012/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/resolver/TypeErrorCode.java:32:
NO_SUCH_TYPE_RETURN("no such type \"%s\" in method return"),
I agree this one is kind of silly.

http://codereview.chromium.org/8384012/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/resolver/TypeErrorCode.java:33:
NO_SUCH_TYPE_VARIABLE_STATEMENT("no such type \"%s\" in variable declaration
statement"),
This one might be too.

[scheglov, 2011-11-01 15:08:57]

On 2011/11/01 13:54:37, zundel wrote:
>
http://codereview.chromium.org/8384012/diff/1/compiler/java/com/google/dart/c...
> File compiler/java/com/google/dart/compiler/resolver/ResolutionContext.java
> (right):
> 
>
http://codereview.chromium.org/8384012/diff/1/compiler/java/com/google/dart/c...
> compiler/java/com/google/dart/compiler/resolver/ResolutionContext.java:202:
Type
> type = resolveType(typeNode, isStatic,
> TypeErrorCode.NO_SUCH_TYPE_GENERICS_ARGUMENT);
> Gilad clarified that instantiating a type requires resolved type parameters,
so
> this should be a Resolver error.
> 
>
http://codereview.chromium.org/8384012/diff/1/compiler/java/com/google/dart/c...
> compiler/java/com/google/dart/compiler/resolver/ResolutionContext.java:217:
> TypeErrorCode.NO_SUCH_TYPE_GENERICS_ARGUMENT);
> ditto.

  Yes, I'm working on new JUnit tests and changes for these cases - separate for
variable declaration warnings and "new" expression compile-time errors.

 
> As a counter point, if you look at NO_SUCH_TYPE_GENERICS_ARGUMENT error
message
> when it is printed out from the compiler, the error is highlighted on the
> generic type, not the type parameter. We should probably fix that, but such
> terse errors have confounded me many a time.

  I've added check in tests to ensure that location of error is reported
correctly for type argument.
  Actually what I observe is that it works correctly now, so probably someone
already fixed this.



> I think the more precise errors are of benefit to the command line compiler,
> especially for unit testing, and in at least some cases in the IDE, especially
> if we expanded them still further to include the name of the entity they
> reference.  Looking at the structure of the code, I see that is kind of
> difficult, so you can ignore these comments.

  Yes.
  If we will go this way to the end, we may be will have error report like "No
such type T in 1-st type argument of Baz of 2-nd type argument of Bar in extends
of class Foo".
  I've changed now it to use single NO_SUCH_TYPE constant of resolve or type
kind, and it is propagated to resolving type arguments, type
arguments-arguments, etc.

[zundel, 2011-11-01 20:03:03]

http://codereview.chromium.org/8384012/diff/7001/compiler/java/com/google/dar...
File compiler/java/com/google/dart/compiler/backend/js/RuntimeTypeInjector.java
(right):

http://codereview.chromium.org/8384012/diff/7001/compiler/java/com/google/dar...
compiler/java/com/google/dart/compiler/backend/js/RuntimeTypeInjector.java:30:
import static com.google.dart.compiler.util.AstUtil.and;
I am assuming the changes in this file are due to another change you got during
a sync up to master.

http://codereview.chromium.org/8384012/diff/7001/tests/language/src/WrongNumb...
File tests/language/src/WrongNumberTypeArgumentsNegativeTest.dart (left):

http://codereview.chromium.org/8384012/diff/7001/tests/language/src/WrongNumb...
tests/language/src/WrongNumberTypeArgumentsNegativeTest.dart:5: Map<String> foo;
 // Error: Map takes 2 type arguments.
I think the right thing to do is to make this a positive test, given the
clarification by Gilad, and then convert this one to be an example where the
wrong number of argument is a compile-time error, say 'new Foo<param1, param2>'
where Foo is only declared to take one type variable.

http://codereview.chromium.org/8384012/diff/7001/tests/stub-generator/src/Min...
File tests/stub-generator/src/MintMakerFullyIsolatedTest-generatedTest.dart
(left):

http://codereview.chromium.org/8384012/diff/7001/tests/stub-generator/src/Min...
tests/stub-generator/src/MintMakerFullyIsolatedTest-generatedTest.dart:9: /*
class = Purse
(tests/stub-generator/src/MintMakerFullyIsolatedTest.dart/MintMakerFullyIsolatedTest.dart:
9) */
revert before committing if you have changes in your client.

http://codereview.chromium.org/8384012/diff/7001/tests/stub-generator/src/Min...
File tests/stub-generator/src/MintMakerPromiseWithStubsTest-generatedTest.dart
(left):

http://codereview.chromium.org/8384012/diff/7001/tests/stub-generator/src/Min...
tests/stub-generator/src/MintMakerPromiseWithStubsTest-generatedTest.dart:9: /*
class = Mint
(tests/stub-generator/src/MintMakerPromiseWithStubsTest.dart/MintMakerPromiseWithStubsTest.dart:
9) */
revert before committing if you have changes in your client.

http://codereview.chromium.org/8384012/diff/7001/tests/stub-generator/stub-ge...
File tests/stub-generator/stub-generator.status (right):

http://codereview.chromium.org/8384012/diff/7001/tests/stub-generator/stub-ge...
tests/stub-generator/stub-generator.status:10: MintMakerPromiseWithStubsTest :
Skip    # Bug 5365165
I'm wondering if we really need to commit the 'Skip' lines - the build bot is
building fine without them.

[scheglov, 2011-11-01 20:25:59]

http://codereview.chromium.org/8384012/diff/7001/compiler/java/com/google/dar...
File compiler/java/com/google/dart/compiler/backend/js/RuntimeTypeInjector.java
(right):

http://codereview.chromium.org/8384012/diff/7001/compiler/java/com/google/dar...
compiler/java/com/google/dart/compiler/backend/js/RuntimeTypeInjector.java:30:
import static com.google.dart.compiler.util.AstUtil.and;
I've touched this file, but then rolled back my changes.
However not this removing unused variable.
I will rollback fully.

http://codereview.chromium.org/8384012/diff/7001/tests/language/src/WrongNumb...
File tests/language/src/WrongNumberTypeArgumentsNegativeTest.dart (left):

http://codereview.chromium.org/8384012/diff/7001/tests/language/src/WrongNumb...
tests/language/src/WrongNumberTypeArgumentsNegativeTest.dart:5: Map<String> foo;
 // Error: Map takes 2 type arguments.
On 2011/11/01 20:03:03, zundel wrote:
> I think the right thing to do is to make this a positive test, given the
> clarification by Gilad, and then convert this one to be an example where the
> wrong number of argument is a compile-time error, say 'new Foo<param1,
param2>'
> where Foo is only declared to take one type variable.

Done.

http://codereview.chromium.org/8384012/diff/7001/tests/stub-generator/stub-ge...
File tests/stub-generator/stub-generator.status (right):

http://codereview.chromium.org/8384012/diff/7001/tests/stub-generator/stub-ge...
tests/stub-generator/stub-generator.status:10: MintMakerPromiseWithStubsTest :
Skip    # Bug 5365165
On 2011/11/01 20:03:03, zundel wrote:
> I'm wondering if we really need to commit the 'Skip' lines - the build bot is
> building fine without them.

When I run tests I see obvious failures:

file:/usr/local/google/users/scheglov/Clients/Dart/dart/tests/stub-generator/src/MintMakerPromiseWithStubsTest-generatedTest.dart/MintMakerPromiseWithStubsTest-generatedTest.dart:81:
no such type "Mint$ProxyImpl"
    80:   static void testMain(TestExpectation expect) {
    81:     Mint$Proxy mint = new Mint$ProxyImpl.createIsolate();
                                  ~~~~~~~~~~~~~~

[zundel, 2011-11-01 20:40:05]

LGTM

[Ben Laurie (Google), 2011-11-02 18:08:34]

http://codereview.chromium.org/8384012/diff/11004/tests/stub-generator/stub-g...
File tests/stub-generator/stub-generator.status (right):

http://codereview.chromium.org/8384012/diff/11004/tests/stub-generator/stub-g...
tests/stub-generator/stub-generator.status:11: MintMakerFullyIsolatedTest : Skip
      # Bug 5365165
The problem here is not bug 5365165 (which is fixed, anyway), but that you have
broken the stub generator by making unresolved types errors: these have to be
warnings when shouldWarnOnNoSuchType() is true.

[zundel, 2011-11-02 18:16:58]

http://codereview.chromium.org/8384012/diff/11004/tests/stub-generator/stub-g...
File tests/stub-generator/stub-generator.status (right):

http://codereview.chromium.org/8384012/diff/11004/tests/stub-generator/stub-g...
tests/stub-generator/stub-generator.status:11: MintMakerFullyIsolatedTest : Skip
      # Bug 5365165
On 2011/11/02 18:08:34, Ben Laurie (Google) wrote:
> The problem here is not bug 5365165 (which is fixed, anyway), but that you
have
> broken the stub generator by making unresolved types errors: these have to be
> warnings when shouldWarnOnNoSuchType() is true.

Hi Ben,

I know it is frustrating to see a test that has been working for so long go red.

What's happened is we've gotten rid of the 'warnOnNoSuchType' feature in dartc
intentionally, as there is no such advertised feature in Dart.  We've also
synced up with the spec.  After syncing up, there are some cases that have
changed. Before it was always an error.  Now we've gone through and made it a
warning in cases where it is supposed to be a warning, and an error in those
cases where it is defined to be a compile time error, clarified with Gilad's
help.  

In the case of what are now compile time error cases, AFAIK, there is no way to
make proper functioning code that is Dart spec compliant - if you find a counter
example in the spec, let us know or file a bug.

[zundel, 2011-11-02 18:27:28]

On 2011/11/02 18:16:58, zundel wrote:
>
http://codereview.chromium.org/8384012/diff/11004/tests/stub-generator/stub-g...
> File tests/stub-generator/stub-generator.status (right):
> 
>
http://codereview.chromium.org/8384012/diff/11004/tests/stub-generator/stub-g...
> tests/stub-generator/stub-generator.status:11: MintMakerFullyIsolatedTest :
Skip
>       # Bug 5365165
> On 2011/11/02 18:08:34, Ben Laurie (Google) wrote:
> > The problem here is not bug 5365165 (which is fixed, anyway), but that you
> have
> > broken the stub generator by making unresolved types errors: these have to
be
> > warnings when shouldWarnOnNoSuchType() is true.
> 
> Hi Ben,
> 
> I know it is frustrating to see a test that has been working for so long go
red.
> 
> What's happened is we've gotten rid of the 'warnOnNoSuchType' feature in dartc
> intentionally, as there is no such advertised feature in Dart.  We've also
> synced up with the spec.  After syncing up, there are some cases that have
> changed. Before it was always an error.  Now we've gone through and made it a
> warning in cases where it is supposed to be a warning, and an error in those
> cases where it is defined to be a compile time error, clarified with Gilad's
> help.  
> 
> In the case of what are now compile time error cases, AFAIK, there is no way
to
> make proper functioning code that is Dart spec compliant - if you find a
counter
> example in the spec, let us know or file a bug.

Looks like I was wrong, there are still a couple of references to the
warnOnNoSuch type which were unintentionally left in place.

[Ben Laurie (Google), 2011-11-02 18:48:30]

The problem is the stub generator doesn't take Dart-compliant code, it takes
nearly-Dart compliant code with unresolved types and produces code that makes
the original code compliant.

So, you can't completely kill warnOnNoSuchType without breaking the stub
generator.

Of course, you can set it to false for standard compiles.