refactor(parseOptions): better handle errors (#674)

* refactor(parseOptions): better handle errors

For the case where `parseOptions()` is passed a string that does not
start with a hyphen, we should reject this string instead of returning
the same value. This has no change on any other test cases, and should
not affect any commands since we are careful about what input we pass to
`parseOptions()`

This also adjust how we were handling error cases in the function. We
were previously using two different calls to `common.error()`, one for
if `errorOptions` is passed, and one without. This hurts readability,
because it reads like "if we have errorOptions, then this is an error,
and if we don't then it's also an error," instead of the more
appropriate, "we reached an error case and should use errorOptions if
it's available, otherwise we should signal an error without using it."

We do not need to use `errorOptions` for the added call to `error()`,
since this is an error which indicates misuse, and should not be
recoverable.

This adds a test case as well, for a line which was not previously
covered in our tests.

Partial fix for #671

* Refactor parseOptions()

This validates input at the top of `parseOptions()` for type
correctness. This changes `typeof x === 'object'` to `x instanceOf
Object` to improve robustness. Now we can safely use `errorOptions`
knowing that if it has a truthy value, it will be an object, otherwise
we should use defaults instead.

The new tests ensure that we have 100% unit test coverage for this
function.

* Use exceptions, not common.error()s, for internal misuse

* Add common.isObject() helper function

The `isObject()` helper is used throughout common.js to reliably test if
variables are JavaScript objects.

src/common.js

@@ -72,6 +72,12 @@ exports.platform = platform;
 // This is populated by calls to commonl.wrap()
 var pipeMethods = [];
 
+// Reliably test if something is any sort of javascript object
+function isObject(a) {
+  return typeof a === 'object' && a !== null;
+}
+exports.isObject = isObject;
+
 function log() {
   /* istanbul ignore next */
   if (!config.silent) {
@@ -103,9 +109,9 @@ function error(msg, _code, options) {
     silent: false,
   };
 
-  if (typeof _code === 'number' && typeof options === 'object') {
+  if (typeof _code === 'number' && isObject(options)) {
     options.code = _code;
-  } else if (typeof _code === 'object') { // no 'code'
+  } else if (isObject(_code)) { // no 'code'
     options = _code;
   } else if (typeof _code === 'number') { // no 'options'
     options = { code: _code };
@@ -184,58 +190,57 @@ exports.getUserHome = getUserHome;
 // Returns {'reference': 'string-value', 'bob': false} when passed two dictionaries of the form:
 //   parseOptions({'-r': 'string-value'}, {'r':'reference', 'b':'bob'});
 function parseOptions(opt, map, errorOptions) {
-  if (!map) error('parseOptions() internal error: no map given');
+  // Validate input
+  if (typeof opt !== 'string' && !isObject(opt)) {
+    throw new Error('options must be strings or key-value pairs');
+  } else if (!isObject(map)) {
+    throw new Error('parseOptions() internal error: map must be an object');
+  } else if (errorOptions && !isObject(errorOptions)) {
+    throw new Error('parseOptions() internal error: errorOptions must be object');
+  }
 
   // All options are false by default
   var options = {};
   Object.keys(map).forEach(function (letter) {
-    if (map[letter][0] !== '!') {
-      options[map[letter]] = false;
+    var optName = map[letter];
+    if (optName[0] !== '!') {
+      options[optName] = false;
     }
   });
 
-  if (!opt) return options; // defaults
+  if (opt === '') return options; // defaults
 
-  var optionName;
   if (typeof opt === 'string') {
     if (opt[0] !== '-') {
-      return options;
+      error("Options string must start with a '-'", errorOptions || {});
     }
 
     // e.g. chars = ['R', 'f']
     var chars = opt.slice(1).split('');
 
     chars.forEach(function (c) {
       if (c in map) {
-        optionName = map[c];
+        var optionName = map[c];
         if (optionName[0] === '!') {
           options[optionName.slice(1)] = false;
         } else {
           options[optionName] = true;
         }
-      } else if (typeof errorOptions === 'object') {
-        error('option not recognized: ' + c, errorOptions);
       } else {
-        error('option not recognized: ' + c);
+        error('option not recognized: ' + c, errorOptions || {});
       }
     });
-  } else if (typeof opt === 'object') {
+  } else { // opt is an Object
     Object.keys(opt).forEach(function (key) {
       // key is a string of the form '-r', '-d', etc.
       var c = key[1];
       if (c in map) {
-        optionName = map[c];
+        var optionName = map[c];
         options[optionName] = opt[key]; // assign the given value
-      } else if (typeof errorOptions === 'object') {
-        error('option not recognized: ' + c, errorOptions);
       } else {
-        error('option not recognized: ' + c);
+        error('option not recognized: ' + c, errorOptions || {});
       }
     });
-  } else if (typeof errorOptions === 'object') {
-    error('options must be strings or key-value pairs', errorOptions);
-  } else {
-    error('options must be strings or key-value pairs');
   }
   return options;
 }
@@ -328,7 +333,7 @@ function wrap(cmd, fn, options) {
       if (options.unix === false) { // this branch is for exec()
         retValue = fn.apply(this, args);
       } else { // and this branch is for everything else
-        if (args[0] instanceof Object && args[0].constructor.name === 'Object') {
+        if (isObject(args[0]) && args[0].constructor.name === 'Object') {
           // a no-op, allowing the syntax `touch({'-r': file}, ...)`
         } else if (args.length === 0 || typeof args[0] !== 'string' || args[0].length <= 1 || args[0][0] !== '-') {
           args.unshift(''); // only add dummy option if '-option' not already present
@@ -348,7 +353,7 @@ function wrap(cmd, fn, options) {
 
         // Convert ShellStrings (basically just String objects) to regular strings
         args = args.map(function (arg) {
-          if (arg instanceof Object && arg.constructor.name === 'String') {
+          if (isObject(arg) && arg.constructor.name === 'String') {
             return arg.toString();
           }
           return arg;
@@ -371,7 +376,7 @@ function wrap(cmd, fn, options) {
 
         try {
           // parse options if options are provided
-          if (typeof options.cmdOptions === 'object') {
+          if (isObject(options.cmdOptions)) {
             args[0] = parseOptions(args[0], options.cmdOptions);
           }
 

test/common.js

@@ -37,6 +37,52 @@ test('parseOptions (invalid option in options object)', t => {
   });
 });
 
+test('parseOptions (without a hyphen in the string)', t => {
+  t.throws(() => {
+    common.parseOptions('f', {
+      f: 'force',
+    });
+  });
+});
+
+test('parseOptions (opt is not a string/object)', t => {
+  t.throws(() => {
+    common.parseOptions(1, {
+      f: 'force',
+    });
+  });
+});
+
+test('parseOptions (map is not an object)', t => {
+  t.throws(() => {
+    common.parseOptions('-f', 27);
+  });
+});
+
+test('parseOptions (errorOptions is not an object)', t => {
+  t.throws(() => {
+    common.parseOptions('-f', {
+      f: 'force',
+    }, 'not a valid errorOptions');
+  });
+});
+
+test('parseOptions (unrecognized string option)', t => {
+  t.throws(() => {
+    common.parseOptions('-z', {
+      f: 'force',
+    });
+  });
+});
+
+test('parseOptions (unrecognized option in Object)', t => {
+  t.throws(() => {
+    common.parseOptions({ '-c': 7 }, {
+      f: 'force',
+    });
+  });
+});
+
 test('parseOptions (invalid type)', t => {
   t.throws(() => {
     common.parseOptions(12, {