Add an UNUSED_SHOWN_NAMES hint to the analyzer

Add an UNUSED_SHOWN_NAMES hint to the analyzer

BUG=https://github.com/dart-lang/sdk/issues/21030
R=brianwilkerson@google.com, scheglov@google.com

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

[srawlins, 2016-04-06 14:56:31]

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

[srawlins, 2016-04-06 14:56:32]

Hi Konstantin, I see you're the most active on the analyzer, but please redirect
this review if you'd rather it go to someone else!

This fixes https://github.com/dart-lang/sdk/issues/21030.I think I tested it
pretty well, but please let me know.

A manual review leads me to believe that this does not increase analysis time by
much, but I'd be very very happy to run any kind of benchmark, if you can point
me to where/how I would do that. But in the basic sense, it adds short checks to
the existing loops in ImportsVerifier, short-circuiting where possible.

Thanks!!

[Brian Wilkerson, 2016-04-06 15:17:39]

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

[Brian Wilkerson, 2016-04-06 15:17:40]

lgtm

https://codereview.chromium.org/1863103002/diff/1/pkg/analyzer/lib/src/genera...
File pkg/analyzer/lib/src/generated/error.dart (right):

https://codereview.chromium.org/1863103002/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/error.dart:3785: const
HintCode('UNUSED_SHOWN_NAME', "Unused name shown in import");
The name should be included in the error message. Perhaps something like

"The name {0} is shown, but not used."

https://codereview.chromium.org/1863103002/diff/1/pkg/analyzer/lib/src/genera...
File pkg/analyzer/lib/src/generated/resolver.dart (right):

https://codereview.chromium.org/1863103002/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/resolver.dart:4114: List<SimpleIdentifier> list =
usedElements.prefixes[element];
usedElements.prefixes.putIfAbsent(element, () => <SimpleIdentifier>[]);

https://codereview.chromium.org/1863103002/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/resolver.dart:4556: continue;
FWIW, I actually preferred the code the way it was, but that's just a personal
preference.

https://codereview.chromium.org/1863103002/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/resolver.dart:4669: for (var identifier in
identifiers) {
nit: we fully type annotate everything in the analyzer package (applies in
multiple places)

[scheglov, 2016-04-06 15:34:39]

DBC

https://codereview.chromium.org/1863103002/diff/1/pkg/analyzer/lib/src/genera...
File pkg/analyzer/lib/src/generated/resolver.dart (right):

https://codereview.chromium.org/1863103002/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/resolver.dart:4734: List<SimpleIdentifier> list =
_unusedShownNamesMap[importDirective];
Could you use a more specific name than "list"?

https://codereview.chromium.org/1863103002/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/resolver.dart:4812: List list = new
List<SimpleIdentifier>();
1. Add type arguments.
2. Use a better name.

https://codereview.chromium.org/1863103002/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/resolver.dart:12668: final Map<PrefixElement,
List<SimpleIdentifier>> prefixes =
1. Instances of the class UsedImportedElements are put into the analysis cache.
It is a BAD idea to put AST nodes into the cache. Use elements instead.

2. Update the name and comment.

[srawlins, 2016-04-07 15:27:23]

Thanks for the review! Everything's updated!

https://codereview.chromium.org/1863103002/diff/1/pkg/analyzer/lib/src/genera...
File pkg/analyzer/lib/src/generated/error.dart (right):

https://codereview.chromium.org/1863103002/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/error.dart:3785: const
HintCode('UNUSED_SHOWN_NAME', "Unused name shown in import");
On 2016/04/06 15:17:40, Brian Wilkerson wrote:
> The name should be included in the error message. Perhaps something like
> 
> "The name {0} is shown, but not used."

Done.

https://codereview.chromium.org/1863103002/diff/1/pkg/analyzer/lib/src/genera...
File pkg/analyzer/lib/src/generated/resolver.dart (right):

https://codereview.chromium.org/1863103002/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/resolver.dart:4114: List<SimpleIdentifier> list =
usedElements.prefixes[element];
On 2016/04/06 15:17:40, Brian Wilkerson wrote:
> usedElements.prefixes.putIfAbsent(element, () => <SimpleIdentifier>[]);

Done.

https://codereview.chromium.org/1863103002/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/resolver.dart:4556: continue;
On 2016/04/06 15:17:40, Brian Wilkerson wrote:
> FWIW, I actually preferred the code the way it was, but that's just a personal
> preference.

Acknowledged.

https://codereview.chromium.org/1863103002/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/resolver.dart:4669: for (var identifier in
identifiers) {
On 2016/04/06 15:17:40, Brian Wilkerson wrote:
> nit: we fully type annotate everything in the analyzer package (applies in
> multiple places)

Done.

https://codereview.chromium.org/1863103002/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/resolver.dart:4734: List<SimpleIdentifier> list =
_unusedShownNamesMap[importDirective];
On 2016/04/06 15:34:39, scheglov wrote:
> Could you use a more specific name than "list"?

Done.

https://codereview.chromium.org/1863103002/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/resolver.dart:4812: List list = new
List<SimpleIdentifier>();
On 2016/04/06 15:34:39, scheglov wrote:
> 1. Add type arguments.
> 2. Use a better name.

Done.

https://codereview.chromium.org/1863103002/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/resolver.dart:12668: final Map<PrefixElement,
List<SimpleIdentifier>> prefixes =
On 2016/04/06 15:34:39, scheglov wrote:
> 1. Instances of the class UsedImportedElements are put into the analysis
cache.
> It is a BAD idea to put AST nodes into the cache. Use elements instead.
> 
> 2. Update the name and comment.

Done.

[scheglov, 2016-04-07 16:26:15]

LGTM

[Brian Wilkerson, 2016-04-07 16:30:08]

lgtm

[srawlins, 2016-04-08 18:41:45]

I ran a performance analysis. Results here:

https://gist.github.com/srawlins/684659c7d7aec389d54c5e517a904f17

TL;DR this new analysis costs less than 1% total time (less than 0.1s) for a
file with 100 import lines.

[srawlins, 2016-04-08 18:44:40]

Description was changed from

==========
Add an UNUSED_SHOWN_NAMES hint to the analyzer

BUG=https://github.com/dart-lang/sdk/issues/21030
==========

to

==========
Add an UNUSED_SHOWN_NAMES hint to the analyzer

BUG=https://github.com/dart-lang/sdk/issues/21030
R=brianwilkerson@google.com, scheglov@google.com

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

[srawlins, 2016-04-08 18:44:41]

Committed patchset #3 (id:40001) manually as
c1963b3995765dc8c4a5a90a3d01dac079552bf0 (presubmit successful).

[Paul Berry, 2016-04-08 21:00:58]

On 2016/04/08 18:44:41, srawlins wrote:
> Committed patchset #3 (id:40001) manually as
> c1963b3995765dc8c4a5a90a3d01dac079552bf0 (presubmit successful).

It looks like this CL caused a unit test failure in analyzer.

Repro with: python tools/test.py -aia32 --checked -mrelease -t60
pkg/analyzer/test/generated/non_hint_code_test

Sample output:

FAIL: NonHintCodeTest | test_unusedImport_prefix_topLevelFunction
  Expected 0 errors of type HintCode.UNUSED_SHOWN_NAME, found 1 (84)
  package:unittest/src/simple_configuration.dart 128:34      
SimpleConfiguration.onExpectFailure
  package:unittest/src/simple_configuration.dart 24:13       
_ExpectFailureHandler.fail
  package:unittest/src/matcher/expect.dart 103:18             fail
  pkg/analyzer/test/generated/test_support.dart 330:7        
GatheringErrorListener.assertErrorsWithCodes
  pkg/analyzer/test/generated/resolver_test_case.dart 387:19 
ResolverTestCase.assertErrors
  pkg/analyzer/test/generated/resolver_test_case.dart 422:5  
ResolverTestCase.assertNoErrors
  pkg/analyzer/test/generated/non_hint_code_test.dart 1095:5 
NonHintCodeTest.test_unusedImport_prefix_topLevelFunction
  dart:mirrors-patch/mirrors_impl.dart 533                   
_LocalInstanceMirror._invoke
  dart:mirrors-patch/mirrors_impl.dart 529                   
_LocalInstanceMirror.invoke
  pkg/analyzer/test/reflective_tests.dart 128:35              _runTest.<fn>
  dart:async/zone.dart 902                                    _rootRunUnary
  dart:async/zone.dart 798                                   
_CustomZone.runUnary
  dart:async/future_impl.dart 553                            
_Future._propagateToListeners.handleValueCallback
  dart:async/future_impl.dart 639                            
_Future._propagateToListeners
  dart:async/future_impl.dart 426                            
_Future._completeWithValue
  dart:async/future_impl.dart 481                            
_Future._asyncComplete.<fn>
  dart:async/zone.dart 895                                    _rootRun
  dart:async/zone.dart 790                                    _CustomZone.run
  dart:async/zone.dart 696                                   
_CustomZone.runGuarded
  dart:async/zone.dart 721                                   
_CustomZone.bindCallback.<fn>
  dart:async/schedule_microtask.dart 41                       _microtaskLoop
  dart:async/schedule_microtask.dart 50                      
_startMicrotaskLoop
  dart:isolate-patch/isolate_patch.dart 96                   
_runPendingImmediateCallback
  dart:isolate-patch/isolate_patch.dart 149                  
_RawReceivePortImpl._handleMessage

Would you mind having a look?