Reland "Reland "Reland "Improve error messages for property access on…

… null/undefined""" This is a reland of 819c3ae Original change's description: > Reland "Reland "Improve error messages for property access on null/undefined"" > > This is a reland of 8b18c5e > > Original change's description: > > Reland "Improve error messages for property access on null/undefined" > > > > This is a reland of 24c626c > > > > Original change's description: > > > Improve error messages for property access on null/undefined > > > > > > Only print the property name when accessing null/undefined if we can > > > convert it to a string without causing side effects. > > > If we can't, omit the property name in the error message. > > > This should avoid confusion when the key is an object with toString(). > > > E.g. undefined[{toString:()=>'a'}] doesn't print 'read property [object > > > Object]' anymore, which was misleading since the property accessed would > > > be 'a', but we can't evaluate the key without side effects. > > > > > > Bug: v8:11365 > > > Change-Id: If82d1adb42561d4851e2bd2ca297a1c71738aee8 > > > Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2960211 > > > Reviewed-by: Toon Verwaest <verwaest@chromium.org> > > > Commit-Queue: Patrick Thier <pthier@chromium.org> > > > Cr-Commit-Position: refs/heads/master@{#75250} > > > > Bug: v8:11365 > > Change-Id: Ie2312337f4f1915faa31528a728d90833d80dbd1 > > Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2979599 > > Reviewed-by: Toon Verwaest <verwaest@chromium.org> > > Commit-Queue: Patrick Thier <pthier@chromium.org> > > Cr-Commit-Position: refs/heads/master@{#75571} > > Bug: v8:11365 > Change-Id: I90360641ecd870bd93247aa6d91dfb0ad049cfb8 > Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3008219 > Auto-Submit: Patrick Thier <pthier@chromium.org> > Commit-Queue: Toon Verwaest <verwaest@chromium.org> > Reviewed-by: Toon Verwaest <verwaest@chromium.org> > Cr-Commit-Position: refs/heads/master@{#75604} Bug: v8:11365 Change-Id: I002b537144f328ccbbdcd655e26e5dc87c49c6f5 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3013935 Reviewed-by: Toon Verwaest <verwaest@chromium.org> Commit-Queue: Patrick Thier <pthier@chromium.org> Cr-Commit-Position: refs/heads/master@{#75645}
v8 · Jul 8, 2021 · c0fd89c · c0fd89c
1 parent d31f77a
commit c0fd89c
Show file tree

Hide file tree

Showing 30 changed files with 549 additions and 465 deletions.
diff --git a/src/common/message-template.h b/src/common/message-template.h
@@ -129,8 +129,12 @@ namespace internal {
   T(NonObjectAssertOption, "The 'assert' option must be an object")            \
   T(NonObjectInInstanceOfCheck,                                                \
     "Right-hand side of 'instanceof' is not an object")                        \
-  T(NonObjectPropertyLoad, "Cannot read property '%' of %")                    \
-  T(NonObjectPropertyStore, "Cannot set property '%' of %")                    \
+  T(NonObjectPropertyLoad, "Cannot read properties of %")                      \
+  T(NonObjectPropertyLoadWithProperty,                                         \
+    "Cannot read properties of % (reading '%')")                               \
+  T(NonObjectPropertyStore, "Cannot set properties of %")                      \
+  T(NonObjectPropertyStoreWithProperty,                                        \
+    "Cannot set properties of % (setting '%')")                                \
   T(NonObjectImportArgument,                                                   \
     "The second argument to import() must be an object")                       \
   T(NonStringImportAssertionValue, "Import assertion value must be a string")  \

diff --git a/src/execution/messages.cc b/src/execution/messages.cc
@@ -896,6 +896,9 @@ Object ErrorUtils::ThrowLoadFromNullOrUndefined(Isolate* isolate,
   if (key.ToHandle(&key_handle)) {
     if (key_handle->IsString()) {
       maybe_property_name = Handle<String>::cast(key_handle);
+    } else {
+      maybe_property_name =
+          Object::NoSideEffectsToMaybeString(isolate, key_handle);
     }
   }
 
@@ -969,14 +972,16 @@ Object ErrorUtils::ThrowLoadFromNullOrUndefined(Isolate* isolate,
     }
   } else {
     Handle<Object> key_handle;
-    if (!key.ToHandle(&key_handle)) {
-      key_handle = ReadOnlyRoots(isolate).undefined_value_handle();
-    }
-    if (*key_handle == ReadOnlyRoots(isolate).iterator_symbol()) {
+    if (!key.ToHandle(&key_handle) ||
+        !maybe_property_name.ToHandle(&property_name)) {
+      error = isolate->factory()->NewTypeError(
+          MessageTemplate::kNonObjectPropertyLoad, object);
+    } else if (*key_handle == ReadOnlyRoots(isolate).iterator_symbol()) {
       error = NewIteratorError(isolate, object);
     } else {
       error = isolate->factory()->NewTypeError(
-          MessageTemplate::kNonObjectPropertyLoad, key_handle, object);
+          MessageTemplate::kNonObjectPropertyLoadWithProperty, object,
+          property_name);
     }
   }
 

diff --git a/src/ic/ic.cc b/src/ic/ic.cc
@@ -1726,7 +1726,8 @@ MaybeHandle<Object> StoreIC::Store(Handle<Object> object, Handle<Name> name,
       SetCache(name, StoreHandler::StoreSlow(isolate()));
       TraceIC("StoreIC", name);
     }
-    return TypeError(MessageTemplate::kNonObjectPropertyStore, object, name);
+    return TypeError(MessageTemplate::kNonObjectPropertyStoreWithProperty, name,
+                     object);
   }
 
   JSObject::MakePrototypesFast(object, kStartAtPrototype, isolate());

diff --git a/src/objects/objects.cc b/src/objects/objects.cc
@@ -464,8 +464,8 @@ Handle<String> NoSideEffectsErrorToString(Isolate* isolate,
 }  // namespace
 
 // static
-Handle<String> Object::NoSideEffectsToString(Isolate* isolate,
-                                             Handle<Object> input) {
+MaybeHandle<String> Object::NoSideEffectsToMaybeString(Isolate* isolate,
+                                                       Handle<Object> input) {
   DisallowJavascriptExecution no_js(isolate);
 
   if (input->IsString() || input->IsNumber() || input->IsOddball()) {
@@ -562,6 +562,20 @@ Handle<String> Object::NoSideEffectsToString(Isolate* isolate,
       }
     }
   }
+  return MaybeHandle<String>(kNullMaybeHandle);
+}
+
+// static
+Handle<String> Object::NoSideEffectsToString(Isolate* isolate,
+                                             Handle<Object> input) {
+  DisallowJavascriptExecution no_js(isolate);
+
+  // Try to convert input to a meaningful string.
+  MaybeHandle<String> maybe_string = NoSideEffectsToMaybeString(isolate, input);
+  Handle<String> string_handle;
+  if (maybe_string.ToHandle(&string_handle)) {
+    return string_handle;
+  }
 
   // At this point, input is either none of the above or a JSReceiver.
 

diff --git a/src/objects/objects.h b/src/objects/objects.h
@@ -410,6 +410,9 @@ class Object : public TaggedImpl<HeapObjectReferenceType::STRONG, Address> {
   V8_WARN_UNUSED_RESULT static inline MaybeHandle<String> ToString(
       Isolate* isolate, Handle<Object> input);
 
+  V8_EXPORT_PRIVATE static MaybeHandle<String> NoSideEffectsToMaybeString(
+      Isolate* isolate, Handle<Object> input);
+
   V8_EXPORT_PRIVATE static Handle<String> NoSideEffectsToString(
       Isolate* isolate, Handle<Object> input);
 

diff --git a/src/runtime/runtime-classes.cc b/src/runtime/runtime-classes.cc
@@ -675,11 +675,12 @@ MaybeHandle<JSReceiver> GetSuperHolder(Isolate* isolate,
   PrototypeIterator iter(isolate, home_object);
   Handle<Object> proto = PrototypeIterator::GetCurrent(iter);
   if (!proto->IsJSReceiver()) {
-    MessageTemplate message = mode == SuperMode::kLoad
-                                  ? MessageTemplate::kNonObjectPropertyLoad
-                                  : MessageTemplate::kNonObjectPropertyStore;
+    MessageTemplate message =
+        mode == SuperMode::kLoad
+            ? MessageTemplate::kNonObjectPropertyLoadWithProperty
+            : MessageTemplate::kNonObjectPropertyStoreWithProperty;
     Handle<Name> name = key->GetName(isolate);
-    THROW_NEW_ERROR(isolate, NewTypeError(message, name, proto), JSReceiver);
+    THROW_NEW_ERROR(isolate, NewTypeError(message, proto, name), JSReceiver);
   }
   return Handle<JSReceiver>::cast(proto);
 }

diff --git a/src/runtime/runtime-object.cc b/src/runtime/runtime-object.cc
@@ -525,10 +525,21 @@ MaybeHandle<Object> Runtime::SetObjectProperty(
     Handle<Object> value, StoreOrigin store_origin,
     Maybe<ShouldThrow> should_throw) {
   if (object->IsNullOrUndefined(isolate)) {
-    THROW_NEW_ERROR(
-        isolate,
-        NewTypeError(MessageTemplate::kNonObjectPropertyStore, key, object),
-        Object);
+    MaybeHandle<String> maybe_property =
+        Object::NoSideEffectsToMaybeString(isolate, key);
+    Handle<String> property_name;
+    if (maybe_property.ToHandle(&property_name)) {
+      THROW_NEW_ERROR(
+          isolate,
+          NewTypeError(MessageTemplate::kNonObjectPropertyStoreWithProperty,
+                       object, property_name),
+          Object);
+    } else {
+      THROW_NEW_ERROR(
+          isolate,
+          NewTypeError(MessageTemplate::kNonObjectPropertyStore, object),
+          Object);
+    }
   }
 
   // Check if the given key is an array index.

diff --git a/test/cctest/interpreter/bytecode_expectations/PrivateAccessorAccess.golden b/test/cctest/interpreter/bytecode_expectations/PrivateAccessorAccess.golden
@@ -83,7 +83,7 @@ bytecodes: [
                 B(Mov), R(this), R(0),
                 B(Mov), R(context), R(2),
   /*   48 E> */ B(CallRuntime), U16(Runtime::kAddPrivateBrand), R(0), U8(3),
-  /*   53 S> */ B(Wide), B(LdaSmi), I16(279),
+  /*   53 S> */ B(Wide), B(LdaSmi), I16(281),
                 B(Star3),
                 B(LdaConstant), U8(0),
                 B(Star4),
@@ -114,7 +114,7 @@ bytecodes: [
                 B(Mov), R(this), R(0),
                 B(Mov), R(context), R(2),
   /*   41 E> */ B(CallRuntime), U16(Runtime::kAddPrivateBrand), R(0), U8(3),
-  /*   46 S> */ B(Wide), B(LdaSmi), I16(278),
+  /*   46 S> */ B(Wide), B(LdaSmi), I16(280),
                 B(Star3),
                 B(LdaConstant), U8(0),
                 B(Star4),
@@ -145,7 +145,7 @@ bytecodes: [
                 B(Mov), R(this), R(0),
                 B(Mov), R(context), R(2),
   /*   48 E> */ B(CallRuntime), U16(Runtime::kAddPrivateBrand), R(0), U8(3),
-  /*   53 S> */ B(Wide), B(LdaSmi), I16(279),
+  /*   53 S> */ B(Wide), B(LdaSmi), I16(281),
                 B(Star3),
                 B(LdaConstant), U8(0),
                 B(Star4),
@@ -176,7 +176,7 @@ bytecodes: [
                 B(Mov), R(this), R(0),
                 B(Mov), R(context), R(2),
   /*   41 E> */ B(CallRuntime), U16(Runtime::kAddPrivateBrand), R(0), U8(3),
-  /*   46 S> */ B(Wide), B(LdaSmi), I16(278),
+  /*   46 S> */ B(Wide), B(LdaSmi), I16(280),
                 B(Star4),
                 B(LdaConstant), U8(0),
                 B(Star5),

diff --git a/test/cctest/interpreter/bytecode_expectations/PrivateMethodAccess.golden b/test/cctest/interpreter/bytecode_expectations/PrivateMethodAccess.golden
@@ -56,7 +56,7 @@ bytecodes: [
                 B(Mov), R(this), R(0),
                 B(Mov), R(context), R(2),
   /*   44 E> */ B(CallRuntime), U16(Runtime::kAddPrivateBrand), R(0), U8(3),
-  /*   49 S> */ B(Wide), B(LdaSmi), I16(277),
+  /*   49 S> */ B(Wide), B(LdaSmi), I16(279),
                 B(Star3),
                 B(LdaConstant), U8(0),
                 B(Star4),
@@ -88,7 +88,7 @@ bytecodes: [
                 B(Mov), R(this), R(0),
                 B(Mov), R(context), R(2),
   /*   44 E> */ B(CallRuntime), U16(Runtime::kAddPrivateBrand), R(0), U8(3),
-  /*   49 S> */ B(Wide), B(LdaSmi), I16(277),
+  /*   49 S> */ B(Wide), B(LdaSmi), I16(279),
                 B(Star3),
                 B(LdaConstant), U8(0),
                 B(Star4),

diff --git a/test/cctest/interpreter/bytecode_expectations/StaticPrivateMethodAccess.golden b/test/cctest/interpreter/bytecode_expectations/StaticPrivateMethodAccess.golden
@@ -24,7 +24,7 @@ bytecodes: [
                 B(TestReferenceEqual), R(this),
                 B(Mov), R(this), R(1),
                 B(JumpIfTrue), U8(16),
-                B(Wide), B(LdaSmi), I16(275),
+                B(Wide), B(LdaSmi), I16(277),
                 B(Star2),
                 B(LdaConstant), U8(0),
                 B(Star3),
@@ -55,7 +55,7 @@ frame size: 2
 parameter count: 1
 bytecode array length: 14
 bytecodes: [
-  /*   56 S> */ B(Wide), B(LdaSmi), I16(277),
+  /*   56 S> */ B(Wide), B(LdaSmi), I16(279),
                 B(Star0),
                 B(LdaConstant), U8(0),
                 B(Star1),
@@ -82,7 +82,7 @@ frame size: 2
 parameter count: 1
 bytecode array length: 14
 bytecodes: [
-  /*   56 S> */ B(Wide), B(LdaSmi), I16(277),
+  /*   56 S> */ B(Wide), B(LdaSmi), I16(279),
                 B(Star0),
                 B(LdaConstant), U8(0),
                 B(Star1),
@@ -121,7 +121,7 @@ bytecodes: [
                 B(TestReferenceEqual), R(this),
                 B(Mov), R(this), R(0),
                 B(JumpIfTrue), U8(16),
-                B(Wide), B(LdaSmi), I16(275),
+                B(Wide), B(LdaSmi), I16(277),
                 B(Star2),
                 B(LdaConstant), U8(0),
                 B(Star3),
@@ -143,7 +143,7 @@ bytecodes: [
                 B(TestReferenceEqual), R(this),
                 B(Mov), R(this), R(1),
                 B(JumpIfTrue), U8(16),
-                B(Wide), B(LdaSmi), I16(276),
+                B(Wide), B(LdaSmi), I16(278),
                 B(Star3),
                 B(LdaConstant), U8(0),
                 B(Star4),
@@ -158,7 +158,7 @@ bytecodes: [
                 B(TestReferenceEqual), R(this),
                 B(Mov), R(this), R(0),
                 B(JumpIfTrue), U8(16),
-                B(Wide), B(LdaSmi), I16(275),
+                B(Wide), B(LdaSmi), I16(277),
                 B(Star2),
                 B(LdaConstant), U8(0),
                 B(Star3),
@@ -188,7 +188,7 @@ frame size: 2
 parameter count: 1
 bytecode array length: 14
 bytecodes: [
-  /*   60 S> */ B(Wide), B(LdaSmi), I16(279),
+  /*   60 S> */ B(Wide), B(LdaSmi), I16(281),
                 B(Star0),
                 B(LdaConstant), U8(0),
                 B(Star1),
@@ -214,7 +214,7 @@ frame size: 2
 parameter count: 1
 bytecode array length: 14
 bytecodes: [
-  /*   53 S> */ B(Wide), B(LdaSmi), I16(278),
+  /*   53 S> */ B(Wide), B(LdaSmi), I16(280),
                 B(Star0),
                 B(LdaConstant), U8(0),
                 B(Star1),
@@ -240,7 +240,7 @@ frame size: 2
 parameter count: 1
 bytecode array length: 14
 bytecodes: [
-  /*   60 S> */ B(Wide), B(LdaSmi), I16(279),
+  /*   60 S> */ B(Wide), B(LdaSmi), I16(281),
                 B(Star0),
                 B(LdaConstant), U8(0),
                 B(Star1),
@@ -266,7 +266,7 @@ frame size: 3
 parameter count: 1
 bytecode array length: 14
 bytecodes: [
-  /*   46 S> */ B(Wide), B(LdaSmi), I16(278),
+  /*   46 S> */ B(Wide), B(LdaSmi), I16(280),
                 B(Star1),
                 B(LdaConstant), U8(0),
                 B(Star2),

diff --git a/test/debugger/debug/es6/debug-evaluate-receiver-before-super.js b/test/debugger/debug/es6/debug-evaluate-receiver-before-super.js
@@ -27,7 +27,7 @@ class A { constructor () { this.a = 239; } }
 class B extends A {
   constructor () {
     debugger;
-    assertTrue(result.indexOf("Cannot read property 'a' of undefined") >= 0);
+    assertTrue(result.indexOf("Cannot read properties of undefined (reading 'a')") >= 0);
     super();
     debugger;
     assertEquals(239, result);

diff --git a/test/message/fail/destructuring-undefined-number-property.out b/test/message/fail/destructuring-undefined-number-property.out
@@ -1,5 +1,5 @@
-*%(basename)s:5: TypeError: Cannot destructure 'undefined' as it is undefined.
+*%(basename)s:5: TypeError: Cannot destructure property '1' of 'undefined' as it is undefined.
 var { 1: x } = undefined;
-               ^
-TypeError: Cannot destructure 'undefined' as it is undefined.
+         ^
+TypeError: Cannot destructure property '1' of 'undefined' as it is undefined.
     at *%(basename)s:5:10
diff --git a/test/message/fail/overwritten-builtins.out b/test/message/fail/overwritten-builtins.out
@@ -25,9 +25,9 @@
 # (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
 # OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
 
-*%(basename)s:31: TypeError: Cannot read property 'x' of undefined
+*%(basename)s:31: TypeError: Cannot read properties of undefined (reading 'x')
 undefined.x
           ^
-TypeError: Cannot read property 'x' of undefined
+TypeError: Cannot read properties of undefined (reading 'x')
     at *%(basename)s:31:11
 
diff --git a/test/message/fail/undefined-keyed-computed-property.js b/test/message/fail/undefined-keyed-computed-property.js
@@ -0,0 +1,6 @@
+// Copyright 2021 the V8 project authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+var x = undefined;
+x[{toString:()=>'a'}];
diff --git a/test/message/fail/undefined-keyed-computed-property.out b/test/message/fail/undefined-keyed-computed-property.out
@@ -0,0 +1,9 @@
+# Copyright 2021 the V8 project authors. All rights reserved.
+# Use of this source code is governed by a BSD-style license that can be
+# found in the LICENSE file.
+
+*%(basename)s:6: TypeError: Cannot read properties of undefined
+x[{toString:()=>'a'}];
+ ^
+TypeError: Cannot read properties of undefined
+    at *%(basename)s:6:2
diff --git a/test/message/fail/undefined-keyed-number-property.js b/test/message/fail/undefined-keyed-number-property.js
@@ -0,0 +1,6 @@
+// Copyright 2021 the V8 project authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+var x = undefined;
+x[4711];
diff --git a/test/message/fail/undefined-keyed-number-property.out b/test/message/fail/undefined-keyed-number-property.out
@@ -0,0 +1,9 @@
+# Copyright 2021 the V8 project authors. All rights reserved.
+# Use of this source code is governed by a BSD-style license that can be
+# found in the LICENSE file.
+
+*%(basename)s:6: TypeError: Cannot read properties of undefined (reading '4711')
+x[4711];
+ ^
+TypeError: Cannot read properties of undefined (reading '4711')
+    at *%(basename)s:6:2
diff --git a/test/message/fail/undefined-keyed-string-property.js b/test/message/fail/undefined-keyed-string-property.js
@@ -0,0 +1,6 @@
+// Copyright 2021 the V8 project authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+var x = undefined;
+x['a'];
diff --git a/test/message/fail/undefined-keyed-string-property.out b/test/message/fail/undefined-keyed-string-property.out
@@ -0,0 +1,9 @@
+# Copyright 2021 the V8 project authors. All rights reserved.
+# Use of this source code is governed by a BSD-style license that can be
+# found in the LICENSE file.
+
+*%(basename)s:6: TypeError: Cannot read properties of undefined (reading 'a')
+x['a'];
+ ^
+TypeError: Cannot read properties of undefined (reading 'a')
+    at *%(basename)s:6:2