Not dart: or package: URIs cause exception

[scheglov]

import 'abc:lib';

scheglov@scheglov-macbookpro4:~/Source/Dart/sdk.git/sdk (aa-kernel-FrontEndCompiler2)$ pkg/front_end/tool/fasta compile /Users/scheglov/dart/test/bin/test.dart
abc:lib: Error: PhysicalFileSystem only supports file:* URIs

And the same issue causes problems in Analyzer integration.

This happens because Loader.read translates only dart or package URIs and keep all other URIs as if they were file URIs.

[sigmundch]

From the IDE picture, I believe what you need is support for dart-ext:* URIs, which is tracked in #31618.

I have made changes in the past to remove any knowledge in fasta about non-dart and non-package schemes. We used to assume they were "file:" URIs incorrectly, instead we changed it to not assume anything and delegate to the underlying FileSystem to figure it out. Now any errors about unknown schemes comes from the underlying file system.

By default we are running the frontend with a PhisicalFileSystem, for analyzer (and for the vm) this needs to be changed to use a file system that supports:

• file URIs
• data URIs
• dart-ext URIs

See also my comment on this other bug about data:* URIs, as I describe how I believe we should compose these file-systems.

If you believe #31618 is tracking what you need, we can probably close this bug as a duplicate.

Beyond supporting dart-ext, the only thing I think we might want to do is to improve the error messages *(either change the default error or catch it and produce something the end user can understand)

[scheglov]

On the Analyzer level it does not matter whether the URI is dart-ext or something completely random, like ht:. If the URI scheme is not supported, the compiler should not throw an exception, just an error should be reported. Users will type random URIs :-)

As it is now, the FileSystem API states that it will throw an exception, if the URI scheme is not supported. We need either handle the exception in front-end, or change API to returning null and again handle this in front-end.

[peter-ahe-google]

Please make sure to include the following information in future bug reports:

• What are you trying to do?

• What did you do? (This means a self contained reproducible command line that can be run by someone without access to your computer).

• What happened? (This means include the text output of your program, not a screen shot from your IDE).

• What did you expect to happen?

[scheglov]

1. I'm integrating resolution from FrontEnd into Analyzer.

2. I call FrontEnd to compile a file that have an import with a URI with a non-standard scheme. See the example in the first message. Specifically I call KernelTarget.buildOutline() and KernelTarget.buildProgram(). That's what I always do in Analyzer integration :-)

3. I get an exception from FrontEnd.

  Invalid argument(s): Only file:// URIs are supported, but dart-ext:x is given.
  #0      _FileSystemAdaptor.entityForUri (package:analyzer/src/dart/analysis/frontend_resolution.dart:485:7)
  #1      SourceLoader.tokenize (package:front_end/src/fasta/source/source_loader.dart:136:47)
  <asynchronous suspension>
  #2      SourceLoader.buildOutline (package:front_end/src/fasta/source/source_loader.dart:176:26)
  <asynchronous suspension>
  #3      Loader.buildOutlines (package:front_end/src/fasta/loader.dart:162:13)
  <asynchronous suspension>
  #4      KernelTarget.buildOutlines (package:front_end/src/fasta/kernel/kernel_target.dart:226:20)
  <asynchronous suspension>
  #5      FrontEndCompiler.compile.<anonymous closure>.<anonymous closure> (package:analyzer/src/dart/analysis/frontend_resolution.dart:246:30)
  <asynchronous suspension>
  #6      PerformanceLog.runAsync (package:front_end/src/base/performance_logger.dart:52:21)
  <asynchronous suspension>
  #7      FrontEndCompiler.compile.<anonymous closure> (package:analyzer/src/dart/analysis/frontend_resolution.dart:245:34)

The method _FileSystemAdaptor.entityForUri behaves according to the documentation, it throws an exception because it supports only the file scheme. But this does not mean that FrontEnd should passthrough this exception to the client.

4. I expect that there are no exceptions thrown by FrontEnd during compilation. If there is an error, it should be reported via onError. If it up to the client how to handle this exception - report to the user, throw an exception and terminate compilation, etc.

@kmillikin Please don't close issues on formal grounds.

[peter-ahe-google]

You get an exception from package:analyzer/src/dart/analysis/frontend_resolution.dart:485:7 because it throws the wrong exception.

As far as I can tell, you wrote this code yourself: https://github.com/dart-lang/sdk/blame/master/pkg/analyzer/lib/src/dart/analysis/frontend_resolution.dart#L485

[scheglov]

The FileSystem implementation in front_resolution.dart does what is specified.

  /// Returns a [FileSystemEntity] corresponding to the given [uri].
  ///
  /// Uses of `..` and `.` in the URI are normalized before returning.
  ///
  /// If the URI scheme is not supported by this file system, an [Error] will be
  /// thrown.
  ///
  /// Does not check whether a file or folder exists at the given location.
  FileSystemEntity entityForUri(Uri uri);

And actually there is no good way for any implementation to support random schemes. If it is not supported, what should it do? Return empty string? How FrontEnd will know that is not supported, so that an error should be reported?

@kmillikin @stefantsov Can you please triage this issue?

[peter-ahe-google]

The comment is wrong, and the API is clearly marked as unstable. You have to throw FileSystemException as I've told you in past code reviews. Please stop this nonsense.

[scheglov]

Did you? I cannot find this :-(

Yes, throwing FileSystemException moves us forward, thank you.
Not that it makes a lot of difference for Analyzer - we still get a generic exception throwing main, without resolution results for the file with the invalid import, and everything that depends on it. That's not a behavior we want to provide to Analysis Server customers.

library;
import self as self;

static method #main() → dynamic {
  throw "dart-ext:x: Error: Only file:// URIs are supported, but dart-ext:x is given.";
}

[peter-ahe-google]

Why did you reopen this bug? Because of the #main method? If so, we don't need anymore bugs tracking that problem.

[scheglov]

I understand that we are all aware that a generic exception throwing main is not something we want to live with. So, you are right, we don't need another issue stating this.

But this is a specific situation - invalid URI makes FrontEnd return not useful results. And I think it is useful to track it separately, because a fix for this problem is also specific.

There is no something generic that can fix all main problems.

[peter-ahe-google]

I was probably thinking about the conversation that led you to implement https://codereview.chromium.org/2864183002. However, it seems like we forgot about entityForUri in that CL. My apologies.

Actually, my plan is to completely get rid of the bad #main. I know it doesn't work for the analyzer, and I know it is a problem for incremental compilation.

So as far as I'm concerned, we need to remove KernelTarget.erroneousProgram and consider it a P1 issue.

[peter-ahe-google]

The issue with #main is tracked in #31833.