support resolving .packages in FE

support resolving .packages in FE

BUG=
R=paulberry@google.com

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

[Siggi Cherem (dart-lang), 2017-07-14 03:55:12]

Patchset #1 (id:1) has been deleted

[Siggi Cherem (dart-lang), 2017-07-14 04:54:34]

Patchset #1 (id:20001) has been deleted

[Siggi Cherem (dart-lang), 2017-07-14 04:54:41]

Patchset #1 (id:40001) has been deleted

[Siggi Cherem (dart-lang), 2017-07-14 04:54:46]

Patchset #1 (id:60001) has been deleted

[Siggi Cherem (dart-lang), 2017-07-14 04:55:58]

Patchset #1 (id:80001) has been deleted

[Siggi Cherem (dart-lang), 2017-07-14 04:58:51]

sigmund@google.com changed reviewers:
+ ahe@google.com, paulberry@google.com

[Siggi Cherem (dart-lang), 2017-07-14 04:58:52]

https://codereview.chromium.org/2979003002/diff/100001/pkg/front_end/lib/src/...
File pkg/front_end/lib/src/fasta/uri_translator_impl.dart (right):

https://codereview.chromium.org/2979003002/diff/100001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/fasta/uri_translator_impl.dart:96: // TODO(sigmund):
report an error.
(I plan to do so once Peter lands his CL adding context.report)

[ahe, 2017-07-14 13:44:58]

I need a late lunch before I finish this review, so here are my preliminary
comments.

The "s" thing is certainly optional.

https://codereview.chromium.org/2979003002/diff/100001/pkg/front_end/lib/src/...
File pkg/front_end/lib/src/base/processed_options.dart (right):

https://codereview.chromium.org/2979003002/diff/100001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/base/processed_options.dart:135:
reportMessageWithoutLocation(messageMissingInputs);
Remove the "s" from Inputs.

https://codereview.chromium.org/2979003002/diff/100001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/base/processed_options.dart:142: // UriTranslator
Can we make this a TODO and check the inputs later?

https://codereview.chromium.org/2979003002/diff/100001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/base/processed_options.dart:274: "Cannot infer a .packages
file when compiling multiple inputs."));
This isn't an internal error, this is a problem with how the API is being used,
and an error that we may report to an end user.

Also, I think there's a tip for this message: "Try adding --packages option."

There's also a possible todo here: we can make a loop and call _findPackages
multiple times and only report an error if we get different answers.

https://codereview.chromium.org/2979003002/diff/100001/pkg/front_end/lib/src/...
File pkg/front_end/lib/src/fasta/uri_translator_impl.dart (right):

https://codereview.chromium.org/2979003002/diff/100001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/fasta/uri_translator_impl.dart:94: return
packages?.resolve(uri, notFound: (_) => null);
Handle not found by reporting an error?

https://codereview.chromium.org/2979003002/diff/100001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/fasta/uri_translator_impl.dart:95: } on ArgumentError
catch (_) {
Argument error can occur for many reasons. Why is it safe to catch it here?

https://codereview.chromium.org/2979003002/diff/100001/pkg/front_end/messages...
File pkg/front_end/messages.yaml (right):

https://codereview.chromium.org/2979003002/diff/100001/pkg/front_end/messages...
pkg/front_end/messages.yaml:794: template: "No input files provided to the
compiler."
files -> file.

https://codereview.chromium.org/2979003002/diff/100001/pkg/front_end/messages...
pkg/front_end/messages.yaml:854: template: "Invalid .packages file '#uri':\n 
#string."
Invalid is a word that we should avoid. How about:

"Unable to read '.packages' file:\n  #string"

Also, in this case, the uri should be in the location, not the message.

https://codereview.chromium.org/2979003002/diff/100001/pkg/front_end/test/src...
File pkg/front_end/test/src/base/processed_options_test.dart (right):

https://codereview.chromium.org/2979003002/diff/100001/pkg/front_end/test/src...
pkg/front_end/test/src/base/processed_options_test.dart:90: var expected =
Uri.parse('file:///$packageDir/a.dart');
Is this a real file or a fake URI?

https://codereview.chromium.org/2979003002/diff/100001/pkg/front_end/test/src...
pkg/front_end/test/src/base/processed_options_test.dart:131:
.entityForUri(Uri.parse('file:///.packages'))
Use custom URI.

I don't like seeing error messages like this:

file:///file:///test-lib:4:11: Error: Expected an expression, but got '...'.
  int y = ......;
          ^

They're really confusing compared to:

org-dartlang-memory-filesystem:///test-lib:4:11: Error: Expected an expression,
but got '...'.
  int y = ......;
          ^

It's also hard to debug who's causing the URI to have a path that starts with
file:.

[Paul Berry, 2017-07-14 16:02:54]

lgtm

[Siggi Cherem (dart-lang), 2017-07-14 19:53:50]

Patchset #2 (id:120001) has been deleted

[Siggi Cherem (dart-lang), 2017-07-14 19:54:07]

Thanks!

https://codereview.chromium.org/2979003002/diff/100001/pkg/front_end/lib/src/...
File pkg/front_end/lib/src/base/processed_options.dart (right):

https://codereview.chromium.org/2979003002/diff/100001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/base/processed_options.dart:135:
reportMessageWithoutLocation(messageMissingInputs);
On 2017/07/14 13:44:57, ahe wrote:
> Remove the "s" from Inputs.

Done. Also updated the other messages to say "NotFound" to distingish better
when they are missing because they not provided or not found.

https://codereview.chromium.org/2979003002/diff/100001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/base/processed_options.dart:142: // UriTranslator
On 2017/07/14 13:44:57, ahe wrote:
> Can we make this a TODO and check the inputs later?

Sure, but it will require some subtle ordering of checks, because if .packages
is not given explicitly, we need to know the inptus to infer a .packages for
uritranslator.

https://codereview.chromium.org/2979003002/diff/100001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/base/processed_options.dart:274: "Cannot infer a .packages
file when compiling multiple inputs."));
On 2017/07/14 13:44:57, ahe wrote:
> This isn't an internal error, this is a problem with how the API is being
used,
> and an error that we may report to an end user.
> 
> Also, I think there's a tip for this message: "Try adding --packages option."

Done.

Note we have to be careful about using flags (like --packages) as tips because
most users of the FE will consume it indirectly from another tool and not
directly from the command line. We have to make sure all tools agree on the
flags before we use them in the tips.

I think `--packages` is a safe one though.
 
> There's also a possible todo here: we can make a loop and call _findPackages
> multiple times and only report an error if we get different answers.

done.

https://codereview.chromium.org/2979003002/diff/100001/pkg/front_end/lib/src/...
File pkg/front_end/lib/src/fasta/uri_translator_impl.dart (right):

https://codereview.chromium.org/2979003002/diff/100001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/fasta/uri_translator_impl.dart:94: return
packages?.resolve(uri, notFound: (_) => null);
On 2017/07/14 13:44:57, ahe wrote:
> Handle not found by reporting an error?

Done.

Note I was preserving the old semantics here: if a package was not declared in
.packages, we were returning null here and reporting an error later when loading
the file.

I added the error here, but it is possible that we have some double-reporting
going on... we need to follow up on that. Added a TODO

https://codereview.chromium.org/2979003002/diff/100001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/fasta/uri_translator_impl.dart:95: } on ArgumentError
catch (_) {
On 2017/07/14 13:44:58, ahe wrote:
> Argument error can occur for many reasons. Why is it safe to catch it here?

I hear you. As discussed offline, I filed a bug on the package_config package to
use a more specific error object:
https://github.com/dart-lang/package_config/issues/40

https://codereview.chromium.org/2979003002/diff/100001/pkg/front_end/messages...
File pkg/front_end/messages.yaml (right):

https://codereview.chromium.org/2979003002/diff/100001/pkg/front_end/messages...
pkg/front_end/messages.yaml:794: template: "No input files provided to the
compiler."
On 2017/07/14 13:44:58, ahe wrote:
> files -> file.

Done.

https://codereview.chromium.org/2979003002/diff/100001/pkg/front_end/messages...
pkg/front_end/messages.yaml:854: template: "Invalid .packages file '#uri':\n 
#string."
On 2017/07/14 13:44:58, ahe wrote:
> Invalid is a word that we should avoid. How about:
> 
> "Unable to read '.packages' file:\n  #string"
> 
Done. However, now that I am catching the argument-errors from the package-uri
validation, I have a new error that says "invalid". Suggestions are welcomed.

> Also, in this case, the uri should be in the location, not the message.

Done, but let's discuss what are the invariants about location here. I had in
mind that location is a point where they can fix the issue. If the problem is in
the input file, location is the file + offset, if the problem is a flag, we
can't really use the location URI. In this case the error could be the file, or
the flag (pointing to the wrong .packages file), in which case I'm not sure I
want to use the broken uri as the location.

A different example, if an import has a URi that doesn't exist, I believe the
location should point to the import, but the message should mention the URI.

https://codereview.chromium.org/2979003002/diff/100001/pkg/front_end/test/src...
File pkg/front_end/test/src/base/processed_options_test.dart (right):

https://codereview.chromium.org/2979003002/diff/100001/pkg/front_end/test/src...
pkg/front_end/test/src/base/processed_options_test.dart:90: var expected =
Uri.parse('file:///$packageDir/a.dart');
On 2017/07/14 13:44:58, ahe wrote:
> Is this a real file or a fake URI?

All fake. Tracked in https://github.com/dart-lang/sdk/issues/30141

https://codereview.chromium.org/2979003002/diff/100001/pkg/front_end/test/src...
pkg/front_end/test/src/base/processed_options_test.dart:131:
.entityForUri(Uri.parse('file:///.packages'))
On 2017/07/14 13:44:58, ahe wrote:
> Use custom URI.
> 
> I don't like seeing error messages like this:
> 
> file:///file:///test-lib:4:11: Error: Expected an expression, but got '...'.
>   int y = ......;
>           ^
> 
> They're really confusing compared to:
> 
> org-dartlang-memory-filesystem:///test-lib:4:11: Error: Expected an
expression,
> but got '...'.
>   int y = ......;
>           ^
> 
> It's also hard to debug who's causing the URI to have a path that starts with
> file:.

tracking here https://github.com/dart-lang/sdk/issues/30141

[Siggi Cherem (dart-lang), 2017-07-14 19:54:40]

Description was changed from

==========
support resolving .packages in FE

BUG=
==========

to

==========
support resolving .packages in FE

BUG=
R=paulberry@google.com

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

[Siggi Cherem (dart-lang), 2017-07-14 19:54:42]

Committed patchset #2 (id:140001) manually as
ebd9440b18396124708cdb67bd82c1985e8a427a (presubmit successful).