Don't assign to final 'trace' parameter in catch clause.

Don't assign to final 'trace' parameter in catch clause.

BUG=https://github.com/dart-lang/sdk/issues/26295
BUG=https://github.com/dart-lang/pub/issues/1407
R=rnystrom@google.com

Committed: https://github.com/dart-lang/pub/commit/04ff0cc2cb6a3b698159b1aeef14cd3d7c90e287

[Bill Hesse, 2016-04-19 12:32:42]

whesse@google.com changed reviewers:
+ nweiz@google.com, rnystrom@google.com

[Bob Nystrom, 2016-04-19 15:52:19]

I ran into this the other day but I was in the middle of other stuff and didn't
look into it.

I don't understand what's going on here. Why is this parameter final? This code
has been this way for a long time. What changed to make it be an error?

[Bill Hesse, 2016-04-19 18:20:07]

On 2016/04/19 15:52:19, Bob Nystrom wrote:
> I ran into this the other day but I was in the middle of other stuff and
didn't
> look into it.
> 
> I don't understand what's going on here. Why is this parameter final? This
code
> has been this way for a long time. What changed to make it be an error?

I just looked at the spec, and saw they were final.  I don't know if that
changed, or the implementation did, or if we just never got this code before.  I
would think the analyzer should spot this. We should file a bug against the
analyzer.

[Bob Nystrom, 2016-04-19 19:01:11]

On 2016/04/19 18:20:07, Bill Hesse wrote:
> I just looked at the spec, and saw they were final.  I don't know if that
> changed, or the implementation did, or if we just never got this code before. 
I
> would think the analyzer should spot this. We should file a bug against the
> analyzer.

Are you talking about 17.11 "An on-catch clause of the form on T catch (p1, p2)
s introduces a new scope CS in which final local variables specified by p1 and
p2 are defined."?

That doesn't apply here. There is no catch clause, just closure being passed to
catchError().

[Bill Hesse, 2016-04-19 19:48:43]

On 2016/04/19 19:01:11, Bob Nystrom wrote:
> On 2016/04/19 18:20:07, Bill Hesse wrote:
> > I just looked at the spec, and saw they were final.  I don't know if that
> > changed, or the implementation did, or if we just never got this code
before. 
> I
> > would think the analyzer should spot this. We should file a bug against the
> > analyzer.
> 
> Are you talking about 17.11 "An on-catch clause of the form on T catch (p1,
p2)
> s introduces a new scope CS in which final local variables specified by p1 and
> p2 are defined."?
> 
> That doesn't apply here. There is no catch clause, just closure being passed
to
> catchError().

You are absolutely correct!  If the implementation has changed somehow, to make
this an error, then that change
needs to be reversed.  We could apply the CL, to fix the problem, but then we
should make a regression test,
that catches this requirement that isn't in the spec.

[Bill Hesse, 2016-04-19 19:57:22]

On 2016/04/19 19:48:43, Bill Hesse wrote:
> On 2016/04/19 19:01:11, Bob Nystrom wrote:
> > On 2016/04/19 18:20:07, Bill Hesse wrote:
> > > I just looked at the spec, and saw they were final.  I don't know if that
> > > changed, or the implementation did, or if we just never got this code
> before. 
> > I
> > > would think the analyzer should spot this. We should file a bug against
the
> > > analyzer.
> > 
> > Are you talking about 17.11 "An on-catch clause of the form on T catch (p1,
> p2)
> > s introduces a new scope CS in which final local variables specified by p1
and
> > p2 are defined."?
> > 
> > That doesn't apply here. There is no catch clause, just closure being passed
> to
> > catchError().
> 
> You are absolutely correct!  If the implementation has changed somehow, to
make
> this an error, then that change
> needs to be reversed.  We could apply the CL, to fix the problem, but then we
> should make a regression test,
> that catches this requirement that isn't in the spec.

Wait - I might have uploaded this against an outdated version of Pub.  The last
change to pub changes this .catchError closure to a try - catch, where the
problem actually exists.

Why don't you look at the most recent version of pub, at line 200 in this file,
and make a correct CL, since I really screwed this one up.
Or I'll fix this one, if you want, but it is a real mess.

[Bill Hesse, 2016-04-19 20:07:46]

OK, I have uploaded a rebased version of the CL, which applies the fix to the
current version of barback_server.dart, where one of the two catchError() calls
is replaced by an actual try - catch construct.

In this construct, trace is actually final, so we use a new variable for the
modified version.  I keep the change in the other block of code too, so that
they both work the same.  It is true that Chain implements StackTrace, so it is
okay to overwrite the variable in the case where it isn't final, though.

[Bob Nystrom, 2016-04-19 20:32:31]

On 2016/04/19 20:07:46, Bill Hesse wrote:
> OK, I have uploaded a rebased version of the CL, which applies the fix to the
> current version of barback_server.dart, where one of the two catchError()
calls
> is replaced by an actual try - catch construct.
> 
> In this construct, trace is actually final, so we use a new variable for the
> modified version.  I keep the change in the other block of code too, so that
> they both work the same.  It is true that Chain implements StackTrace, so it
is
> okay to overwrite the variable in the case where it isn't final, though.

Ah, I see what happened now. I broke this in
642addca734b05cb401e3ce656c6f5712adfd6d0.

I didn't realize I broke it because analyzer doesn't warn here like it should.
(Filed https://github.com/dart-lang/sdk/issues/26298 to track.)

Now your fix makes sense. :) For what it's worth, only the second example needs
to be changed since the first one is still using catchError(), but it doesn't
hurt to fix them both.

Thanks for looking into this!

– bob

[Bob Nystrom, 2016-04-19 20:32:44]

lgtm

[Bill Hesse, 2016-04-19 20:39:35]

Description was changed from

==========
Don't assign to final 'trace' parameter in catch clause.

BUG=https://github.com/dart-lang/sdk/issues/26295
BUG=https://github.com/dart-lang/pub/issues/1407
==========

to

==========
Don't assign to final 'trace' parameter in catch clause.

BUG=https://github.com/dart-lang/sdk/issues/26295
BUG=https://github.com/dart-lang/pub/issues/1407
R=rnystrom@google.com

Committed:
https://github.com/dart-lang/pub/commit/04ff0cc2cb6a3b698159b1aeef14cd3d7c90e287
==========

[Bill Hesse, 2016-04-19 20:39:37]

Committed patchset #2 (id:20001) manually as
04ff0cc2cb6a3b698159b1aeef14cd3d7c90e287 (presubmit successful).