Fix newly introduced errors and warnings

Fix newly introduced errors and warnings

R=scheglov@google.com

Committed: https://github.com/dart-lang/sdk/commit/2d9e690e6c4f8c7eba9ee52f43c816fac17823e1

[Brian Wilkerson, 2016-09-20 23:36:12]

brianwilkerson@google.com changed reviewers:
+ scheglov@google.com

[scheglov, 2016-09-21 00:05:07]

lgtm :-(

https://codereview.chromium.org/2357553003/diff/1/pkg/analyzer/lib/src/summar...
File pkg/analyzer/lib/src/summary/package_bundle_reader.dart (right):

https://codereview.chromium.org/2357553003/diff/1/pkg/analyzer/lib/src/summar...
pkg/analyzer/lib/src/summary/package_bundle_reader.dart:185: result as
ResultDescriptor<bool>, true, TargetedResult.EMPTY_LIST);
Ouch :-(

https://codereview.chromium.org/2357553003/diff/1/pkg/analyzer/test/src/conte...
File pkg/analyzer/test/src/context/cache_test.dart (right):

https://codereview.chromium.org/2357553003/diff/1/pkg/analyzer/test/src/conte...
pkg/analyzer/test/src/context/cache_test.dart:61: ResultDescriptor<String>
resultA = new ResultDescriptor<String>('A', null);
I would just drop the LHS type.

[Jennifer Messerly, 2016-09-21 00:20:29]

jmesserly@google.com changed reviewers:
+ jmesserly@google.com

[Jennifer Messerly, 2016-09-21 00:20:30]

Hey sorry we made a ":-(" ... feel free to suggest other behavior if you think
we should do something different.

So far much of the inference changes have been driven by user feedback.
Sometimes it has unintended consequences though.

I'm not sure how the CC is intended. If there's a way I can catch these before
submitting inference changes, I'll try and adjust my presubmit process. Let me
know.

If you wanted to give feedback about the inference, definitely CC Leaf on that.
Bugs are welcome too if something doesn't feel right. It's sometimes hard to
tell seeing only the resulting code what the error was.

https://codereview.chromium.org/2357553003/diff/1/pkg/analyzer/lib/src/summar...
File pkg/analyzer/lib/src/summary/package_bundle_reader.dart (right):

https://codereview.chromium.org/2357553003/diff/1/pkg/analyzer/lib/src/summar...
pkg/analyzer/lib/src/summary/package_bundle_reader.dart:185: result as
ResultDescriptor<bool>, true, TargetedResult.EMPTY_LIST);
On 2016/09/21 00:05:07, scheglov wrote:
> Ouch :-(

if it's easier, you could just pass <bool> to the generic method:

    entry.setValue<bool>(result, true, ...);

that said, there'd still be an implicit downcast for "result".

Another option would be to explicitly pass "dynamic"

    entry.setValue<dynamic>(result, true, ...);

I presume that's what the old code did.

But yeah, we lost some type info on ResultDescriptor so if we want to get back
to a typed world (and I'm not sure it's necessary, since setValue may not
actually care?) we'd need a cast.

https://codereview.chromium.org/2357553003/diff/1/pkg/analyzer/test/src/conte...
File pkg/analyzer/test/src/context/cache_test.dart (right):

https://codereview.chromium.org/2357553003/diff/1/pkg/analyzer/test/src/conte...
pkg/analyzer/test/src/context/cache_test.dart:61: ResultDescriptor<String>
resultA = new ResultDescriptor<String>('A', null);
On 2016/09/21 00:05:07, scheglov wrote:
> I would just drop the LHS type.

yeah +1 to that.

But I do wonder about this one. The old behavior would've inferred
ResultDescriptor<dynamic>. Does that not happen anymore?

[scheglov, 2016-09-21 00:32:37]

https://codereview.chromium.org/2357553003/diff/1/pkg/analyzer/lib/src/summar...
File pkg/analyzer/lib/src/summary/package_bundle_reader.dart (right):

https://codereview.chromium.org/2357553003/diff/1/pkg/analyzer/lib/src/summar...
pkg/analyzer/lib/src/summary/package_bundle_reader.dart:185: result as
ResultDescriptor<bool>, true, TargetedResult.EMPTY_LIST);
On 2016/09/21 00:20:29, John Messerly wrote:
> On 2016/09/21 00:05:07, scheglov wrote:
> > Ouch :-(
> 
> if it's easier, you could just pass <bool> to the generic method:
> 
>     entry.setValue<bool>(result, true, ...);
> 
> that said, there'd still be an implicit downcast for "result".
> 
> Another option would be to explicitly pass "dynamic"
> 
>     entry.setValue<dynamic>(result, true, ...);
> 
> I presume that's what the old code did.
> 
> But yeah, we lost some type info on ResultDescriptor so if we want to get back
> to a typed world (and I'm not sure it's necessary, since setValue may not
> actually care?) we'd need a cast.

Thank you for the long explanation.
I appreciate your doing this.

Yes, I agree that we need to have casts here. It just seems sad that we have to
do this even though we know for sure that it is equal to a fixed set of values,
each of which has the same type. But I guess it might be too confusing to infer
using ==.

[Brian Wilkerson, 2016-09-21 15:00:43]

> I'm not sure how the CC is intended.

I just want you to be aware of the changes that are required in our code in case
you see something that was unexpected or unintended.

> If there's a way I can catch these before submitting inference
> changes, I'll try and adjust my presubmit process. Let me know.

Well, you can always run the analyzer over the analyzer, analyzer_cli and
analysis_server packages. :-) They are currently error / warning free (though
there are several hints that we can't quite get rid of at this point that you'd
have to ignore).

You should also run it over ddc; there are several similar errors introduced in
the dev_compiler package that I didn't try to clean up.

> If you wanted to give feedback about the inference, definitely CC
> Leaf on that.

Yes, I should have done.

https://codereview.chromium.org/2357553003/diff/1/pkg/analyzer/test/src/conte...
File pkg/analyzer/test/src/context/cache_test.dart (right):

https://codereview.chromium.org/2357553003/diff/1/pkg/analyzer/test/src/conte...
pkg/analyzer/test/src/context/cache_test.dart:61: ResultDescriptor<String>
resultA = new ResultDescriptor<String>('A', null);
> I would just drop the LHS type.

I expect someone will write a bulk edit tool at some point that will remove type
annotations, so I'm not going to waste my time removing them by hand. :-)

[Brian Wilkerson, 2016-09-21 15:01:06]

Description was changed from

==========
Fix newly introduced errors and warnings
==========

to

==========
Fix newly introduced errors and warnings

R=scheglov@google.com

Committed:
https://github.com/dart-lang/sdk/commit/2d9e690e6c4f8c7eba9ee52f43c816fac17823e1
==========

[Brian Wilkerson, 2016-09-21 15:01:07]

Committed patchset #1 (id:1) manually as
2d9e690e6c4f8c7eba9ee52f43c816fac17823e1 (presubmit successful).