Bug 1566057: Convert Detecting function to use bytecode iterator. r=tcampbell

Differential Revision: https://phabricator.services.mozilla.com/D42611

--HG--
extra : moz-landing-system : lando

js/src/vm/BytecodeLocation-inl.h

@@ -26,6 +26,12 @@ inline uint32_t BytecodeLocation::bytecodeToOffset(JSScript* script) {
   return script->pcToOffset(this->rawBytecode_);
 }
 
+inline PropertyName* BytecodeLocation::getPropertyName(
+    const JSScript* script) const {
+  MOZ_ASSERT(this->isValid());
+  return script->getName(this->rawBytecode_);
+}
+
 }  // namespace js
 
 #endif

js/src/vm/BytecodeLocation.h

@@ -14,6 +14,8 @@ namespace js {
 
 typedef uint32_t RawBytecodeLocationOffset;
 
+class PropertyName;
+
 class BytecodeLocationOffset {
   RawBytecodeLocationOffset rawOffset_;
 
@@ -69,6 +71,8 @@ class BytecodeLocation {
 
   uint32_t bytecodeToOffset(JSScript* script);
 
+  PropertyName* getPropertyName(const JSScript* script) const;
+
   bool operator==(const BytecodeLocation& other) const {
     MOZ_ASSERT(this->debugOnlyScript_ == other.debugOnlyScript_);
     return rawBytecode_ == other.rawBytecode_;
@@ -120,6 +124,16 @@ class BytecodeLocation {
 
   uint32_t icIndex() const { return GET_ICINDEX(rawBytecode_); }
 
+  bool isEqualityOp() const { return IsEqualityOp(getOp()); }
+
+  bool isStrictEqualityOp() const {
+    return is(JSOP_STRICTEQ) || is(JSOP_STRICTNE);
+  }
+
+  bool isDetectingOp() const { return IsDetecting(getOp()); }
+
+  bool isNameOp() const { return IsNameOp(getOp()); }
+
   // Accessors:
   JSOp getOp() const { return JSOp(*rawBytecode_); }
 

js/src/vm/BytecodeUtil.h

@@ -535,6 +535,10 @@ inline bool IsCheckStrictOp(JSOp op) {
   return CodeSpec[op].format & JOF_CHECKSTRICT;
 }
 
+inline bool IsDetecting(JSOp op) { return CodeSpec[op].format & JOF_DETECTING; }
+
+inline bool IsNameOp(JSOp op) { return CodeSpec[op].format & JOF_NAME; }
+
 #ifdef DEBUG
 inline bool IsCheckSloppyOp(JSOp op) {
   return CodeSpec[op].format & JOF_CHECKSLOPPY;

js/src/vm/NativeObject.cpp

@@ -21,6 +21,7 @@
 
 #include "gc/Nursery-inl.h"
 #include "vm/ArrayObject-inl.h"
+#include "vm/BytecodeLocation-inl.h"
 #include "vm/EnvironmentObject-inl.h"
 #include "vm/JSObject-inl.h"
 #include "vm/JSScript-inl.h"
@@ -2333,56 +2334,67 @@ bool js::NativeGetExistingProperty(JSContext* cx, HandleObject receiver,
 
 /*
  * Given pc pointing after a property accessing bytecode, return true if the
- * access is "property-detecting" -- that is, if we shouldn't warn about it
- * even if no such property is found and strict warnings are enabled.
+ * access is "property-detecting". Caller will use this value to determine
+ * whether or not to warn that an undefined object property *may* be used in
+ * a way that the programmer does not expect. In other words, we do not want
+ * to warn the programmer if he/she/they are doing something like
+ *
+ * if (obj.property) { }
+ * or
+ * if (obj.property == undefined) {}
+ * or
+ * if (obj.property == null) {}
  */
 static bool Detecting(JSContext* cx, JSScript* script, jsbytecode* pc) {
   MOZ_ASSERT(script->containsPC(pc));
 
-  // Skip jump target and dup opcodes.
-  while (pc < script->codeEnd() &&
-         (BytecodeIsJumpTarget(JSOp(*pc)) || JSOp(*pc) == JSOP_DUP))
-    pc = GetNextPc(pc);
+  BytecodeIterator scriptIterator =
+      BytecodeIterator(BytecodeLocation(script, pc));
+  BytecodeIterator endIter = BytecodeIterator(script->endLocation());
 
-  MOZ_ASSERT(script->containsPC(pc));
-  if (pc >= script->codeEnd()) {
-    return false;
+  // Skip over jump targets and duplication operations.
+  while (scriptIterator->isJumpTarget() || scriptIterator->is(JSOP_DUP)) {
+    if (++scriptIterator == endIter) {
+      // If we are at the end of the script, we cannot be detecting
+      // the property.
+      return false;
+    }
   }
 
-  // General case: a branch or equality op follows the access.
-  JSOp op = JSOp(*pc);
-  if (CodeSpec[op].format & JOF_DETECTING) {
+  // General case: Do not warn if the operation branches on or tests
+  // the equality of the property.
+  if (scriptIterator->isDetectingOp()) {
     return true;
   }
 
-  jsbytecode* endpc = script->codeEnd();
-
-  if (op == JSOP_NULL) {
-    // Special case #1: don't warn about (obj.prop == null).
-    if (++pc < endpc) {
-      op = JSOp(*pc);
-      return op == JSOP_EQ || op == JSOP_NE;
+  // Special case: Do not warn if we are checking whether the property is null.
+  if (scriptIterator->is(JSOP_NULL)) {
+    if (++scriptIterator == endIter) {
+      return false;
     }
-    return false;
+    return scriptIterator->isEqualityOp() &&
+           !scriptIterator->isStrictEqualityOp();
   }
 
-  // Special case #2: don't warn about (obj.prop == undefined).
-  if (op == JSOP_GETGNAME || op == JSOP_GETNAME) {
-    JSAtom* atom = script->getAtom(GET_UINT32_INDEX(pc));
-    if (atom == cx->names().undefined && (pc += CodeSpec[op].length) < endpc) {
-      op = JSOp(*pc);
-      return op == JSOP_EQ || op == JSOP_NE || op == JSOP_STRICTEQ ||
-             op == JSOP_STRICTNE;
+  // Special case #2: Do not warn if we are checking whether the property is
+  // undefined.
+  if (scriptIterator->is(JSOP_GETGNAME) || scriptIterator->is(JSOP_GETNAME) ||
+      scriptIterator->is(JSOP_UNDEFINED)) {
+    // If we using the result of a variable lookup to use in the comparison
+    // against the property and that lookup does not result in 'undefined',
+    // the type of subsequent operations do not matter -- we always warn.
+    if (scriptIterator->isNameOp() &&
+        scriptIterator->getPropertyName(script) != cx->names().undefined) {
+      return false;
     }
-  }
-  if (op == JSOP_UNDEFINED) {
-    if ((pc += CodeSpec[op].length) < endpc) {
-      op = JSOp(*pc);
-      return op == JSOP_EQ || op == JSOP_NE || op == JSOP_STRICTEQ ||
-             op == JSOP_STRICTNE;
+    // Because we know that the top of the stack is 'undefined', if the next
+    // operation exists and it is a comparison operation (of any kind) we
+    // supress a warning.
+    if (++scriptIterator == endIter) {
+      return false;
     }
+    return scriptIterator->isEqualityOp();
   }
-
   return false;
 }