Fix modifier argument detection by factoring out no-op handling

packages/debugger/lib/data/sagas/index.js

@@ -7,7 +7,8 @@ import {
   prefixName,
   stableKeccak256,
   makeAssignment,
-  makePath
+  makePath,
+  peelAwayPotentialEVMNoOp
 } from "lib/helpers";
 
 import { TICK } from "lib/trace/actions";
@@ -776,6 +777,8 @@ function* decodeMappingKeyCore(indexDefinition, keyDefinition) {
   const modifierDepth = yield select(data.current.modifierDepth);
   const inModifier = yield select(data.current.inModifier);
 
+  let peeledIndexDefinition;
+
   //why the loop? see the end of the block it heads for an explanatory
   //comment
   while (true) {
@@ -864,41 +867,19 @@ function* decodeMappingKeyCore(indexDefinition, keyDefinition) {
       }
     }
     //there's still one more reason we might have failed to decode it:
-    //certain (silent) type conversions aren't sourcemapped either.
+    //certain operations aren't sourcemapped because they're no-ops on
+    //the EVM side.  This includes, for instance, certain type conversions.
     //(thankfully, any type conversion that actually *does* something seems
     //to be sourcemapped.)  So if we've failed to decode it, we try again
-    //with the argument of the type conversion, if it is one; we leave
+    //with the argument of the no-op operation; we leave
     //indexValue undefined so the loop will continue
     //(note that this case is last for a reason; if this were earlier, it
     //would catch *non*-silent type conversions, which we want to just read
     //off the stack)
     else if (
-      indexDefinition.nodeType === "FunctionCall" &&
-      indexDefinition.kind === "typeConversion"
-    ) {
-      debug("type conversion case");
-      indexDefinition = indexDefinition.arguments[0];
-    }
-    //...also prior to 0.5.0, unary + was legal, which needs to be accounted
-    //for for the same reason
-    else if (
-      indexDefinition.nodeType === "UnaryOperation" &&
-      indexDefinition.operator === "+"
-    ) {
-      debug("unary + case");
-      indexDefinition = indexDefinition.subExpression;
-    }
-    //...and starting in 0.8.8, we'd better handle wrap and unwrap as well for
-    //the same reason!
-    else if (
-      indexDefinition.nodeType === "FunctionCall" &&
-      indexDefinition.kind === "functionCall" &&
-      ["wrap", "unwrap"].includes(
-        Codec.Ast.Utils.functionClass(indexDefinition.expression)
-      )
+      (peeledIndexDefinition = peelAwayPotentialEVMNoOp(indexDefinition))
     ) {
-      debug("wrap/unwrap case");
-      indexDefinition = indexDefinition.arguments[0];
+      indexDefinition = peeledIndexDefinition;
     }
     //otherwise, we've just totally failed to decode it, so we mark
     //indexValue as null (as distinct from undefined) to indicate this.  In

packages/debugger/lib/data/selectors/index.js

@@ -10,7 +10,8 @@ import {
   stableKeccak256,
   makePath,
   topLevelNodeTypes,
-  isTopLevelNode
+  isTopLevelNode,
+  peelAwayPotentialEVMNoOp
 } from "lib/helpers";
 
 import trace from "lib/trace/selectors";
@@ -1336,18 +1337,17 @@ const data = createSelectorTree({
         }
 
         //now: are we on the node corresponding to an argument, or, if
-        //it's a type conversion, its nested argument?
+        //it's a potential EVM no-op, its nested argument?
         if (index === undefined) {
           return false;
         }
         let argument = invocation.arguments[index];
-        while (argument.kind === "typeConversion") {
+        do {
           if (node.id === argument.id) {
             return true;
           }
-          argument = argument.arguments[0];
-        }
-        return node.id === argument.id;
+        } while ((argument = peelAwayPotentialEVMNoOp(argument)));
+        return false;
       }
     ),
 

packages/debugger/lib/helpers/index.js

@@ -1,6 +1,27 @@
 import * as Codec from "@truffle/codec";
 import stringify from "json-stable-stringify";
 
+export function peelAwayPotentialEVMNoOp(node) {
+  if (node.nodeType === "FunctionCall" && node.kind === "typeConversion") {
+    //some type conversions are no-ops;
+    //not all are, but it's not worth the trouble to detect which ones,
+    //the places where this is used no it's only checking for *potential* no-ops
+    return node.arguments[0];
+  } else if (node.nodeType === "UnaryOperation" && node.operator === "+") {
+    //prior to 0.5.0, unary + was legal, which was a no-op
+    return node.subExpression;
+  } else if (
+    node.nodeType === "FunctionCall" &&
+    node.kind === "functionCall" &&
+    ["wrap", "unwrap"].includes(Codec.Ast.Utils.functionClass(node.expression))
+  ) {
+    //starting in 0.8.8, there are wrap and unwrap which are no-ops
+    return node.arguments[0];
+  } else {
+    return null;
+  }
+}
+
 /** AST node types that are skipped by stepNext() to filter out some noise */
 export function isDeliberatelySkippedNodeType(node) {
   const skippedTypes = [