Skip to content

Commit 8b18c5e

Browse files
Patrick ThierV8 LUCI CQ
Patrick Thier
authored and
V8 LUCI CQ
committed
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 <[email protected]> > Commit-Queue: Patrick Thier <[email protected]> > 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 <[email protected]> Commit-Queue: Patrick Thier <[email protected]> Cr-Commit-Position: refs/heads/master@{#75571}
1 parent d42ae80 commit 8b18c5e

30 files changed

Lines changed: 549 additions & 465 deletions

‎src/common/message-template.h‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -129,8 +129,12 @@ namespace internal {
129129
T(NonObjectAssertOption, "The 'assert' option must be an object") \
130130
T(NonObjectInInstanceOfCheck, \
131131
"Right-hand side of 'instanceof' is not an object") \
132-
T(NonObjectPropertyLoad, "Cannot read property '%' of %") \
133-
T(NonObjectPropertyStore, "Cannot set property '%' of %") \
132+
T(NonObjectPropertyLoad, "Cannot read properties of %") \
133+
T(NonObjectPropertyLoadWithProperty, \
134+
"Cannot read properties of % (reading '%')") \
135+
T(NonObjectPropertyStore, "Cannot set properties of %") \
136+
T(NonObjectPropertyStoreWithProperty, \
137+
"Cannot set properties of % (setting '%')") \
134138
T(NonObjectImportArgument, \
135139
"The second argument to import() must be an object") \
136140
T(NonStringImportAssertionValue, "Import assertion value must be a string") \

‎src/execution/messages.cc‎

Lines changed: 10 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -896,6 +896,9 @@ Object ErrorUtils::ThrowLoadFromNullOrUndefined(Isolate* isolate,
896896
if (key.ToHandle(&key_handle)) {
897897
if (key_handle->IsString()) {
898898
maybe_property_name = Handle<String>::cast(key_handle);
899+
} else {
900+
maybe_property_name =
901+
Object::NoSideEffectsToMaybeString(isolate, key_handle);
899902
}
900903
}
901904

@@ -969,14 +972,16 @@ Object ErrorUtils::ThrowLoadFromNullOrUndefined(Isolate* isolate,
969972
}
970973
} else {
971974
Handle<Object> key_handle;
972-
if (!key.ToHandle(&key_handle)) {
973-
key_handle = ReadOnlyRoots(isolate).undefined_value_handle();
974-
}
975-
if (*key_handle == ReadOnlyRoots(isolate).iterator_symbol()) {
975+
if (!key.ToHandle(&key_handle) ||
976+
!maybe_property_name.ToHandle(&property_name)) {
977+
error = isolate->factory()->NewTypeError(
978+
MessageTemplate::kNonObjectPropertyLoad, object);
979+
} else if (*key_handle == ReadOnlyRoots(isolate).iterator_symbol()) {
976980
error = NewIteratorError(isolate, object);
977981
} else {
978982
error = isolate->factory()->NewTypeError(
979-
MessageTemplate::kNonObjectPropertyLoad, key_handle, object);
983+
MessageTemplate::kNonObjectPropertyLoadWithProperty, object,
984+
property_name);
980985
}
981986
}
982987

‎src/ic/ic.cc‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1726,7 +1726,8 @@ MaybeHandle<Object> StoreIC::Store(Handle<Object> object, Handle<Name> name,
17261726
SetCache(name, StoreHandler::StoreSlow(isolate()));
17271727
TraceIC("StoreIC", name);
17281728
}
1729-
return TypeError(MessageTemplate::kNonObjectPropertyStore, object, name);
1729+
return TypeError(MessageTemplate::kNonObjectPropertyStoreWithProperty, name,
1730+
object);
17301731
}
17311732

17321733
JSObject::MakePrototypesFast(object, kStartAtPrototype, isolate());

‎src/objects/objects.cc‎

Lines changed: 16 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -463,8 +463,8 @@ Handle<String> NoSideEffectsErrorToString(Isolate* isolate,
463463
} // namespace
464464

465465
// static
466-
Handle<String> Object::NoSideEffectsToString(Isolate* isolate,
467-
Handle<Object> input) {
466+
MaybeHandle<String> Object::NoSideEffectsToMaybeString(Isolate* isolate,
467+
Handle<Object> input) {
468468
DisallowJavascriptExecution no_js(isolate);
469469

470470
if (input->IsString() || input->IsNumber() || input->IsOddball()) {
@@ -561,6 +561,20 @@ Handle<String> Object::NoSideEffectsToString(Isolate* isolate,
561561
}
562562
}
563563
}
564+
return MaybeHandle<String>(kNullMaybeHandle);
565+
}
566+
567+
// static
568+
Handle<String> Object::NoSideEffectsToString(Isolate* isolate,
569+
Handle<Object> input) {
570+
DisallowJavascriptExecution no_js(isolate);
571+
572+
// Try to convert input to a meaningful string.
573+
MaybeHandle<String> maybe_string = NoSideEffectsToMaybeString(isolate, input);
574+
Handle<String> string_handle;
575+
if (maybe_string.ToHandle(&string_handle)) {
576+
return string_handle;
577+
}
564578

565579
// At this point, input is either none of the above or a JSReceiver.
566580

‎src/objects/objects.h‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -410,6 +410,9 @@ class Object : public TaggedImpl<HeapObjectReferenceType::STRONG, Address> {
410410
V8_WARN_UNUSED_RESULT static inline MaybeHandle<String> ToString(
411411
Isolate* isolate, Handle<Object> input);
412412

413+
V8_EXPORT_PRIVATE static MaybeHandle<String> NoSideEffectsToMaybeString(
414+
Isolate* isolate, Handle<Object> input);
415+
413416
V8_EXPORT_PRIVATE static Handle<String> NoSideEffectsToString(
414417
Isolate* isolate, Handle<Object> input);
415418

‎src/runtime/runtime-classes.cc‎

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -675,11 +675,12 @@ MaybeHandle<JSReceiver> GetSuperHolder(Isolate* isolate,
675675
PrototypeIterator iter(isolate, home_object);
676676
Handle<Object> proto = PrototypeIterator::GetCurrent(iter);
677677
if (!proto->IsJSReceiver()) {
678-
MessageTemplate message = mode == SuperMode::kLoad
679-
? MessageTemplate::kNonObjectPropertyLoad
680-
: MessageTemplate::kNonObjectPropertyStore;
678+
MessageTemplate message =
679+
mode == SuperMode::kLoad
680+
? MessageTemplate::kNonObjectPropertyLoadWithProperty
681+
: MessageTemplate::kNonObjectPropertyStoreWithProperty;
681682
Handle<Name> name = key->GetName(isolate);
682-
THROW_NEW_ERROR(isolate, NewTypeError(message, name, proto), JSReceiver);
683+
THROW_NEW_ERROR(isolate, NewTypeError(message, proto, name), JSReceiver);
683684
}
684685
return Handle<JSReceiver>::cast(proto);
685686
}

‎src/runtime/runtime-object.cc‎

Lines changed: 15 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -525,10 +525,21 @@ MaybeHandle<Object> Runtime::SetObjectProperty(
525525
Handle<Object> value, StoreOrigin store_origin,
526526
Maybe<ShouldThrow> should_throw) {
527527
if (object->IsNullOrUndefined(isolate)) {
528-
THROW_NEW_ERROR(
529-
isolate,
530-
NewTypeError(MessageTemplate::kNonObjectPropertyStore, key, object),
531-
Object);
528+
MaybeHandle<String> maybe_property =
529+
Object::NoSideEffectsToMaybeString(isolate, key);
530+
Handle<String> property_name;
531+
if (maybe_property.ToHandle(&property_name)) {
532+
THROW_NEW_ERROR(
533+
isolate,
534+
NewTypeError(MessageTemplate::kNonObjectPropertyStoreWithProperty,
535+
object, property_name),
536+
Object);
537+
} else {
538+
THROW_NEW_ERROR(
539+
isolate,
540+
NewTypeError(MessageTemplate::kNonObjectPropertyStore, object),
541+
Object);
542+
}
532543
}
533544

534545
// Check if the given key is an array index.

‎test/cctest/interpreter/bytecode_expectations/PrivateAccessorAccess.golden‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -83,7 +83,7 @@ bytecodes: [
8383
B(Mov), R(this), R(0),
8484
B(Mov), R(context), R(2),
8585
/* 48 E> */ B(CallRuntime), U16(Runtime::kAddPrivateBrand), R(0), U8(3),
86-
/* 53 S> */ B(Wide), B(LdaSmi), I16(279),
86+
/* 53 S> */ B(Wide), B(LdaSmi), I16(281),
8787
B(Star3),
8888
B(LdaConstant), U8(0),
8989
B(Star4),
@@ -114,7 +114,7 @@ bytecodes: [
114114
B(Mov), R(this), R(0),
115115
B(Mov), R(context), R(2),
116116
/* 41 E> */ B(CallRuntime), U16(Runtime::kAddPrivateBrand), R(0), U8(3),
117-
/* 46 S> */ B(Wide), B(LdaSmi), I16(278),
117+
/* 46 S> */ B(Wide), B(LdaSmi), I16(280),
118118
B(Star3),
119119
B(LdaConstant), U8(0),
120120
B(Star4),
@@ -145,7 +145,7 @@ bytecodes: [
145145
B(Mov), R(this), R(0),
146146
B(Mov), R(context), R(2),
147147
/* 48 E> */ B(CallRuntime), U16(Runtime::kAddPrivateBrand), R(0), U8(3),
148-
/* 53 S> */ B(Wide), B(LdaSmi), I16(279),
148+
/* 53 S> */ B(Wide), B(LdaSmi), I16(281),
149149
B(Star3),
150150
B(LdaConstant), U8(0),
151151
B(Star4),
@@ -176,7 +176,7 @@ bytecodes: [
176176
B(Mov), R(this), R(0),
177177
B(Mov), R(context), R(2),
178178
/* 41 E> */ B(CallRuntime), U16(Runtime::kAddPrivateBrand), R(0), U8(3),
179-
/* 46 S> */ B(Wide), B(LdaSmi), I16(278),
179+
/* 46 S> */ B(Wide), B(LdaSmi), I16(280),
180180
B(Star4),
181181
B(LdaConstant), U8(0),
182182
B(Star5),

‎test/cctest/interpreter/bytecode_expectations/PrivateMethodAccess.golden‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -56,7 +56,7 @@ bytecodes: [
5656
B(Mov), R(this), R(0),
5757
B(Mov), R(context), R(2),
5858
/* 44 E> */ B(CallRuntime), U16(Runtime::kAddPrivateBrand), R(0), U8(3),
59-
/* 49 S> */ B(Wide), B(LdaSmi), I16(277),
59+
/* 49 S> */ B(Wide), B(LdaSmi), I16(279),
6060
B(Star3),
6161
B(LdaConstant), U8(0),
6262
B(Star4),
@@ -88,7 +88,7 @@ bytecodes: [
8888
B(Mov), R(this), R(0),
8989
B(Mov), R(context), R(2),
9090
/* 44 E> */ B(CallRuntime), U16(Runtime::kAddPrivateBrand), R(0), U8(3),
91-
/* 49 S> */ B(Wide), B(LdaSmi), I16(277),
91+
/* 49 S> */ B(Wide), B(LdaSmi), I16(279),
9292
B(Star3),
9393
B(LdaConstant), U8(0),
9494
B(Star4),

‎test/cctest/interpreter/bytecode_expectations/StaticPrivateMethodAccess.golden‎

Lines changed: 10 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,7 @@ bytecodes: [
2424
B(TestReferenceEqual), R(this),
2525
B(Mov), R(this), R(1),
2626
B(JumpIfTrue), U8(16),
27-
B(Wide), B(LdaSmi), I16(275),
27+
B(Wide), B(LdaSmi), I16(277),
2828
B(Star2),
2929
B(LdaConstant), U8(0),
3030
B(Star3),
@@ -55,7 +55,7 @@ frame size: 2
5555
parameter count: 1
5656
bytecode array length: 14
5757
bytecodes: [
58-
/* 56 S> */ B(Wide), B(LdaSmi), I16(277),
58+
/* 56 S> */ B(Wide), B(LdaSmi), I16(279),
5959
B(Star0),
6060
B(LdaConstant), U8(0),
6161
B(Star1),
@@ -82,7 +82,7 @@ frame size: 2
8282
parameter count: 1
8383
bytecode array length: 14
8484
bytecodes: [
85-
/* 56 S> */ B(Wide), B(LdaSmi), I16(277),
85+
/* 56 S> */ B(Wide), B(LdaSmi), I16(279),
8686
B(Star0),
8787
B(LdaConstant), U8(0),
8888
B(Star1),
@@ -121,7 +121,7 @@ bytecodes: [
121121
B(TestReferenceEqual), R(this),
122122
B(Mov), R(this), R(0),
123123
B(JumpIfTrue), U8(16),
124-
B(Wide), B(LdaSmi), I16(275),
124+
B(Wide), B(LdaSmi), I16(277),
125125
B(Star2),
126126
B(LdaConstant), U8(0),
127127
B(Star3),
@@ -143,7 +143,7 @@ bytecodes: [
143143
B(TestReferenceEqual), R(this),
144144
B(Mov), R(this), R(1),
145145
B(JumpIfTrue), U8(16),
146-
B(Wide), B(LdaSmi), I16(276),
146+
B(Wide), B(LdaSmi), I16(278),
147147
B(Star3),
148148
B(LdaConstant), U8(0),
149149
B(Star4),
@@ -158,7 +158,7 @@ bytecodes: [
158158
B(TestReferenceEqual), R(this),
159159
B(Mov), R(this), R(0),
160160
B(JumpIfTrue), U8(16),
161-
B(Wide), B(LdaSmi), I16(275),
161+
B(Wide), B(LdaSmi), I16(277),
162162
B(Star2),
163163
B(LdaConstant), U8(0),
164164
B(Star3),
@@ -188,7 +188,7 @@ frame size: 2
188188
parameter count: 1
189189
bytecode array length: 14
190190
bytecodes: [
191-
/* 60 S> */ B(Wide), B(LdaSmi), I16(279),
191+
/* 60 S> */ B(Wide), B(LdaSmi), I16(281),
192192
B(Star0),
193193
B(LdaConstant), U8(0),
194194
B(Star1),
@@ -214,7 +214,7 @@ frame size: 2
214214
parameter count: 1
215215
bytecode array length: 14
216216
bytecodes: [
217-
/* 53 S> */ B(Wide), B(LdaSmi), I16(278),
217+
/* 53 S> */ B(Wide), B(LdaSmi), I16(280),
218218
B(Star0),
219219
B(LdaConstant), U8(0),
220220
B(Star1),
@@ -240,7 +240,7 @@ frame size: 2
240240
parameter count: 1
241241
bytecode array length: 14
242242
bytecodes: [
243-
/* 60 S> */ B(Wide), B(LdaSmi), I16(279),
243+
/* 60 S> */ B(Wide), B(LdaSmi), I16(281),
244244
B(Star0),
245245
B(LdaConstant), U8(0),
246246
B(Star1),
@@ -266,7 +266,7 @@ frame size: 3
266266
parameter count: 1
267267
bytecode array length: 14
268268
bytecodes: [
269-
/* 46 S> */ B(Wide), B(LdaSmi), I16(278),
269+
/* 46 S> */ B(Wide), B(LdaSmi), I16(280),
270270
B(Star1),
271271
B(LdaConstant), U8(0),
272272
B(Star2),

0 commit comments

Comments
 (0)