Give correct warnings/errors on type expressions.

Give correct warnings/errors on type expressions.


BUG=http://dartbug.com/5029

[ahe, 2013-02-05 08:39:52]

I think I understand what is going on, but I need to more carefully study the
changes in members.dart. However, I noticed something that I thought I'd mention
immediately.

https://codereview.chromium.org/12210010/diff/1/tests/co19/co19-dart2js.status
File tests/co19/co19-dart2js.status (right):

https://codereview.chromium.org/12210010/diff/1/tests/co19/co19-dart2js.statu...
tests/co19/co19-dart2js.status:113: Language/12_Statements/10_Try_A03_t01: Crash
# Issue 8325
Please do not regress on crashes, it is never in our users' interest to have the
compiler crashing.

[Johnni Winther, 2013-02-05 08:46:12]

https://codereview.chromium.org/12210010/diff/1/tests/co19/co19-dart2js.status
File tests/co19/co19-dart2js.status (right):

https://codereview.chromium.org/12210010/diff/1/tests/co19/co19-dart2js.statu...
tests/co19/co19-dart2js.status:113: Language/12_Statements/10_Try_A03_t01: Crash
# Issue 8325
On 2013/02/05 08:39:52, ahe wrote:
> Please do not regress on crashes, it is never in our users' interest to have
the
> compiler crashing.

I know. Should I use Skip instead ;)

The problem is that handling unresolved types correctly as a warning propagates
the malformed type to code that does not handle this. I know you're not in favor
for fixing this in the current CL but it cannot be done before (since code is
not exercised) and, as it seems, not after since I should not use Crash.

[ahe, 2013-02-05 08:58:55]

On 2013/02/05 08:46:12, Johnni Winther wrote:
> https://codereview.chromium.org/12210010/diff/1/tests/co19/co19-dart2js.status
> File tests/co19/co19-dart2js.status (right):
> 
>
https://codereview.chromium.org/12210010/diff/1/tests/co19/co19-dart2js.statu...
> tests/co19/co19-dart2js.status:113: Language/12_Statements/10_Try_A03_t01:
Crash
> # Issue 8325
> On 2013/02/05 08:39:52, ahe wrote:
> > Please do not regress on crashes, it is never in our users' interest to have
> the
> > compiler crashing.
> 
> I know. Should I use Skip instead ;)
> 
> The problem is that handling unresolved types correctly as a warning
propagates
> the malformed type to code that does not handle this. I know you're not in
favor
> for fixing this in the current CL but it cannot be done before (since code is
> not exercised) and, as it seems, not after since I should not use Crash.

In my opinion, crashes takes precedence over everything else. I'm sure the code
that crashes can be changed to something like:

if (type.isMalformed) {
  compiler.internalError('Malformed type not yet supported in this context',
node: node);
}

This displays a fairly low-key error message to the user. On the other hand, if
the compiler crashes, we beg the user to report a bug an turn it into a critical
bug.

[Johnni Winther, 2013-02-05 09:02:20]

On 2013/02/05 08:58:55, ahe wrote:
> On 2013/02/05 08:46:12, Johnni Winther wrote:
> >
https://codereview.chromium.org/12210010/diff/1/tests/co19/co19-dart2js.status
> > File tests/co19/co19-dart2js.status (right):
> > 
> >
>
https://codereview.chromium.org/12210010/diff/1/tests/co19/co19-dart2js.statu...
> > tests/co19/co19-dart2js.status:113: Language/12_Statements/10_Try_A03_t01:
> Crash
> > # Issue 8325
> > On 2013/02/05 08:39:52, ahe wrote:
> > > Please do not regress on crashes, it is never in our users' interest to
have
> > the
> > > compiler crashing.
> > 
> > I know. Should I use Skip instead ;)
> > 
> > The problem is that handling unresolved types correctly as a warning
> propagates
> > the malformed type to code that does not handle this. I know you're not in
> favor
> > for fixing this in the current CL but it cannot be done before (since code
is
> > not exercised) and, as it seems, not after since I should not use Crash.
> 
> In my opinion, crashes takes precedence over everything else. I'm sure the
code
> that crashes can be changed to something like:
> 
> if (type.isMalformed) {
>   compiler.internalError('Malformed type not yet supported in this context',
> node: node);
> }
> 
> This displays a fairly low-key error message to the user. On the other hand,
if
> the compiler crashes, we beg the user to report a bug an turn it into a
critical
> bug.

Yes, the fix is probably easy, but I don't think is should get in the way of
this more important CL.

[ahe, 2013-02-05 09:06:16]

On 2013/02/05 09:02:20, Johnni Winther wrote:
> Yes, the fix is probably easy, but I don't think is should get in the way of
> this more important CL.

This CL fixes bug 5029 which is marked "Priority-Medium".

When you introduce a crash, you create a bug that is "Priority-Critical". That
is a step backwards, not a step forward. You need to take crashes seriously, our
users deserve that.

[ahe, 2013-02-05 11:26:28]

I have written a rather long explanation of why you shouldn't change for loops
to for-in in CL 12087134.

Otherwise, LGTM

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

https://codereview.chromium.org/12210010/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/resolution/members.dart:1350:
this.isTypeExpression = true;
I don't think it is safe to call resolveTypeExpression from
resolveTypeAnnotation and vice versa. I cannot convince myself that is good. So
it would ease my mind if you could store the old value and restore it after the
call.

But no need to use try/finally.

https://codereview.chromium.org/12210010/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/resolution/members.dart:1387: bool
hasTypeArgumentMismatch = resolveTypeArguments(
The old name was funnier ;-)

https://codereview.chromium.org/12210010/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/resolution/members.dart:1487: for
(Node typeArgument in node.typeArguments.nodes) {
Please avoid for-in.

https://codereview.chromium.org/12210010/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/resolution/members.dart:3074: for
(TypeAnnotation interface in interfaces.nodes){
Please avoid for-in.