support part file completions

support part file completions

BUG=
R=paulberry@google.com

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

[danrubel, 2014-12-18 04:40:38]

danrubel@google.com changed reviewers:
+ paulberry@google.com

[Paul Berry, 2014-12-18 18:37:02]

https://codereview.chromium.org/815683002/diff/1/pkg/analysis_server/lib/src/...
File pkg/analysis_server/lib/src/services/completion/dart_completion_cache.dart
(right):

https://codereview.chromium.org/815683002/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/services/completion/dart_completion_cache.dart:123:
libUnit = context.resolveCompilationUnit2(libSource, libSource);
Two issues:
1. context.resolveCompilationUnit2(libSource, libSource) will give us the
resolved AST for libSource.  I think we want the resolved AST for source, so
this should be context.resolveCompilationUnit2(source, libSource).

2. Why not use context.computeResolvedCompilationUnitAsync()?  Its whole reason
for existence is to allow the completion engine to get resolved AST's without
blocking, which is exactly what we want here.  If your assumption is correct
that the AST is cached or is not costly to recompute, then there's no harm; and
if your assumption is wrong and it is costly to recompute (say because a bunch
of other libraries need to be analyzed first) the server will still be able to
respond to other requests from the client.

https://codereview.chromium.org/815683002/diff/1/pkg/analysis_server/lib/src/...
File
pkg/analysis_server/lib/src/services/completion/dart_completion_manager.dart
(right):

https://codereview.chromium.org/815683002/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/services/completion/dart_completion_manager.dart:193:
if (libraries == null || libraries.length == 0 || libraries[0] == null) {
Are all of these possibilities expected to arise in practice?  My understanding
of the contract of getLibrariesContainingSource() is that it always returns a
non-null list containing non-null elements, so the only case which can actually
arise is libraries.length == 0.  I'm fine with defensive programming, but we
should include an assert statement inside the body of the if statement so that
(a) if getLibrariesContainingSource() ever breaks its contract we'll find out,
and (b) future readers of the code won't draw incorrect conclusions about the
contract.

Accordingly, I'd recommend adding this assertion:

assert(libraries != null && libraries.length == 0);

[danrubel, 2014-12-19 18:46:10]

https://codereview.chromium.org/815683002/diff/1/pkg/analysis_server/lib/src/...
File pkg/analysis_server/lib/src/services/completion/dart_completion_cache.dart
(right):

https://codereview.chromium.org/815683002/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/services/completion/dart_completion_cache.dart:123:
libUnit = context.resolveCompilationUnit2(libSource, libSource);
On 2014/12/18 18:37:02, Paul Berry wrote:
> Two issues:
> 1. context.resolveCompilationUnit2(libSource, libSource) will give us the
> resolved AST for libSource.  I think we want the resolved AST for source, so
> this should be context.resolveCompilationUnit2(source, libSource).

I need the library unit not the part unit to traverse the library directives and
cache the imported elements.

> 2. Why not use context.computeResolvedCompilationUnitAsync()?  Its whole
reason
> for existence is to allow the completion engine to get resolved AST's without
> blocking, which is exactly what we want here.  If your assumption is correct
> that the AST is cached or is not costly to recompute, then there's no harm;
and
> if your assumption is wrong and it is costly to recompute (say because a bunch
> of other libraries need to be analyzed first) the server will still be able to
> respond to other requests from the client.

Right! I use computeResolvedCompilationUnitAsync elsewhere and should use it
here as well. Done.

https://codereview.chromium.org/815683002/diff/1/pkg/analysis_server/lib/src/...
File
pkg/analysis_server/lib/src/services/completion/dart_completion_manager.dart
(right):

https://codereview.chromium.org/815683002/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/services/completion/dart_completion_manager.dart:193:
if (libraries == null || libraries.length == 0 || libraries[0] == null) {
On 2014/12/18 18:37:02, Paul Berry wrote:
> Are all of these possibilities expected to arise in practice?  My
understanding
> of the contract of getLibrariesContainingSource() is that it always returns a
> non-null list containing non-null elements, so the only case which can
actually
> arise is libraries.length == 0.  I'm fine with defensive programming, but we
> should include an assert statement inside the body of the if statement so that
> (a) if getLibrariesContainingSource() ever breaks its contract we'll find out,
> and (b) future readers of the code won't draw incorrect conclusions about the
> contract.
> 
> Accordingly, I'd recommend adding this assertion:
> 
> assert(libraries != null && libraries.length == 0);

The dartdoc does not clearly state that it will not return a null array or null
element in the array. That said, I like your point that those should not happen
in practice and thus should be coded as assert statements. Done.

[danrubel, 2014-12-19 18:46:24]

PTAL

[Paul Berry, 2014-12-19 19:13:30]

lgtm

https://codereview.chromium.org/815683002/diff/1/pkg/analysis_server/lib/src/...
File pkg/analysis_server/lib/src/services/completion/dart_completion_cache.dart
(right):

https://codereview.chromium.org/815683002/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/services/completion/dart_completion_cache.dart:123:
libUnit = context.resolveCompilationUnit2(libSource, libSource);
On 2014/12/19 18:46:09, danrubel wrote:
> On 2014/12/18 18:37:02, Paul Berry wrote:
> > Two issues:
> > 1. context.resolveCompilationUnit2(libSource, libSource) will give us the
> > resolved AST for libSource.  I think we want the resolved AST for source, so
> > this should be context.resolveCompilationUnit2(source, libSource).
> 
> I need the library unit not the part unit to traverse the library directives
and
> cache the imported elements.

Ok.  It would be nice to add a comment to the code explaining this, since at
first glance it looks like a mistake.

https://codereview.chromium.org/815683002/diff/20001/pkg/analysis_server/lib/...
File
pkg/analysis_server/lib/src/services/completion/dart_completion_manager.dart
(right):

https://codereview.chromium.org/815683002/diff/20001/pkg/analysis_server/lib/...
pkg/analysis_server/lib/src/services/completion/dart_completion_manager.dart:193:
assert(libraries != null);
Nit pick: the change you've made adds assertions, which is great.  But it drops
the defensive programming, because it means that when the code is run in
unchecked mode, the presence of null will cause an exception.

IMHO it's best to be defensive for the benefit of unchecked mode and use
assertions for the benefit of checked mode.  For example, instead of doing the
rewrite you've done in this patch set, you could go back to what you had in
patch set 1 and just add a single assert statement:

if (libraries == null || libraries.length == 0 || libraries[0] == null) {
  // Only libraries.length == 0 is expected to occur in practice;
  // the others are to avoid NRE's.
  assert(libraries != null && libraries.length == 0);
  return new Future.value(null);
}

Your call, though.

[danrubel, 2014-12-20 05:12:30]

https://codereview.chromium.org/815683002/diff/1/pkg/analysis_server/lib/src/...
File pkg/analysis_server/lib/src/services/completion/dart_completion_cache.dart
(right):

https://codereview.chromium.org/815683002/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/services/completion/dart_completion_cache.dart:123:
libUnit = context.resolveCompilationUnit2(libSource, libSource);
On 2014/12/19 19:13:30, Paul Berry wrote:
> On 2014/12/19 18:46:09, danrubel wrote:
> > On 2014/12/18 18:37:02, Paul Berry wrote:
> > > Two issues:
> > > 1. context.resolveCompilationUnit2(libSource, libSource) will give us the
> > > resolved AST for libSource.  I think we want the resolved AST for source,
so
> > > this should be context.resolveCompilationUnit2(source, libSource).
> > 
> > I need the library unit not the part unit to traverse the library directives
> and
> > cache the imported elements.
> 
> Ok.  It would be nice to add a comment to the code explaining this, since at
> first glance it looks like a mistake.

Good point. Done.

[danrubel, 2014-12-20 05:15:56]

Committed patchset #4 (id:60001) manually as 42541 (presubmit successful).