Mix file: and package: URIs during analysis.

Mix file: and package: URIs during analysis.

R=brianwilkerson@google.com, paulberry@google.com
BUG=

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

[Brian Wilkerson, 2016-11-02 22:11:26]

If we hit the cache with a package: URI this will optimize a later access using
a file: URI, but not the other way around. That seems kind of strange. How big
of a performance improvement is this? Is this necessary? If so, the I guess it
lgtm, but unless we know it helps the asymmetry raises red flags.

[Paul Berry, 2016-11-02 22:16:20]

The asymmetry feels weird to me too.  Is it necessary to store the unlinked
summary at both URIs in order to get correct behavior, or is it just a speed
optimization?

If it's just a speed optimization, I'm curious how much it helps.  (My intuition
is that in practical projects, files inside "lib" are always accessed through
"package:" URIs, and files outside "lib" are always accessed through "file:"
URIs, so it shouldn't actually make a difference).

If it's necessary for correctness, I would like to understand why.

[scheglov, 2016-11-02 22:25:45]

On 2016/11/02 22:16:20, Paul Berry wrote:
> The asymmetry feels weird to me too.  Is it necessary to store the unlinked
> summary at both URIs in order to get correct behavior, or is it just a speed
> optimization?
> 
> If it's just a speed optimization, I'm curious how much it helps.  (My
intuition
> is that in practical projects, files inside "lib" are always accessed through
> "package:" URIs, and files outside "lib" are always accessed through "file:"
> URIs, so it shouldn't actually make a difference).
> 
> If it's necessary for correctness, I would like to understand why.

Yes, this necessary for correctness.

When we have a situation like in the added test, we have one file with the path
/test/lib/a.dart, with the "primary" URI package:test/a.dart.

But /test/test/c.dart (which has the URI file:///test/test/c.dart) references
a.dart using relative URI so during linking will attempt to get unlinked unit
with URI file:///test/lib/a.dart.
If we don't put a.dart with the file: URI, linking of c.dart fails.

[Brian Wilkerson, 2016-11-02 22:41:00]

> If we don't put a.dart with the file: URI, linking of c.dart fails.

I probably just don't understand linking well enough, but why would it fail?
Wouldn't we just look for (and create if necessary) a summary for a different
URI?

As for the symmetry, it sounds like we only need the file: URI for files in the
lib directory. Is that true?

Out of curiosity, would it solve the problem if we always used the file path
rather than the URI to cache the unlinked unit?

Or, alternatively, could we just fix the test to use a package: URI?

[Paul Berry, 2016-11-03 13:35:30]

On 2016/11/02 22:41:00, Brian Wilkerson wrote:
> > If we don't put a.dart with the file: URI, linking of c.dart fails.
> 
> I probably just don't understand linking well enough, but why would it fail?
> Wouldn't we just look for (and create if necessary) a summary for a different
> URI?
> 
> As for the symmetry, it sounds like we only need the file: URI for files in
the
> lib directory. Is that true?
> 
> Out of curiosity, would it solve the problem if we always used the file path
> rather than the URI to cache the unlinked unit?
> 
> Or, alternatively, could we just fix the test to use a package: URI?

Ok, I think I understand what's going on now, and I think this patch is working
around a latent bug.

The issue is that we keep just one FileState object per file, but the resolution
of imports, exports, and part declarations depends on the URI by which the file
was reached.  In a Dart program written by a sane person, it doesn't matter. 
But there are some insane corner cases where it does.  And I think getting the
corner cases right is important, because it helps keep the analysis server
faithful to how the program is going to be interpreted by the VM.  For example,
here is an insane program where my_pkg/lib/c.dart is interpreted differently
depending on how you got to it.  Assume strong mode is enabled:

my_pkg/.packages:
  my_pkg:lib

my_pkg/bin/a.dart:
  import 'package:my_pkg/c.dart';
  main() {}
  int x = y;

my_pkg/bin/b.dart:
  import '../lib/c.dart';
  main() => foo();
  int x = y;

my_pkg/lib/c.dart:
  import '../test/d.dart';
  var y = z;

my_pkg/test/d.dart:
  String z = "string";

When analysis server analyzes my_pkg/lib/c.dart, it should assume the file is
being visited via the URI "package:my_pkg/c.dart", so it should produce an error
on the import statement, because you can't go up a directory from the root of a
package.

When analysis server analyzer my_pkg/bin/b.dart, it should produce the error "A
value of type 'String' can't be assigned to a variable of type 'int'", because
file:///my_pkg/bin/b.dart imports file:///my_pkg/lib/c.dart, which successfully
imports file:///my_pkg/test/d.dart, causing y to have an inferred type of
String.

When analysis server analyzes my_pkg/bin/a.dart, it should produce no error,
because file:///my_pkg/bin/a.dart imports package:my_pkg/c.dart, and
package:my_pkg/c.dart's import is erroneous, causing y's reference to z to be
unresolved (and therefore have type dynamic).

It's crazy, but analyzer (and analysis server as currently implemented) get it
right.  I think the new driver should too.

I think achieving that is going to require a little bit of re-architecting. 
Instead of storing a map from path to FileState, we should have a map from URI
to FileState, so that a given file can have different states depending on the
URI that is used to access it.  When we get a changeFile notification we'll need
to invalidate *all* FileStates that resolve to the given path.

And we should put my example above (or something like it) into the unit tests to
make sure we don't regress.

[Paul Berry, 2016-11-03 14:41:28]

On 2016/11/03 13:35:30, Paul Berry wrote:
> On 2016/11/02 22:41:00, Brian Wilkerson wrote:
> > > If we don't put a.dart with the file: URI, linking of c.dart fails.
> > 
> > I probably just don't understand linking well enough, but why would it fail?
> > Wouldn't we just look for (and create if necessary) a summary for a
different
> > URI?
> > 
> > As for the symmetry, it sounds like we only need the file: URI for files in
> the
> > lib directory. Is that true?
> > 
> > Out of curiosity, would it solve the problem if we always used the file path
> > rather than the URI to cache the unlinked unit?
> > 
> > Or, alternatively, could we just fix the test to use a package: URI?
> 
> Ok, I think I understand what's going on now, and I think this patch is
working
> around a latent bug.
> 
> The issue is that we keep just one FileState object per file, but the
resolution
> of imports, exports, and part declarations depends on the URI by which the
file
> was reached.  In a Dart program written by a sane person, it doesn't matter. 
> But there are some insane corner cases where it does.  And I think getting the
> corner cases right is important, because it helps keep the analysis server
> faithful to how the program is going to be interpreted by the VM.  For
example,
> here is an insane program where my_pkg/lib/c.dart is interpreted differently
> depending on how you got to it.  Assume strong mode is enabled:
> 
> my_pkg/.packages:
>   my_pkg:lib
> 
> my_pkg/bin/a.dart:
>   import 'package:my_pkg/c.dart';
>   main() {}
>   int x = y;
> 
> my_pkg/bin/b.dart:
>   import '../lib/c.dart';
>   main() => foo();
>   int x = y;
> 
> my_pkg/lib/c.dart:
>   import '../test/d.dart';
>   var y = z;
> 
> my_pkg/test/d.dart:
>   String z = "string";
> 
> When analysis server analyzes my_pkg/lib/c.dart, it should assume the file is
> being visited via the URI "package:my_pkg/c.dart", so it should produce an
error
> on the import statement, because you can't go up a directory from the root of
a
> package.
> 
> When analysis server analyzer my_pkg/bin/b.dart, it should produce the error
"A
> value of type 'String' can't be assigned to a variable of type 'int'", because
> file:///my_pkg/bin/b.dart imports file:///my_pkg/lib/c.dart, which
successfully
> imports file:///my_pkg/test/d.dart, causing y to have an inferred type of
> String.
> 
> When analysis server analyzes my_pkg/bin/a.dart, it should produce no error,
> because file:///my_pkg/bin/a.dart imports package:my_pkg/c.dart, and
> package:my_pkg/c.dart's import is erroneous, causing y's reference to z to be
> unresolved (and therefore have type dynamic).
> 
> It's crazy, but analyzer (and analysis server as currently implemented) get it
> right.  I think the new driver should too.
> 
> I think achieving that is going to require a little bit of re-architecting. 
> Instead of storing a map from path to FileState, we should have a map from URI
> to FileState, so that a given file can have different states depending on the
> URI that is used to access it.  When we get a changeFile notification we'll
need
> to invalidate *all* FileStates that resolve to the given path.
> 
> And we should put my example above (or something like it) into the unit tests
to
> make sure we don't regress.

On further reflection, maybe it would be better to separate FileState into two
data structures: FileState to keep track of  the state of files (which would
include the content, content hash, unlinked summary, and API signature) and a
new data structure to keep track of the import graph.  FileState would continue
to be keyed by file path (As it is now), whereas the new data structure would be
keyed by URI.

[scheglov, 2016-11-03 20:48:26]

On 2016/11/03 14:41:28, Paul Berry wrote:
> On 2016/11/03 13:35:30, Paul Berry wrote:
> > On 2016/11/02 22:41:00, Brian Wilkerson wrote:
> > > > If we don't put a.dart with the file: URI, linking of c.dart fails.
> > > 
> > > I probably just don't understand linking well enough, but why would it
fail?
> > > Wouldn't we just look for (and create if necessary) a summary for a
> different
> > > URI?
> > > 
> > > As for the symmetry, it sounds like we only need the file: URI for files
in
> > the
> > > lib directory. Is that true?
> > > 
> > > Out of curiosity, would it solve the problem if we always used the file
path
> > > rather than the URI to cache the unlinked unit?
> > > 
> > > Or, alternatively, could we just fix the test to use a package: URI?
> > 
> > Ok, I think I understand what's going on now, and I think this patch is
> working
> > around a latent bug.
> > 
> > The issue is that we keep just one FileState object per file, but the
> resolution
> > of imports, exports, and part declarations depends on the URI by which the
> file
> > was reached.  In a Dart program written by a sane person, it doesn't matter.

> > But there are some insane corner cases where it does.  And I think getting
the
> > corner cases right is important, because it helps keep the analysis server
> > faithful to how the program is going to be interpreted by the VM.  For
> example,
> > here is an insane program where my_pkg/lib/c.dart is interpreted differently
> > depending on how you got to it.  Assume strong mode is enabled:
> > 
> > my_pkg/.packages:
> >   my_pkg:lib
> > 
> > my_pkg/bin/a.dart:
> >   import 'package:my_pkg/c.dart';
> >   main() {}
> >   int x = y;
> > 
> > my_pkg/bin/b.dart:
> >   import '../lib/c.dart';
> >   main() => foo();
> >   int x = y;
> > 
> > my_pkg/lib/c.dart:
> >   import '../test/d.dart';
> >   var y = z;
> > 
> > my_pkg/test/d.dart:
> >   String z = "string";
> > 
> > When analysis server analyzes my_pkg/lib/c.dart, it should assume the file
is
> > being visited via the URI "package:my_pkg/c.dart", so it should produce an
> error
> > on the import statement, because you can't go up a directory from the root
of
> a
> > package.
> > 
> > When analysis server analyzer my_pkg/bin/b.dart, it should produce the error
> "A
> > value of type 'String' can't be assigned to a variable of type 'int'",
because
> > file:///my_pkg/bin/b.dart imports file:///my_pkg/lib/c.dart, which
> successfully
> > imports file:///my_pkg/test/d.dart, causing y to have an inferred type of
> > String.
> > 
> > When analysis server analyzes my_pkg/bin/a.dart, it should produce no error,
> > because file:///my_pkg/bin/a.dart imports package:my_pkg/c.dart, and
> > package:my_pkg/c.dart's import is erroneous, causing y's reference to z to
be
> > unresolved (and therefore have type dynamic).
> > 
> > It's crazy, but analyzer (and analysis server as currently implemented) get
it
> > right.  I think the new driver should too.
> > 
> > I think achieving that is going to require a little bit of re-architecting. 
> > Instead of storing a map from path to FileState, we should have a map from
URI
> > to FileState, so that a given file can have different states depending on
the
> > URI that is used to access it.  When we get a changeFile notification we'll
> need
> > to invalidate *all* FileStates that resolve to the given path.
> > 
> > And we should put my example above (or something like it) into the unit
tests
> to
> > make sure we don't regress.
> 
> On further reflection, maybe it would be better to separate FileState into two
> data structures: FileState to keep track of  the state of files (which would
> include the content, content hash, unlinked summary, and API signature) and a
> new data structure to keep track of the import graph.  FileState would
continue
> to be keyed by file path (As it is now), whereas the new data structure would
be
> keyed by URI.

Paul, I agree.
I thought so too, but want to delay this separation a bit.

Separation between FileState and UriState (or UnitState) could improve even more
sharing.
Currently we keep separate copies of UnlinkedBundle and content for each
analysis root.
With introduction of shared FileState (pure files) we could share bundles and
content.

But I think this sharing is not critical for initial version, so I would like to
do it after many other things are done.

[scheglov, 2016-11-03 21:21:15]

PTAL

[Brian Wilkerson, 2016-11-03 21:26:21]

lgtm

[Paul Berry, 2016-11-03 21:39:34]

https://codereview.chromium.org/2469323004/diff/20001/pkg/analyzer/lib/src/da...
File pkg/analyzer/lib/src/dart/analysis/driver.dart (right):

https://codereview.chromium.org/2469323004/diff/20001/pkg/analyzer/lib/src/da...
pkg/analyzer/lib/src/dart/analysis/driver.dart:686: bool apiChanged =
file.refresh();
Don't we need to refresh all files that are associated with [path]?

Otherwise, if a file's import/export/part declarations are changed, only the
FileState corresponding to its canonical URI will get updated, and the import
graph for its other URIs will still be old.

[scheglov, 2016-11-04 04:03:38]

PTAL

https://codereview.chromium.org/2469323004/diff/20001/pkg/analyzer/lib/src/da...
File pkg/analyzer/lib/src/dart/analysis/driver.dart (right):

https://codereview.chromium.org/2469323004/diff/20001/pkg/analyzer/lib/src/da...
pkg/analyzer/lib/src/dart/analysis/driver.dart:686: bool apiChanged =
file.refresh();
On 2016/11/03 21:39:34, Paul Berry wrote:
> Don't we need to refresh all files that are associated with [path]?
> 
> Otherwise, if a file's import/export/part declarations are changed, only the
> FileState corresponding to its canonical URI will get updated, and the import
> graph for its other URIs will still be old.

Done.

[Paul Berry, 2016-11-04 04:12:50]

lgtm

[scheglov, 2016-11-04 04:14:52]

Description was changed from

==========
Mix file: and package: URIs during analysis.

R=brianwilkerson@google.com, paulberry@google.com
BUG=
==========

to

==========
Mix file: and package: URIs during analysis.

R=brianwilkerson@google.com, paulberry@google.com
BUG=

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

[scheglov, 2016-11-04 04:14:53]

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