Deprecate module-root option in dartdevc, part of #32272

Adds an option for specifying the output JS module name if that is
needed (only applies for some module formats).

Also removes repl-compile (it's set via API, not the command line).

Refactors dartdevk options to match dartdevc so we can migrate more
easily. Moves shared code into a shared location and removes copied
code.

Change-Id: I966343ecbbc962f5d0f14ea7e65d78660159f420
Reviewed-on: https://dart-review.googlesource.com/64823
Commit-Queue: Jenny Messerly <[email protected]>
Reviewed-by: Bob Nystrom <[email protected]>

Author: Jenny Messerly

pkg/dev_compiler/lib/src/analyzer/code_generator.dart

@@ -559,9 +559,11 @@ class CodeGenerator extends Object
     if (source.uri.scheme == 'dart') {
       return JS.dartSdkModule;
     }
-    var moduleName = _buildUnit.libraryToModule(source);
+    var summaryPath = (source as InSummarySource).summaryPath;
+    var moduleName = options.summaryModules[summaryPath];
     if (moduleName == null) {
-      throw StateError('Could not find module containing "$library".');
+      throw StateError('Could not find module name for library "$library" '
+          'from summary path "$summaryPath".');
     }
     return moduleName;
   }

pkg/dev_compiler/lib/src/analyzer/command.dart

@@ -5,14 +5,12 @@
 import 'dart:io';
 import 'package:analyzer/src/command_line/arguments.dart'
     show defineAnalysisArguments, ignoreUnrecognizedFlagsFlag;
-import 'package:analyzer/src/generated/source.dart' show Source;
 import 'package:analyzer/src/summary/package_bundle_reader.dart'
-    show ConflictingSummaryException, InSummarySource;
+    show ConflictingSummaryException;
 import 'package:args/args.dart' show ArgParser, ArgResults;
 import 'package:args/command_runner.dart' show UsageException;
 import 'package:path/path.dart' as path;
 
-import '../compiler/module_builder.dart';
 import 'context.dart' show AnalyzerOptions;
 import 'module_compiler.dart' show BuildUnit, CompilerOptions, ModuleCompiler;
 
@@ -58,7 +56,7 @@ int compile(List<String> args, {void printFn(Object obj)}) {
     return 0;
   } on UsageException catch (error) {
     // Incorrect usage, input file not found, etc.
-    printFn(error);
+    printFn('${error.message}\n\n$_usageMessage');
     return 64;
   } on ConflictingSummaryException catch (error) {
     // Same input file appears in multiple provided summaries.
@@ -106,14 +104,14 @@ ArgParser ddcArgParser({bool hide = true}) {
         defaultsTo: false,
         hide: hide)
     ..addMultiOption('out', abbr: 'o', help: 'Output file (required).')
-    ..addOption('module-root',
-        help: 'Root module directory. Module paths are relative to this root.')
+    ..addOption('module-name',
+        help: 'The output module name, used in some JS module formats.\n'
+            'Defaults to the output file name (without .js).')
     ..addOption('library-root',
         help: 'Root of source files. Library names are relative to this root.');
+  CompilerOptions.addArguments(argParser, hide: hide);
   defineAnalysisArguments(argParser, hide: hide, ddc: true);
-  addModuleFormatOptions(argParser, allowMultiple: true, hide: hide);
   AnalyzerOptions.addArguments(argParser, hide: hide);
-  CompilerOptions.addArguments(argParser, hide: hide);
   return argParser;
 }
 
@@ -131,23 +129,17 @@ void _compile(ArgResults argResults, AnalyzerOptions analyzerOptions,
   var compiler = ModuleCompiler(analyzerOptions);
   var compilerOpts = CompilerOptions.fromArguments(argResults);
   var outPaths = argResults['out'] as List<String>;
-  var moduleFormats = parseModuleFormatOption(argResults);
-  bool singleOutFile = argResults['single-out-file'];
-  if (singleOutFile) {
-    for (var format in moduleFormats) {
-      if (format != ModuleFormat.amd && format != ModuleFormat.legacy) {
-        _usageException('Format $format cannot be combined with '
-            'single-out-file. Only amd and legacy modes are supported.');
-      }
-    }
-  }
-
+  var moduleFormats = compilerOpts.moduleFormats;
   if (outPaths.isEmpty) {
-    _usageException('Please include the output file location. For example:\n'
-        '    -o PATH/TO/OUTPUT_FILE.js');
+    throw UsageException(
+        'Please specify the output file location. For example:\n'
+        '    -o PATH/TO/OUTPUT_FILE.js',
+        '');
   } else if (outPaths.length != moduleFormats.length) {
-    _usageException('Number of output files (${outPaths.length}) must match '
-        'number of module formats (${moduleFormats.length}).');
+    throw UsageException(
+        'Number of output files (${outPaths.length}) must match '
+        'number of module formats (${moduleFormats.length}).',
+        '');
   }
 
   // TODO(jmesserly): for now the first one is special. This will go away once
@@ -160,27 +152,20 @@ void _compile(ArgResults argResults, AnalyzerOptions analyzerOptions,
   } else {
     libraryRoot = Directory.current.path;
   }
-  var moduleRoot = argResults['module-root'] as String;
-  String modulePath;
-  if (moduleRoot != null) {
-    moduleRoot = path.absolute(moduleRoot);
-    if (!path.isWithin(moduleRoot, firstOutPath)) {
-      _usageException('Output file $firstOutPath must be within the module '
-          'root directory $moduleRoot');
+  var moduleName = argResults['module-name'] as String;
+  if (moduleName == null) {
+    var moduleRoot = compilerOpts.moduleRoot;
+    if (moduleRoot != null) {
+      // TODO(jmesserly): remove this legacy support after a deprecation period.
+      // (Mainly this is to give time for migrating build rules.)
+      moduleName =
+          path.withoutExtension(path.relative(firstOutPath, from: moduleRoot));
+    } else {
+      moduleName = path.basenameWithoutExtension(firstOutPath);
     }
-    modulePath =
-        path.withoutExtension(path.relative(firstOutPath, from: moduleRoot));
-  } else {
-    moduleRoot = path.dirname(firstOutPath);
-    modulePath = path.basenameWithoutExtension(firstOutPath);
   }
 
-  var unit = BuildUnit(
-      modulePath,
-      libraryRoot,
-      argResults.rest,
-      (source) =>
-          _moduleForLibrary(moduleRoot, source, analyzerOptions, compilerOpts));
+  var unit = BuildUnit(moduleName, libraryRoot, argResults.rest);
 
   var module = compiler.compile(unit, compilerOpts);
   module.errors.forEach(printFn);
@@ -193,8 +178,7 @@ void _compile(ArgResults argResults, AnalyzerOptions analyzerOptions,
 
   // Write JS file, as well as source map and summary (if requested).
   for (var i = 0; i < outPaths.length; i++) {
-    module.writeCodeSync(moduleFormats[i], outPaths[i],
-        singleOutFile: singleOutFile);
+    module.writeCodeSync(moduleFormats[i], outPaths[i]);
   }
   if (module.summaryBytes != null) {
     var summaryPaths = compilerOpts.summaryOutPath != null
@@ -216,34 +200,6 @@ void _compile(ArgResults argResults, AnalyzerOptions analyzerOptions,
   }
 }
 
-String _moduleForLibrary(String moduleRoot, Source source,
-    AnalyzerOptions analyzerOptions, CompilerOptions compilerOpts) {
-  if (source is InSummarySource) {
-    var summaryPath = source.summaryPath;
-
-    if (analyzerOptions.customSummaryModules.containsKey(summaryPath)) {
-      return analyzerOptions.customSummaryModules[summaryPath];
-    }
-
-    var ext = '.${compilerOpts.summaryExtension}';
-    if (path.isWithin(moduleRoot, summaryPath) && summaryPath.endsWith(ext)) {
-      var buildUnitPath =
-          summaryPath.substring(0, summaryPath.length - ext.length);
-      return path.url
-          .joinAll(path.split(path.relative(buildUnitPath, from: moduleRoot)));
-    }
-
-    _usageException('Imported file ${source.uri} is not within the module root '
-        'directory $moduleRoot');
-  }
-
-  _usageException(
-      'Imported file "${source.uri}" was not found as a summary or source '
-      'file. Please pass in either the summary or the source file '
-      'for this import.');
-  return null; // unreachable
-}
-
 String get _usageMessage =>
     'The Dart Development Compiler compiles Dart sources into a JavaScript '
     'module.\n\n'
@@ -262,10 +218,6 @@ String _getVersion() {
   }
 }
 
-void _usageException(String message) {
-  throw UsageException(message, _usageMessage);
-}
-
 /// Thrown when the input source code has errors.
 class CompileErrorException implements Exception {
   toString() => '\nPlease fix all errors before compiling (warnings are okay).';

pkg/dev_compiler/lib/src/analyzer/context.dart

@@ -31,11 +31,6 @@ class AnalyzerOptions {
   /// Package root when resolving 'package:' urls the standard way.
   String get packageRoot => contextBuilderOptions.defaultPackagesDirectoryPath;
 
-  /// List of summary file paths.
-  final List<String> summaryPaths;
-
-  final Map<String, String> customSummaryModules = {};
-
   /// Path to the dart-sdk, or `null` if the path couldn't be determined.
   final String dartSdkPath;
 
@@ -49,63 +44,49 @@ class AnalyzerOptions {
 
   AnalyzerOptions._(
       {this.contextBuilderOptions,
-      List<String> summaryPaths,
       String dartSdkPath,
       this.customUrlMappings = const {}})
-      : dartSdkPath = dartSdkPath ?? getSdkDir().path,
-        summaryPaths = summaryPaths ?? const [] {
+      : dartSdkPath = dartSdkPath ?? getSdkDir().path {
     contextBuilderOptions.declaredVariables ??= const {};
-    _parseCustomSummaryModules();
   }
 
   factory AnalyzerOptions.basic(
-      {String dartSdkPath,
-      String dartSdkSummaryPath,
-      List<String> summaryPaths}) {
-    var contextBuilderOptions = ContextBuilderOptions()
-      ..defaultOptions = (AnalysisOptionsImpl()..previewDart2 = true)
-      ..dartSdkSummaryPath = dartSdkSummaryPath;
-
+      {String dartSdkPath, String dartSdkSummaryPath}) {
     return AnalyzerOptions._(
-        contextBuilderOptions: contextBuilderOptions,
-        dartSdkPath: dartSdkPath,
-        summaryPaths: summaryPaths);
+        contextBuilderOptions: ContextBuilderOptions()
+          ..defaultOptions = (AnalysisOptionsImpl()..previewDart2 = true)
+          ..dartSdkSummaryPath = dartSdkSummaryPath,
+        dartSdkPath: dartSdkPath);
   }
 
   factory AnalyzerOptions.fromArguments(ArgResults args,
-      {String dartSdkSummaryPath, List<String> summaryPaths}) {
-    var contextBuilderOptions =
+      {String dartSdkSummaryPath}) {
+    var contextOpts =
         createContextBuilderOptions(args, trackCacheDependencies: false);
-    (contextBuilderOptions.defaultOptions as AnalysisOptionsImpl).previewDart2 =
-        true;
+    (contextOpts.defaultOptions as AnalysisOptionsImpl).previewDart2 = true;
 
     var dartSdkPath = args['dart-sdk'] as String ?? getSdkDir().path;
 
-    dartSdkSummaryPath ??= contextBuilderOptions.dartSdkSummaryPath;
-    dartSdkSummaryPath ??=
+    dartSdkSummaryPath ??= contextOpts.dartSdkSummaryPath ??
         path.join(dartSdkPath, 'lib', '_internal', 'ddc_sdk.sum');
     // For building the SDK, we explicitly set the path to none.
     if (dartSdkSummaryPath == 'build') dartSdkSummaryPath = null;
-    contextBuilderOptions.dartSdkSummaryPath = dartSdkSummaryPath;
+    contextOpts.dartSdkSummaryPath = dartSdkSummaryPath;
 
     return AnalyzerOptions._(
-        contextBuilderOptions: contextBuilderOptions,
-        summaryPaths: summaryPaths ?? args['summary'] as List<String>,
+        contextBuilderOptions: contextOpts,
         dartSdkPath: dartSdkPath,
         customUrlMappings:
             _parseUrlMappings(args['url-mapping'] as List<String>));
   }
 
   static void addArguments(ArgParser parser, {bool hide = true}) {
-    parser
-      ..addOption('summary',
-          abbr: 's', help: 'summary file(s) to include', allowMultiple: true)
-      ..addOption('url-mapping',
-          help: '--url-mapping=libraryUri,/path/to/library.dart uses\n'
-              'library.dart as the source for an import of of "libraryUri".',
-          allowMultiple: true,
-          splitCommas: false,
-          hide: hide);
+    parser.addOption('url-mapping',
+        help: '--url-mapping=libraryUri,/path/to/library.dart uses\n'
+            'library.dart as the source for an import of of "libraryUri".',
+        allowMultiple: true,
+        splitCommas: false,
+        hide: hide);
   }
 
   static Map<String, String> _parseUrlMappings(List<String> argument) {
@@ -118,23 +99,6 @@ class AnalyzerOptions {
     }
     return mappings;
   }
-
-  /// A summary path can contain "=" followed by an explicit module name to
-  /// allow working with summaries whose physical location is outside of the
-  /// module root directory.
-  ///
-  /// Removes any explicit module names from [summaryPaths] and populates with
-  /// [customSummaryModules] with them.
-  void _parseCustomSummaryModules() {
-    for (var i = 0; i < summaryPaths.length; i++) {
-      var summaryPath = summaryPaths[i];
-      var pipe = summaryPath.indexOf("=");
-      if (pipe != -1) {
-        summaryPaths[i] = summaryPath.substring(0, pipe);
-        customSummaryModules[summaryPaths[i]] = summaryPath.substring(pipe + 1);
-      }
-    }
-  }
 }
 
 /// Creates a SourceFactory configured by the [options].
@@ -147,19 +111,17 @@ SourceFactory createSourceFactory(AnalyzerOptions options,
     SummaryDataStore summaryData,
     ResourceProvider resourceProvider}) {
   resourceProvider ??= PhysicalResourceProvider.INSTANCE;
-  var resolvers = <UriResolver>[];
+  var resolvers = <UriResolver>[sdkResolver];
   if (options.customUrlMappings.isNotEmpty) {
     resolvers
         .add(CustomUriResolver(resourceProvider, options.customUrlMappings));
   }
-  resolvers.add(sdkResolver);
   if (summaryData != null) {
     resolvers.add(InSummaryUriResolver(resourceProvider, summaryData));
   }
 
-  if (fileResolvers == null)
-    fileResolvers =
-        createFileResolvers(options, resourceProvider: resourceProvider);
+  fileResolvers ??=
+      createFileResolvers(options, resourceProvider: resourceProvider);
   resolvers.addAll(fileResolvers);
   return SourceFactory(resolvers, null, resourceProvider);
 }