From c7bb3b1d8babbbcf13489df0306277d6974b0d01 Mon Sep 17 00:00:00 2001 From: nickreid Date: Fri, 24 Aug 2018 12:03:22 -0700 Subject: [PATCH] Generalize access-control checks on constructor invocations. This change: - detangles controls on CLASS constructors versus namespaces - expands checks to cover more cases using type information - consolidates some special-casing used to track how a class reference is being used ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=210134923 --- .../jscomp/CheckAccessControls.java | 306 +++++++++++------ .../CheckAccessControlsEs6ClassTest.java | 325 +++++++++++++++++- 2 files changed, 512 insertions(+), 119 deletions(-) diff --git a/src/com/google/javascript/jscomp/CheckAccessControls.java b/src/com/google/javascript/jscomp/CheckAccessControls.java index c3f0a26f98e..33aebcddb8e 100644 --- a/src/com/google/javascript/jscomp/CheckAccessControls.java +++ b/src/com/google/javascript/jscomp/CheckAccessControls.java @@ -338,84 +338,117 @@ public boolean shouldTraverse(NodeTraversal traversal, Node node, Node parent) { } @Override - public void visit(NodeTraversal t, Node n, Node parent) { - switch (n.getToken()) { - case NAME: - checkNameDeprecation(t, n, parent); - checkNameVisibility(t, n, parent); - break; - case STRING_KEY: - case GETTER_DEF: - case SETTER_DEF: - case MEMBER_FUNCTION_DEF: - if (!parent.isObjectLit()) { - // TODO(b/80580110): Eventually object literals should be covered by `PropertyReference`s. - // However, doing so initially would have caused too many errors in existing code and - // delayed support for class syntax. - break; - } + public void visit(NodeTraversal traversal, Node node, Node parent) { - checkKeyVisibilityConvention(t, n, parent); - break; - case NEW: - checkConstructorDeprecation(t, n); + IdentifierBehaviour identifierBehaviour = IdentifierBehaviour.select(node); + @Nullable PropertyReference propRef = createPropertyReference(node); + + checkDeprecation(node, propRef, identifierBehaviour, traversal); + checkVisibility(node, propRef, identifierBehaviour, traversal); + checkConstantProperty(propRef, identifierBehaviour, traversal); + + checkFinalClassOverrides(node, traversal); + + @Nullable Node accessControlRoot = primaryAccessControlScopeRootFor(node); + if (accessControlRoot != null) { + exitAccessControlScope(accessControlRoot); + } + } + + private void checkDeprecation( + Node node, + @Nullable PropertyReference propRef, + IdentifierBehaviour identifierBehaviour, + NodeTraversal traversal) { + + switch (identifierBehaviour) { + case ES5_CLASS_INVOCATION: + case ES6_CLASS_INVOCATION: + case ES6_CLASS_NAMESPACE: + // At these usages, treat the deprecation applied to type-declaration as referring to the + // type, not the identifier (e.g. "the use of class `Foo` is deprecated"). + checkTypeDeprecation(traversal, node); break; - case FUNCTION: - case CLASS: - checkFinalClassOverrides(t, n); + + case NON_CONSTRUCTOR: + // For all identifiers that are not constructors, deprecation refers to the identifier (e.g. + // "the use of variable `x` is deprecated"). + checkNameDeprecation(traversal, node); break; + default: break; } - @Nullable PropertyReference propRef = createPropertyReference(n); - if (propRef != null) { - checkPropertyDeprecation(t, propRef); - checkPropertyVisibility(t, propRef); - checkConstantProperty(t, propRef); - } - - @Nullable Node accessControlRoot = primaryAccessControlScopeRootFor(n); - if (accessControlRoot != null) { - exitAccessControlScope(accessControlRoot); + if (propRef != null && !identifierBehaviour.equals(IdentifierBehaviour.ES5_CLASS_NAMESPACE)) { + checkPropertyDeprecation(traversal, propRef); } } - /** Checks the given NEW node to ensure that access restrictions are obeyed. */ - private void checkConstructorDeprecation(NodeTraversal t, Node n) { - if (!shouldEmitDeprecationWarning(t, n)) { - return; + private void checkVisibility( + Node node, + @Nullable PropertyReference propRef, + IdentifierBehaviour identifierBehaviour, + NodeTraversal traversal) { + if (identifierBehaviour.equals(IdentifierBehaviour.ES6_CLASS_INVOCATION)) { + checkEs6ConstructorVisibility(node, traversal); } - JSType type = n.getJSType(); + if (!identifierBehaviour.equals(IdentifierBehaviour.ES5_CLASS_NAMESPACE)) { + checkNameVisibility(traversal, node); + } - if (type != null) { - String deprecationInfo = getTypeDeprecationInfo(type); + if (node.getParent().isObjectLit()) { + // TODO(b/80580110): Eventually object literals should be covered by `PropertyReference`s. + // However, doing so initially would have caused too many errors in existing code and + // delayed support for class syntax. - if (deprecationInfo != null) { + switch (node.getToken()) { + case STRING_KEY: + case GETTER_DEF: + case SETTER_DEF: + case MEMBER_FUNCTION_DEF: + checkKeyVisibilityConvention(traversal, node, node.getParent()); + break; - if (!deprecationInfo.isEmpty()) { - compiler.report( - t.makeError(n, DEPRECATED_CLASS_REASON, - type.toString(), deprecationInfo)); - } else { - compiler.report( - t.makeError(n, DEPRECATED_CLASS, type.toString())); - } + default: + break; } } + + if (propRef != null && !identifierBehaviour.equals(IdentifierBehaviour.ES5_CLASS_NAMESPACE)) { + checkPropertyVisibility(traversal, propRef); + } } /** - * Checks the given NAME node to ensure that access restrictions are obeyed. + * Reports deprecation issue with regard to a type usage. + * + *

Precondition: {@code n} has a constructor {@link JSType}. */ - private void checkNameDeprecation(NodeTraversal t, Node n, Node parent) { + private void checkTypeDeprecation(NodeTraversal t, Node n) { if (!shouldEmitDeprecationWarning(t, n)) { return; } - // Don't bother checking definitions or constructors. - if (parent.isFunction() || NodeUtil.isNameDeclaration(parent) || parent.isNew()) { + ObjectType instanceType = n.getJSType().toMaybeFunctionType().getInstanceType(); + + String deprecationInfo = getTypeDeprecationInfo(instanceType); + if (deprecationInfo == null) { + return; + } + + DiagnosticType message = deprecationInfo.isEmpty() ? DEPRECATED_CLASS : DEPRECATED_CLASS_REASON; + compiler.report(t.makeError(n, message, instanceType.toString(), deprecationInfo)); + } + + /** Checks the given NAME node to ensure that access restrictions are obeyed. */ + private void checkNameDeprecation(NodeTraversal t, Node n) { + if (!n.isName()) { + return; + } + + if (!shouldEmitDeprecationWarning(t, n)) { return; } @@ -512,10 +545,15 @@ private void checkKeyVisibilityConvention(NodeTraversal t, Node key, Node parent /** * Reports an error if the given name is not visible in the current context. + * * @param t The current traversal. * @param name The name node. */ - private void checkNameVisibility(NodeTraversal t, Node name, Node parent) { + private void checkNameVisibility(NodeTraversal t, Node name) { + if (!name.isName()) { + return; + } + Var var = t.getScope().getVar(name.getString()); if (var == null) { return; @@ -534,7 +572,7 @@ private void checkNameVisibility(NodeTraversal t, Node name, Node parent) { } break; case PRIVATE: - if (!isPrivateAccessAllowed(var, name, parent)) { + if (!isPrivateAccessAllowed(var, name)) { compiler.report( t.makeError(name, BAD_PRIVATE_GLOBAL_ACCESS, name.getString(), var.getSourceFile().getName())); @@ -563,38 +601,29 @@ private Visibility checkPrivateNameConvention(Visibility v, Node name) { return v; } - private static boolean isPrivateAccessAllowed(Var var, Node name, Node parent) { + private static boolean isPrivateAccessAllowed(Var var, Node name) { StaticSourceFile varSrc = var.getSourceFile(); StaticSourceFile refSrc = name.getStaticSourceFile(); - JSDocInfo docInfo = var.getJSDocInfo(); - if (varSrc != null - && refSrc != null - && !varSrc.getName().equals(refSrc.getName())) { - return docInfo != null && docInfo.isConstructor() - && isValidPrivateConstructorAccess(parent); - } else { - return true; - } + + return varSrc == null || refSrc == null || Objects.equals(varSrc.getName(), refSrc.getName()); } private boolean isPackageAccessAllowed(Var var, Node name) { StaticSourceFile varSrc = var.getSourceFile(); StaticSourceFile refSrc = name.getStaticSourceFile(); - CodingConvention codingConvention = compiler.getCodingConvention(); - if (varSrc != null && refSrc != null) { - String srcPackage = codingConvention.getPackageName(varSrc); - String refPackage = codingConvention.getPackageName(refSrc); - return srcPackage != null - && refPackage != null - && srcPackage.equals(refPackage); - } else { - // If the source file of either var or name is unavailable, conservatively - // assume they belong to different packages. - // TODO(brndn): by contrast, isPrivateAccessAllowed does allow - // private access when a source file is unknown. I didn't change it - // in order not to break existing code. + if (varSrc == null && refSrc == null) { + // If the source file of either var or name is unavailable, conservatively assume they belong + // to different packages. + // + // TODO(brndn): by contrast, isPrivateAccessAllowed does allow private access when a source + // file is unknown. I didn't change it in order not to break existing code. return false; } + + CodingConvention codingConvention = compiler.getCodingConvention(); + String srcPackage = codingConvention.getPackageName(varSrc); + String refPackage = codingConvention.getPackageName(refSrc); + return srcPackage != null && refPackage != null && Objects.equals(srcPackage, refPackage); } private void checkOverriddenPropertyVisibilityMismatch( @@ -625,8 +654,10 @@ private static Visibility getOverridingPropertyVisibility(PropertyReference prop } /** Checks if a constructor is trying to override a final class. */ - private void checkFinalClassOverrides(NodeTraversal t, Node ctor) { - checkArgument(isFunctionOrClass(ctor), ctor); + private void checkFinalClassOverrides(Node ctor, NodeTraversal t) { + if (!isFunctionOrClass(ctor)) { + return; + } JSType type = ctor.getJSType().toMaybeFunctionType(); if (type != null && type.isConstructor()) { @@ -643,7 +674,14 @@ private void checkFinalClassOverrides(NodeTraversal t, Node ctor) { } /** Determines whether the given constant property got reassigned */ - private void checkConstantProperty(NodeTraversal t, PropertyReference propRef) { + private void checkConstantProperty( + @Nullable PropertyReference propRef, + IdentifierBehaviour identifierBehaviour, + NodeTraversal t) { + if (propRef == null || identifierBehaviour.equals(IdentifierBehaviour.ES5_CLASS_NAMESPACE)) { + return; + } + if (!propRef.isMutation()) { return; } @@ -747,8 +785,6 @@ private void checkPropertyVisibility(NodeTraversal t, PropertyReference propRef) StaticSourceFile definingSource = AccessControlUtils.getDefiningSource(propRef.getSourceNode(), referenceType, propertyName); - boolean isClassType = false; - // Is this a normal property access, or are we trying to override // an existing property? boolean isOverride = propRef.isDocumentedDeclaration() || propRef.isOverride(); @@ -783,7 +819,6 @@ private void checkPropertyVisibility(NodeTraversal t, PropertyReference propRef) } reportType = objectType; definingSource = node.getStaticSourceFile(); - isClassType = objectType.getOwnPropertyJSDocInfo(propertyName).isConstructor(); } else if (!isPrivateByConvention && fileOverviewVisibility == null) { // We can only check visibility references if we know what file // it was defined in. @@ -800,10 +835,33 @@ private void checkPropertyVisibility(NodeTraversal t, PropertyReference propRef) t, propRef, visibility, fileOverviewVisibility, reportType, sameInput); } else { checkNonOverriddenPropertyVisibility( - t, propRef, visibility, isClassType, reportType, referenceSource, definingSource); + t, propRef, visibility, reportType, referenceSource, definingSource); } } + /** + * Reports visibility violations on ES6 class constructor invocations. + * + *

Precondition: {@code target} has an ES6 class {@link JSType}. + */ + private void checkEs6ConstructorVisibility(Node target, NodeTraversal traversal) { + FunctionType ctorType = target.getJSType().toMaybeFunctionType(); + + // Check constructor visibility by pretending we're accessing `Foo.prototype.constructor`. + PropertyReference fauxCtorRef = + PropertyReference.builder() + .setSourceNode(target) + .setName("constructor") + .setReceiverType(ctorType.getPrototype()) + .setMutation(false) // This shouldn't matter. + .setDeclaration(false) // This shouldn't matter. + .setOverride(false) // This shouldn't matter. + .setReadableTypeName(() -> ctorType.getInstanceType().toString()) + .build(); + + checkPropertyVisibility(traversal, fauxCtorRef); + } + private static boolean propertyIsDeclaredButNotPrivate(PropertyReference propRef) { if (!propRef.isDocumentedDeclaration() && !propRef.isOverride()) { return false; @@ -853,7 +911,6 @@ private void checkNonOverriddenPropertyVisibility( NodeTraversal t, PropertyReference propRef, Visibility visibility, - boolean isClassType, JSType objectType, StaticSourceFile referenceSource, StaticSourceFile definingSource) { @@ -871,7 +928,7 @@ private void checkNonOverriddenPropertyVisibility( checkPackagePropertyVisibility(t, propRef, referenceSource, definingSource); break; case PRIVATE: - checkPrivatePropertyVisibility(t, propRef, isClassType, ownerType); + checkPrivatePropertyVisibility(t, propRef, ownerType); break; case PROTECTED: checkProtectedPropertyVisibility(t, propRef, ownerType); @@ -904,13 +961,8 @@ private void checkPackagePropertyVisibility( private void checkPrivatePropertyVisibility( NodeTraversal t, PropertyReference propRef, - boolean isClassType, @Nullable ObjectType ownerType) { - if (isClassType && isValidPrivateConstructorAccess(propRef.getParentNode())) { - return; - } - // private access is not allowed outside the file from a different // enclosing class. // TODO(tbreisacher): Should we also include the filename where ownerType is defined? @@ -951,24 +1003,6 @@ private void checkProtectedPropertyVisibility( propRef.getReadableTypeNameOrDefault())); } - /** - * Whether the given access of a private constructor is legal. - * - * For example, - * new PrivateCtor_(); // not legal - * PrivateCtor_.newInstance(); // legal - * x instanceof PrivateCtor_ // legal - * - * This is a weird special case, because our visibility system is inherited - * from Java, and JavaScript has no distinction between classes and - * constructors like Java does. - * - * We may want to revisit this if we decide to make the restrictions tighter. - */ - private static boolean isValidPrivateConstructorAccess(Node parent) { - return !parent.isNew(); - } - /** * Determines whether a deprecation warning should be emitted. * @@ -1175,6 +1209,57 @@ private JSType getTypeOfThis(Node scopeRoot) { } } + /** + * The set of ways in which JSDoc and identifier usage can interact. + * + *

Before ES6, for a given type, there was no way to differentate the declaration of the + * constructor function, namespace object, and type; the declaration of all three constructs was + * the same node. This lead to some assumptions were made about how access-control modifiers + * applied to each. + * + *

At ES6, class-syntax separated the constructor function from the namespace object and type + * declaration. This allowed finer grained control over access-control modifiers; however it broke + * some of the eariler assumptions. + * + *

This type exists to simplify maintaining both sets of assumptions. It allows other code to + * branch on behaviour in a more obvious way. + * + *

TODO(b/113127707): Make this unnecessary by better modeling or decomposing this pass. + */ + private static enum IdentifierBehaviour { + NON_CONSTRUCTOR, + ES5_CLASS_INVOCATION, + ES5_CLASS_NAMESPACE, + ES6_CLASS_INVOCATION, + ES6_CLASS_NAMESPACE; + + public static IdentifierBehaviour select(Node target) { + JSType type = target.getJSType(); + if (type == null || !type.isFunctionType()) { + // If we aren't sure what we're dealing with be more strict. + return IdentifierBehaviour.NON_CONSTRUCTOR; + } + + FunctionType ctorType = type.toMaybeFunctionType(); + if (!ctorType.isConstructor()) { + return IdentifierBehaviour.NON_CONSTRUCTOR; + } + + boolean isInvocation = NodeUtil.isInvocationTarget(target) || isExtendsTarget(target); + boolean isEs6 = (ctorType.getSource() != null) && ctorType.getSource().isClass(); + + if (!isEs6) { + return isInvocation + ? IdentifierBehaviour.ES5_CLASS_INVOCATION + : IdentifierBehaviour.ES5_CLASS_NAMESPACE; + } else { + return isInvocation + ? IdentifierBehaviour.ES6_CLASS_INVOCATION + : IdentifierBehaviour.ES6_CLASS_NAMESPACE; + } + } + } + /** * A representation of an object property reference in JS code. * @@ -1227,7 +1312,6 @@ abstract interface Builder { Builder setOverride(boolean isOverride); Builder setReadableTypeName(Supplier typeName); - PropertyReference build(); } diff --git a/test/com/google/javascript/jscomp/CheckAccessControlsEs6ClassTest.java b/test/com/google/javascript/jscomp/CheckAccessControlsEs6ClassTest.java index 104120a2bbc..107a566f5e5 100644 --- a/test/com/google/javascript/jscomp/CheckAccessControlsEs6ClassTest.java +++ b/test/com/google/javascript/jscomp/CheckAccessControlsEs6ClassTest.java @@ -506,6 +506,40 @@ public void testPrivateAccessForProperties6() { .withMessage("Access to private property prop of x.y.z.Parent not allowed here.")); } + public void testPrivateAccessToConstructorThroughNew() { + test( + srcs( + lines( + "class Foo {", // + " /** @private */", + " constructor() { }", + "}", + "", + "new Foo();"))); + } + + public void testPrivateAccessToConstructorThroughExtendsClause() { + test( + srcs( + lines( + "class Foo {", // + " /** @private */", + " constructor() { }", + "}", + "", + "class SubFoo extends Foo { }"))); + } + + public void testPrivateAccessToClass() { + test( + srcs( + lines( + "/** @private */", // + "class Foo { }", + "", + "Foo;"))); + } + public void testPrivateAccess_googModule() { test( srcs( @@ -726,6 +760,40 @@ public void testNoPrivateAccessForProperties12() { error(BAD_PRIVATE_PROPERTY_ACCESS)); } + public void testNoPrivateAccessToConstructorThroughNew() { + test( + srcs( + lines( + "class Foo {", // + " /** @private */", + " constructor() { }", + "}"), + lines("new Foo();")), + error(BAD_PRIVATE_PROPERTY_ACCESS)); + } + + public void testNoPrivateAccessToConstructorThroughExtendsClause() { + test( + srcs( + lines( + "class Foo {", // + " /** @private */", + " constructor() { }", + "}"), + lines("class SubFoo extends Foo { }")), + error(BAD_PRIVATE_PROPERTY_ACCESS)); + } + + public void testNoPrivateAccessToClass() { + test( + srcs( + lines( + "/** @private */", // + "class Foo { }"), + lines("Foo")), + error(BAD_PRIVATE_GLOBAL_ACCESS)); + } + public void testProtectedAccessForProperties1() { test( srcs( @@ -1056,6 +1124,41 @@ public void testProtectedAccessForProperties16() { "}"))); } + public void testProtectedAccessToConstructorThroughExtendsClause() { + test( + srcs( + lines( + "class Foo {", // + " /** @protected */ constructor() {}", + "}"), + lines("class OtherFoo extends Foo { }"))); + } + + public void testProtectedAccessToConstructorThroughSubclassInstanceMethod() { + test( + srcs( + lines( + "class Foo {", // + " /** @protected */ constructor() {}", + "}"), + lines( + "class OtherFoo extends Foo {", // + " bar() { new Foo(); }", + "}"))); + } + + public void testProtectedAccessToClassThroughSubclassInstanceMethod() { + test( + srcs( + lines( + "/** @protected */", // + "class Foo {}"), + lines( + "class OtherFoo extends Foo {", // + " bar() { Foo; }", + "}"))); + } + public void testProtectedAccessThroughNestedFunction() { test( srcs( @@ -1257,6 +1360,21 @@ public void testNoProtectedAccessForProperties9() { error(BAD_PROTECTED_PROPERTY_ACCESS)); } + public void testNoProtectedAccessToConstructorFromUnrelatedClass() { + test( + srcs( + lines( + "class Foo {", // + " /** @protected */", + " constructor() {}", + "}"), + lines( + "class OtherFoo {", // + " bar() { new Foo(); }", + "}")), + error(BAD_PROTECTED_PROPERTY_ACCESS)); + } + public void testNoProtectedAccessForPropertiesWithNoRhs() { test( srcs( @@ -1358,6 +1476,95 @@ public void testPackagePrivateAccessForProperties5() { error(BAD_PACKAGE_PROPERTY_ACCESS)); } + public void testPackageAccessToConstructorThroughNew() { + test( + srcs( + SourceFile.fromCode( + Compiler.joinPathParts("foo", "bar.js"), + lines( + "class Foo {", // + " /** @package */", + " constructor() {}", + "}")), + SourceFile.fromCode( + Compiler.joinPathParts("foo", "quux.js"), // + "new Foo();"))); + } + + public void testPackageAccessToConstructorThroughExtendsClause() { + test( + srcs( + SourceFile.fromCode( + Compiler.joinPathParts("foo", "bar.js"), + lines( + "class Foo {", // + " /** @package */", + " constructor() {}", + "}")), + SourceFile.fromCode( + Compiler.joinPathParts("foo", "quux.js"), // + "class SubFoo extends Foo { }"))); + } + + public void testPackageAccessToClass() { + test( + srcs( + SourceFile.fromCode( + Compiler.joinPathParts("foo", "bar.js"), + lines( + "/** @package */", // + "class Foo {}")), + SourceFile.fromCode( + Compiler.joinPathParts("foo", "quux.js"), // + "Foo;"))); + } + + public void testPackageAccessToConstructorThroughNew_fileOveriew() { + test( + srcs( + SourceFile.fromCode( + Compiler.joinPathParts("foo", "bar.js"), + lines( + "/** @fileoverview @package */", // + "", + "class Foo {", // + " constructor() {}", + "}")), + SourceFile.fromCode( + Compiler.joinPathParts("foo", "quux.js"), // + "new Foo();"))); + } + + public void testPackageAccessToConstructorThroughExtendsClause_fileOveriew() { + test( + srcs( + SourceFile.fromCode( + Compiler.joinPathParts("foo", "bar.js"), + lines( + "/** @fileoverview @package */", // + "", + "class Foo {", // + " constructor() {}", + "}")), + SourceFile.fromCode( + Compiler.joinPathParts("foo", "quux.js"), // + "class SubFoo extends Foo { }"))); + } + + public void testPackageAccessToClass_fileOveriew() { + test( + srcs( + SourceFile.fromCode( + Compiler.joinPathParts("foo", "bar.js"), + lines( + "/** @fileoverview @package */", // + "", + "class Foo {}")), + SourceFile.fromCode( + Compiler.joinPathParts("foo", "quux.js"), // + "Foo;"))); + } + public void testNoPackagePrivateAccessForProperties1() { test( srcs( @@ -1498,6 +1705,103 @@ public void testNoPackagePrivateAccessForProperties7() { error(BAD_PACKAGE_PROPERTY_ACCESS)); } + public void testNoPackgeAccessToConstructorThroughNew() { + test( + srcs( + SourceFile.fromCode( + Compiler.joinPathParts("foo", "bar.js"), + lines( + "class Foo {", // + " /** @package */", + " constructor() {}", + "}")), + SourceFile.fromCode( + Compiler.joinPathParts("baz", "quux.js"), // + "new Foo();")), + error(BAD_PACKAGE_PROPERTY_ACCESS)); + } + + public void testNoPackageAccessToConstructorThroughExtendsClause() { + test( + srcs( + SourceFile.fromCode( + Compiler.joinPathParts("foo", "bar.js"), + lines( + "class Foo {", // + " /** @package */", + " constructor() {}", + "}")), + SourceFile.fromCode( + Compiler.joinPathParts("baz", "quux.js"), // + "class SubFoo extends Foo { }")), + error(BAD_PACKAGE_PROPERTY_ACCESS)); + } + + public void testNoPackageAccessToClass() { + test( + srcs( + SourceFile.fromCode( + Compiler.joinPathParts("foo", "bar.js"), + lines( + "/** @package */", // + "class Foo {}")), + SourceFile.fromCode( + Compiler.joinPathParts("baz", "quux.js"), // + "Foo;")), + error(BAD_PACKAGE_PROPERTY_ACCESS)); + } + + public void testNoPackgeAccessToConstructorThroughNew_fileOveriew() { + test( + srcs( + SourceFile.fromCode( + Compiler.joinPathParts("foo", "bar.js"), + lines( + "/** @fileoverview @package */", // + "", + "/** @public */", + "class Foo {", + " constructor() {}", + "}")), + SourceFile.fromCode( + Compiler.joinPathParts("baz", "quux.js"), // + "new Foo();")), + error(BAD_PACKAGE_PROPERTY_ACCESS)); + } + + public void testNoPackageAccessToConstructorThroughExtendsClause_fileOveriew() { + test( + srcs( + SourceFile.fromCode( + Compiler.joinPathParts("foo", "bar.js"), + lines( + "/** @fileoverview @package */", // + "", + "/** @public */", + "class Foo {", + " constructor() {}", + "}")), + SourceFile.fromCode( + Compiler.joinPathParts("baz", "quux.js"), // + "class SubFoo extends Foo { }")), + error(BAD_PACKAGE_PROPERTY_ACCESS)); + } + + public void testNoPackageAccessToClass_fileOveriew() { + test( + srcs( + SourceFile.fromCode( + Compiler.joinPathParts("foo", "bar.js"), + lines( + "/** @fileoverview @package */", // + "", + "class Foo {}")), + SourceFile.fromCode( + Compiler.joinPathParts("baz", "quux.js"), // + "Foo;")), + error(BAD_PACKAGE_PROPERTY_ACCESS)); + } + public void testOverrideWithoutVisibilityRedeclInFileWithFileOverviewVisibilityNotAllowed_OneFile() { test( @@ -1629,7 +1933,9 @@ public void testPublicFileOverviewVisibilityDoesNotApplyToNameWithExplicitPackag "", "/** @package */", "class Foo {}")), - SourceFile.fromCode(Compiler.joinPathParts("baz", "quux.js"), "new Foo();")), + SourceFile.fromCode( + Compiler.joinPathParts("baz", "quux.js"), // + "new Foo();")), error(BAD_PACKAGE_PROPERTY_ACCESS)); } @@ -1645,7 +1951,9 @@ public void testPackageFileOverviewVisibilityDoesNotApplyToNameWithExplicitPubli " */", "/** @public */", "class Foo {}")), - SourceFile.fromCode(Compiler.joinPathParts("baz", "quux.js"), "new Foo();"))); + SourceFile.fromCode( + Compiler.joinPathParts("baz", "quux.js"), // + "new Foo();"))); } public void testPackageFileOverviewVisibilityAppliesToNameWithoutExplicitVisibility() { @@ -1660,7 +1968,9 @@ public void testPackageFileOverviewVisibilityAppliesToNameWithoutExplicitVisibil " */", "", "var Foo = class {};")), - SourceFile.fromCode(Compiler.joinPathParts("baz", "quux.js"), "new Foo();")), + SourceFile.fromCode( + Compiler.joinPathParts("baz", "quux.js"), // + "new Foo();")), error(BAD_PACKAGE_PROPERTY_ACCESS)); } @@ -1704,7 +2014,10 @@ public void testPackageFileOverviewVisibilityAppliesToNameWithoutExplicitVisibil " bar() {}", "};")), SourceFile.fromCode( - Compiler.joinPathParts("baz", "quux.js"), "var foo = new Foo();" + "foo.bar();")), + Compiler.joinPathParts("baz", "quux.js"), + lines( + "var foo = new Foo();", // + "foo.bar();"))), error(BAD_PACKAGE_PROPERTY_ACCESS)); } @@ -1880,8 +2193,6 @@ public void testAccessOfStaticMethodOnPrivateClass() { " static create() { return new Foo(); }", "}"), lines("Foo.create()")), - // TODO(b/80580110): This error is probably correct, but will break the depot, so - // we should suppress it until we fix usages. error(BAD_PRIVATE_GLOBAL_ACCESS)); } @@ -1897,8 +2208,6 @@ public void testAccessOfStaticMethodOnPrivateQualifiedConstructor() { " static create() { return new goog.Foo(); }", "}"), lines("goog.Foo.create()")), - // TODO(b/80580110): This error is probably correct, but will break the depot, so - // we should suppress it until we fix usages. error(BAD_PRIVATE_PROPERTY_ACCESS)); }