update DDC and analyzer cli preprocessArgs

pkg/analyzer/lib/src/command_line/arguments.dart

Index: pkg/analyzer/lib/src/command_line/arguments.dart
diff --git a/pkg/analyzer/lib/src/command_line/arguments.dart b/pkg/analyzer/lib/src/command_line/arguments.dart
index 939623d9d8ccda6a3280c8e1127d4faa7c1e03af..9b6e9f81f69d57a9fed1a88de6fd72d9f490739c 100644
--- a/pkg/analyzer/lib/src/command_line/arguments.dart
+++ b/pkg/analyzer/lib/src/command_line/arguments.dart
@@ -227,10 +227,13 @@ ArgResults parse(
 }
 
 /**
- * Preprocess the given list of command line [args] by checking whether the real
- * arguments are in a file (Bazel worker mode).
+ * Preprocess the given list of command line [args].
+ * If the final arg is `@file_path` (Bazel worker mode),
+ * then read in all the lines of that file and add those as args.
+ * Always returns a new modifiable list.
  */
 List<String> preprocessArgs(ResourceProvider provider, List<String> args) {
+  args = new List.from(args);

[Brian Wilkerson, 2016/12/13 18:48:20]

Is there a reason for creating a copy of the args if they don't change?

[danrubel, 2016/12/13 18:50:08]

Yes. DDC expects that.

[Brian Wilkerson, 2016/12/13 19:02:16]

I was looking for an answer to the question of "why is this a necessary
invariant for this method?". I'm guessing that the next level up (the answer to
"why does DDC expect that") is "because they modify it", but I don't understand
why they would modify the list.

At some level it doesn't really matter because this is a one-time operation, so
copying the list needlessly isn't going to have any measurable impact on
performance, but I'm curious to understand the rationale.

That said, it seems like the reason should be given in the comment so that
someone doesn't break them later by deciding that we don't need to copy the
list.

[danrubel, 2016/12/13 19:09:13]

Yes, they modify it :-)

I can revert this code change so that it does not copy the list and instead copy
the list in the DDC command line processing. That would push the invariant into
the code that uses it.

Which would you prefer?

[vsm, 2016/12/13 19:14:33]

Either is fine by me.

[Brian Wilkerson, 2016/12/13 19:18:33]

Modifying the list seems strange, but given that, I guess I don't have a strong
preference. Not modifying the list would mean that DDC would sometimes end up
copying the list twice; copying the list means that CLI will sometimes copy the
list when it doesn't need to. Unfortunate either way, but not a big deal.

   if (args.isEmpty) {
     return args;
   }
@@ -238,16 +241,15 @@ List<String> preprocessArgs(ResourceProvider provider, List<String> args) {
   if (lastArg.startsWith('@')) {
     File argsFile = provider.getFile(lastArg.substring(1));
     try {
-      List<String> newArgs = args.sublist(0, args.length - 1).toList();
-      newArgs.addAll(argsFile
+      args.removeLast();
+      args.addAll(argsFile
           .readAsStringSync()
           .replaceAll('\r\n', '\n')
           .replaceAll('\r', '\n')
           .split('\n')
           .where((String line) => line.isNotEmpty));
-      return newArgs;
-    } on FileSystemException {
-      // Don't modify args if the file does not exist or cannot be read.
+    } on FileSystemException catch (e) {
+      throw new Exception('Failed to read file specified by $lastArg : $e');

[Brian Wilkerson, 2016/12/13 19:02:16]

Also, it isn't clear to me that it helps to wrap the exception. Why not just let
it be thrown (if we're not going to ignore it).

[danrubel, 2016/12/13 19:09:13]

I wanted it to be clear that the @filePath was the cause of the file exception.
It was not obvious to me that the raw file exception would communicate that
clearly. The path in the FileException might differ from the @filePath argument
specified by the user depending upon what is returned by the resource provider.

[Brian Wilkerson, 2016/12/13 19:18:33]

Ah. Makes sense.

     }
   }
   return args;