Skip to content

Commit 8379354

Browse files
varomodtcopybara-github
authored andcommitted
prevent writes to properties on readonly types
PiperOrigin-RevId: 508469546
1 parent be54db6 commit 8379354

File tree

2 files changed

+115
-33
lines changed

2 files changed

+115
-33
lines changed

src/com/google/javascript/jscomp/TypeCheck.java

Lines changed: 85 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,7 @@
3838
import static java.util.stream.Collectors.joining;
3939

4040
import com.google.common.annotations.VisibleForTesting;
41+
import com.google.common.collect.ImmutableList;
4142
import com.google.common.collect.ImmutableSet;
4243
import com.google.common.collect.Iterables;
4344
import com.google.errorprone.annotations.CanIgnoreReturnValue;
@@ -67,6 +68,7 @@
6768
import com.google.javascript.rhino.jstype.TemplateType;
6869
import com.google.javascript.rhino.jstype.TemplateTypeMap;
6970
import com.google.javascript.rhino.jstype.TemplatizedType;
71+
import com.google.javascript.rhino.jstype.UnionType;
7072
import java.util.HashMap;
7173
import java.util.HashSet;
7274
import java.util.Iterator;
@@ -316,56 +318,62 @@ public final class TypeCheck implements NodeTraversal.Callback, CompilerPass {
316318
"JSC_SAME_INTERFACE_MULTIPLE_IMPLEMENTS",
317319
"Cannot @implement the same interface more than once\nRepeated interface: {0}");
318320

321+
static final DiagnosticType PROPERTY_ASSIGNMENT_TO_READONLY_VALUE =
322+
DiagnosticType.error(
323+
"JSC_PROPERTY_ASSIGNMENT_TO_READONLY_VALUE",
324+
"Should not assign to a property of readonly type ''{0}''");
325+
319326
// If a diagnostic is disabled by default, do not add it in this list
320327
// TODO(dimvar): Either INEXISTENT_PROPERTY shouldn't be here, or we should
321328
// change DiagnosticGroups.setWarningLevel to not accidentally enable it.
322329
static final DiagnosticGroup ALL_DIAGNOSTICS =
323330
new DiagnosticGroup(
324-
DETERMINISTIC_TEST,
325-
INEXISTENT_ENUM_ELEMENT,
326-
INEXISTENT_PROPERTY,
327-
POSSIBLE_INEXISTENT_PROPERTY,
328-
INEXISTENT_PROPERTY_WITH_SUGGESTION,
329-
NOT_A_CONSTRUCTOR,
330-
INSTANTIATE_ABSTRACT_CLASS,
331-
BIT_OPERATION,
332-
UNARY_OPERATION,
331+
ABSTRACT_METHOD_IN_CONCRETE_CLASS,
332+
ABSTRACT_SUPER_METHOD_NOT_USABLE,
333+
BAD_IMPLEMENTED_TYPE,
333334
BINARY_OPERATION,
335+
BIT_OPERATION,
336+
CONFLICTING_EXTENDED_TYPE,
334337
CONFLICTING_GETTER_SETTER_TYPE,
335-
NOT_CALLABLE,
338+
CONFLICTING_IMPLEMENTED_TYPE,
336339
CONSTRUCTOR_NOT_CALLABLE,
340+
DETERMINISTIC_TEST,
341+
ES5_CLASS_EXTENDING_ES6_CLASS,
342+
EXPECTED_THIS_TYPE,
337343
FUNCTION_MASKS_VARIABLE,
338-
MULTIPLE_VAR_DEF,
339-
INVALID_INTERFACE_MEMBER_DECLARATION,
340-
INTERFACE_METHOD_NOT_EMPTY,
341-
CONFLICTING_EXTENDED_TYPE,
342-
CONFLICTING_IMPLEMENTED_TYPE,
343-
BAD_IMPLEMENTED_TYPE,
344-
TypeValidator.HIDDEN_SUPERCLASS_PROPERTY_MISMATCH,
345344
HIDDEN_PROTOTYPAL_SUPERTYPE_PROPERTY_MISMATCH,
346-
UNKNOWN_OVERRIDE,
347-
UNKNOWN_PROTOTYPAL_OVERRIDE,
348-
WRONG_ARGUMENT_COUNT,
349-
ILLEGAL_IMPLICIT_CAST,
350-
INCOMPATIBLE_EXTENDED_PROPERTY_TYPE,
351-
EXPECTED_THIS_TYPE,
352-
IN_USED_WITH_STRUCT,
353345
ILLEGAL_CLASS_KEY,
346+
ILLEGAL_IMPLICIT_CAST,
347+
ILLEGAL_OBJLIT_KEY,
354348
ILLEGAL_PROPERTY_CREATION,
355349
ILLEGAL_PROPERTY_CREATION_ON_UNION_TYPE,
356-
ILLEGAL_OBJLIT_KEY,
350+
INCOMPATIBLE_EXTENDED_PROPERTY_TYPE,
351+
INEXISTENT_ENUM_ELEMENT,
352+
INEXISTENT_PROPERTY,
353+
INEXISTENT_PROPERTY_WITH_SUGGESTION,
354+
INSTANTIATE_ABSTRACT_CLASS,
355+
INTERFACE_METHOD_NOT_EMPTY,
356+
INVALID_INTERFACE_MEMBER_DECLARATION,
357+
IN_USED_WITH_STRUCT,
358+
MULTIPLE_VAR_DEF,
357359
NON_STRINGIFIABLE_OBJECT_KEY,
358-
ABSTRACT_METHOD_IN_CONCRETE_CLASS,
359-
ABSTRACT_SUPER_METHOD_NOT_USABLE,
360-
ES5_CLASS_EXTENDING_ES6_CLASS,
361-
SAME_INTERFACE_MULTIPLE_IMPLEMENTS,
360+
NOT_A_CONSTRUCTOR,
361+
NOT_CALLABLE,
362+
POSSIBLE_INEXISTENT_PROPERTY,
363+
PROPERTY_ASSIGNMENT_TO_READONLY_VALUE,
364+
RhinoErrorReporter.CYCLIC_INHERITANCE_ERROR,
362365
RhinoErrorReporter.TYPE_PARSE_ERROR,
363366
RhinoErrorReporter.UNRECOGNIZED_TYPE_ERROR,
364-
RhinoErrorReporter.CYCLIC_INHERITANCE_ERROR,
365-
TypedScopeCreator.UNKNOWN_LENDS,
366-
TypedScopeCreator.LENDS_ON_NON_OBJECT,
367+
SAME_INTERFACE_MULTIPLE_IMPLEMENTS,
368+
TypeValidator.HIDDEN_SUPERCLASS_PROPERTY_MISMATCH,
367369
TypedScopeCreator.CTOR_INITIALIZER,
368-
TypedScopeCreator.IFACE_INITIALIZER);
370+
TypedScopeCreator.IFACE_INITIALIZER,
371+
TypedScopeCreator.LENDS_ON_NON_OBJECT,
372+
TypedScopeCreator.UNKNOWN_LENDS,
373+
UNARY_OPERATION,
374+
UNKNOWN_OVERRIDE,
375+
UNKNOWN_PROTOTYPAL_OVERRIDE,
376+
WRONG_ARGUMENT_COUNT);
369377

370378
private final AbstractCompiler compiler;
371379
private final TypeValidator validator;
@@ -1303,6 +1311,9 @@ private void checkCanAssignToNameGetpropOrGetelem(
13031311
checkArgument(
13041312
lvalue.isName() || lvalue.isGetProp() || lvalue.isGetElem() || lvalue.isCast(), lvalue);
13051313

1314+
// Ensure our LHS is not readonly.
1315+
checkNotReadonlyPropertyAssignment(lvalue);
1316+
13061317
if (lvalue.isGetProp()) {
13071318
Node object = lvalue.getFirstChild();
13081319
JSType objectJsType = getJSType(object);
@@ -3445,6 +3456,47 @@ private boolean classHasToString(ObjectType type) {
34453456
return false;
34463457
}
34473458

3459+
/**
3460+
* Given the LHS of a property or element assignment, checks that the type that we're assigning
3461+
* into is not readonly (ReadonlyArray, in particular).
3462+
*
3463+
* <p>This is basically unsound since it checks just "ReadonlyArray" so casting up to "Iterable"
3464+
* or down to "Array" will defeat it; but it should catch basic errors.
3465+
*/
3466+
private void checkNotReadonlyPropertyAssignment(Node lhs) {
3467+
// We only care about element or property assignments.
3468+
if (!lhs.isGetProp() && !lhs.isGetElem()) {
3469+
return;
3470+
}
3471+
3472+
// If we do not have type information, drop out.
3473+
JSType lhsType = lhs.getFirstChild().getJSType();
3474+
if (lhsType == null) {
3475+
return;
3476+
}
3477+
3478+
// We could be a reference to "ReadonlyArray" or a templatized wrapper.
3479+
JSType roArray = getNativeType(JSTypeNative.READONLY_ARRAY_TYPE);
3480+
ImmutableList<JSType> alternates = flattenUnion(lhsType);
3481+
for (int i = 0; i < alternates.size(); i++) {
3482+
JSType type = alternates.get(i);
3483+
if (roArray.equals(type) || roArray.equals(maybeReferencedType(type))) {
3484+
compiler.report(JSError.make(lhs, PROPERTY_ASSIGNMENT_TO_READONLY_VALUE, type.toString()));
3485+
break;
3486+
}
3487+
}
3488+
}
3489+
3490+
private static @Nullable JSType maybeReferencedType(JSType type) {
3491+
TemplatizedType maybeTemplatized = type.toMaybeTemplatizedType();
3492+
return (maybeTemplatized != null) ? maybeTemplatized.getReferencedType() : null;
3493+
}
3494+
3495+
private static ImmutableList<JSType> flattenUnion(JSType maybeUnion) {
3496+
UnionType union = maybeUnion.toMaybeUnionType();
3497+
return (union == null) ? ImmutableList.of(maybeUnion) : union.getAlternates();
3498+
}
3499+
34483500
private static boolean declaresOverride(@Nullable JSDocInfo jsdoc) {
34493501
return (jsdoc != null) && jsdoc.isOverride();
34503502
}

test/com/google/javascript/jscomp/TypeCheckTest.java

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8474,6 +8474,36 @@ public void testAssignInference() {
84748474
.run();
84758475
}
84768476

8477+
@Test
8478+
public void testAssignReadonlyArrayValueFails() {
8479+
newTest()
8480+
.includeDefaultExterns()
8481+
.addSource("const foo = /** @type {!ReadonlyArray<number>} */ ([5]); " + "foo[0] = 3; ")
8482+
.diagnosticsAreErrors()
8483+
.addDiagnostic(TypeCheck.PROPERTY_ASSIGNMENT_TO_READONLY_VALUE)
8484+
.run();
8485+
}
8486+
8487+
@Test
8488+
public void testAssignReadonlyArrayValueFailsWithoutTypeParameter() {
8489+
newTest()
8490+
.includeDefaultExterns()
8491+
.addSource("const foo = /** @type {!ReadonlyArray} */ ([5]); " + "foo[0] = 3; ")
8492+
.diagnosticsAreErrors()
8493+
.addDiagnostic(TypeCheck.PROPERTY_ASSIGNMENT_TO_READONLY_VALUE)
8494+
.run();
8495+
}
8496+
8497+
@Test
8498+
public void testAssignReadonlyArrayLengthFails() {
8499+
newTest()
8500+
.includeDefaultExterns()
8501+
.addSource("const foo = /** @type {!ReadonlyArray<number>} */ ([5]); " + "foo.length = 0; ")
8502+
.diagnosticsAreErrors()
8503+
.addDiagnostic(TypeCheck.PROPERTY_ASSIGNMENT_TO_READONLY_VALUE)
8504+
.run();
8505+
}
8506+
84778507
@Test
84788508
public void testOr1() {
84798509
newTest()

0 commit comments

Comments
 (0)