incremental improvement to top level code completion results

incremental improvement to top level code completion results

BUG=
R=paulberry@google.com

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

[Paul Berry, 2014-08-06 15:48:59]

lgtm

[danrubel, 2014-08-06 18:35:43]

Found serious enough problems that I did not want to submit the CL as it was.

PTAL

[Paul Berry, 2014-08-06 18:50:41]

lgtm

https://codereview.chromium.org/440343003/diff/20001/pkg/analysis_services/li...
File pkg/analysis_services/lib/src/completion/top_level_computer.dart (right):

https://codereview.chromium.org/440343003/diff/20001/pkg/analysis_services/li...
pkg/analysis_services/lib/src/completion/top_level_computer.dart:35:
visibleLibraries.addAll(unit.element.library.importedLibraries);
Do we need to do extra work to filter out names beginning with '_' from imported
libraries?  E.g.

foo.dart:
import 'bar.dart';
class A {}
class _B {}
main() {
  ^
}

bar.dart:
class C {}
class _D {}

Completing at the position marked "^" should show "A", "_B", and "C" as options,
but not "_D".

(It's ok by me if you want to address this in a future CL).

[danrubel, 2014-08-06 19:15:04]

Committed patchset #3 manually as 38942 (presubmit successful).

[danrubel, 2014-08-06 19:44:38]

https://codereview.chromium.org/440343003/diff/20001/pkg/analysis_services/li...
File pkg/analysis_services/lib/src/completion/top_level_computer.dart (right):

https://codereview.chromium.org/440343003/diff/20001/pkg/analysis_services/li...
pkg/analysis_services/lib/src/completion/top_level_computer.dart:35:
visibleLibraries.addAll(unit.element.library.importedLibraries);
On 2014/08/06 18:50:41, Paul Berry wrote:
> Do we need to do extra work to filter out names beginning with '_' from
imported
> libraries?  E.g.
> 
> foo.dart:
> import 'bar.dart';
> class A {}
> class _B {}
> main() {
>   ^
> }
> 
> bar.dart:
> class C {}
> class _D {}
> 
> Completing at the position marked "^" should show "A", "_B", and "C" as
options,
> but not "_D".
> 
> (It's ok by me if you want to address this in a future CL).

Good point. https://codereview.chromium.org/445083003/

[scheglov, 2014-08-07 02:42:26]

https://codereview.chromium.org/440343003/diff/40001/pkg/analysis_services/li...
File pkg/analysis_services/lib/completion/completion_computer.dart (right):

https://codereview.chromium.org/440343003/diff/40001/pkg/analysis_services/li...
pkg/analysis_services/lib/completion/completion_computer.dart:115:
LibraryElement library = context.computeLibraryElement(source);
Do we want to support the case when source is a part?

https://codereview.chromium.org/440343003/diff/40001/pkg/analysis_services/li...
File pkg/analysis_services/lib/src/completion/top_level_computer.dart (right):

https://codereview.chromium.org/440343003/diff/40001/pkg/analysis_services/li...
pkg/analysis_services/lib/src/completion/top_level_computer.dart:37: // Compute
the set of possible classes and top level variables
and functions?
and function type aliases?

https://codereview.chromium.org/440343003/diff/40001/pkg/analysis_services/te...
File pkg/analysis_services/test/completion/top_level_computer_test.dart (right):

https://codereview.chromium.org/440343003/diff/40001/pkg/analysis_services/te...
pkg/analysis_services/test/completion/top_level_computer_test.dart:36:
assertHasResult(CompletionSuggestionKind.TOP_LEVEL_VARIABLE, 'T1');
Is a top-level variable suggestion useful here?

I think we want only types at this point of AST.

[danrubel, 2014-08-08 20:28:40]

Thanks for the comments!

https://codereview.chromium.org/440343003/diff/40001/pkg/analysis_services/li...
File pkg/analysis_services/lib/completion/completion_computer.dart (right):

https://codereview.chromium.org/440343003/diff/40001/pkg/analysis_services/li...
pkg/analysis_services/lib/completion/completion_computer.dart:115:
LibraryElement library = context.computeLibraryElement(source);
On 2014/08/07 02:42:25, scheglov wrote:
> Do we want to support the case when source is a part?

Definitely part of the plan. I'm incrementally improving the code completion
functionality. I'll add a comment to that effect so that I don't forget.

https://codereview.chromium.org/440343003/diff/40001/pkg/analysis_services/li...
File pkg/analysis_services/lib/src/completion/top_level_computer.dart (right):

https://codereview.chromium.org/440343003/diff/40001/pkg/analysis_services/li...
pkg/analysis_services/lib/src/completion/top_level_computer.dart:37: // Compute
the set of possible classes and top level variables
On 2014/08/07 02:42:25, scheglov wrote:
> and functions?
> and function type aliases?

Yes, it should already include functions. I'll add a test for that and update
this comment.

https://codereview.chromium.org/440343003/diff/40001/pkg/analysis_services/te...
File pkg/analysis_services/test/completion/top_level_computer_test.dart (right):

https://codereview.chromium.org/440343003/diff/40001/pkg/analysis_services/te...
pkg/analysis_services/test/completion/top_level_computer_test.dart:36:
assertHasResult(CompletionSuggestionKind.TOP_LEVEL_VARIABLE, 'T1');
On 2014/08/07 02:42:26, scheglov wrote:
> Is a top-level variable suggestion useful here?
> 
> I think we want only types at this point of AST.

Top level variables in one library can be accessed by another, so I think we
should include them.

[scheglov, 2014-08-08 20:58:43]

https://codereview.chromium.org/440343003/diff/40001/pkg/analysis_services/te...
File pkg/analysis_services/test/completion/top_level_computer_test.dart (right):

https://codereview.chromium.org/440343003/diff/40001/pkg/analysis_services/te...
pkg/analysis_services/test/completion/top_level_computer_test.dart:36:
assertHasResult(CompletionSuggestionKind.TOP_LEVEL_VARIABLE, 'T1');
On 2014/08/08 20:28:39, danrubel wrote:
> On 2014/08/07 02:42:26, scheglov wrote:
> > Is a top-level variable suggestion useful here?
> > 
> > I think we want only types at this point of AST.
> 
> Top level variables in one library can be accessed by another, so I think we
> should include them.

Correct me if I'm wrong, but I don't think we can reference a top-level variable
inside of a class body.
We can only define some class member at this point.

[danrubel, 2014-08-09 10:23:03]

https://codereview.chromium.org/440343003/diff/40001/pkg/analysis_services/te...
File pkg/analysis_services/test/completion/top_level_computer_test.dart (right):

https://codereview.chromium.org/440343003/diff/40001/pkg/analysis_services/te...
pkg/analysis_services/test/completion/top_level_computer_test.dart:36:
assertHasResult(CompletionSuggestionKind.TOP_LEVEL_VARIABLE, 'T1');
On 2014/08/08 20:58:43, scheglov wrote:
> On 2014/08/08 20:28:39, danrubel wrote:
> > On 2014/08/07 02:42:26, scheglov wrote:
> > > Is a top-level variable suggestion useful here?
> > > 
> > > I think we want only types at this point of AST.
> > 
> > Top level variables in one library can be accessed by another, so I think we
> > should include them.
> 
> Correct me if I'm wrong, but I don't think we can reference a top-level
variable
> inside of a class body.
> We can only define some class member at this point.

Ah, now I understand what you are saying. Yes, this is a misleading test which I
will correct. I have not yet begun filtering suggestions based upon completion
offset.