Skip to content

Commit

Permalink
Modify visibility checking for ES6 constructor invocation
Browse files Browse the repository at this point in the history 
Treat such constructors as `@public` regardless of any visibility declared by a supertype.

The new behaviour is distinct because visibility is not inherited, as it would be for other properties.
Both implicit and explicit constructors default to `@public`.
`@fileoverview` visibility is still considered before defaulting.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=215975039
  • Loading branch information
@nreid260 @brad4d
nreid260 authored and brad4d committed Oct 8, 2018
1 parent 3b0f001 commit 931cea8
Show file tree
Hide file tree
Showing 2 changed files with 150 additions and 9 deletions.
47 changes: 41 additions & 6 deletions src/com/google/javascript/jscomp/CheckAccessControls.java
View file
Expand Up @@ -391,7 +391,7 @@ private void checkVisibility(
IdentifierBehaviour identifierBehaviour,
NodeTraversal traversal) {
if (identifierBehaviour.equals(IdentifierBehaviour.ES6_CLASS_INVOCATION)) {
checkEs6ConstructorVisibility(node, traversal);
checkEs6ConstructorInvocationVisibility(node, traversal);
}

if (!identifierBehaviour.equals(IdentifierBehaviour.ES5_CLASS_NAMESPACE)) {
Expand Down Expand Up @@ -844,22 +844,54 @@ private void checkPropertyVisibility(NodeTraversal t, PropertyReference propRef)
*
* <p>Precondition: {@code target} has an ES6 class {@link JSType}.
*/
private void checkEs6ConstructorVisibility(Node target, NodeTraversal traversal) {
private void checkEs6ConstructorInvocationVisibility(Node target, NodeTraversal traversal) {
FunctionType ctorType = target.getJSType().toMaybeFunctionType();
ObjectType prototypeType = ctorType.getPrototype();

// Check constructor visibility by pretending we're accessing `Foo.prototype.constructor`.
// We use the class definition site because classes automatically get a implicit constructor,
// so there may not be a definition node.
@Nullable Node classDefinition = ctorType.getSource();

@Nullable
StaticSourceFile definingSource =
(classDefinition == null)
? null
: AccessControlUtils.getDefiningSource(classDefinition, prototypeType, "constructor");

// Synthesize a `PropertyReference` for this constructor call as if we're accessing
// `Foo.prototype.constructor`. This object allows us to reuse the
// `checkNonOverriddenPropertyVisibility` method which actually reports violations.
PropertyReference fauxCtorRef =
PropertyReference.builder()
.setSourceNode(target)
.setName("constructor")
.setReceiverType(ctorType.getPrototype())
.setReceiverType(prototypeType)
.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);
Visibility annotatedCtorVisibility =
// This function defaults to `INHERITED` which isn't what we want here, but it does handle
// combining inline and `@fileoverview` visibilities.
getEffectiveVisibilityForNonOverriddenProperty(
fauxCtorRef,
prototypeType,
defaultVisibilityForFiles.get(definingSource),
enforceCodingConventions ? compiler.getCodingConvention() : null);
Visibility effectiveCtorVisibility =
annotatedCtorVisibility.equals(Visibility.INHERITED)
? Visibility.PUBLIC
: annotatedCtorVisibility;

checkNonOverriddenPropertyVisibility(
traversal,
fauxCtorRef,
effectiveCtorVisibility,
ctorType,
target.getStaticSourceFile(),
definingSource);
}

private static boolean propertyIsDeclaredButNotPrivate(PropertyReference propRef) {
Expand Down Expand Up @@ -1464,7 +1496,10 @@ private static Visibility getEffectiveVisibilityForNonOverriddenProperty(
}
Visibility raw = Visibility.INHERITED;
if (objectType != null) {
raw = objectType.getOwnPropertyJSDocInfo(propertyName).getVisibility();
JSDocInfo jsdoc = objectType.getOwnPropertyJSDocInfo(propertyName);
if (jsdoc != null) {
raw = jsdoc.getVisibility();
}
}
JSType type = propRef.getJSType();
boolean createdFromGoogProvide = (type != null && type.isLiteralObject());
Expand Down
112 changes: 109 additions & 3 deletions test/com/google/javascript/jscomp/CheckAccessControlsEs6ClassTest.java
View file
Expand Up @@ -1953,6 +1953,7 @@ public void testNoPackageAccessToClass_fileOveriew() {
error(BAD_PACKAGE_PROPERTY_ACCESS));
}

@Test
public void
testOverrideWithoutVisibilityRedeclInFileWithFileOverviewVisibilityNotAllowed_OneFile() {
test(
Expand All @@ -1975,6 +1976,7 @@ public void testNoPackageAccessToClass_fileOveriew() {
error(BAD_PROPERTY_OVERRIDE_IN_FILE_WITH_FILEOVERVIEW_VISIBILITY));
}

@Test
public void
testOverrideWithoutVisibilityRedeclInFileWithFileOverviewVisibilityNotAllowed_TwoFiles() {
test(
Expand Down Expand Up @@ -2091,7 +2093,7 @@ public void testPublicFileOverviewVisibilityDoesNotApplyToNameWithExplicitPackag
"class Foo {}")),
SourceFile.fromCode(
Compiler.joinPathParts("baz", "quux.js"), //
"new Foo();")),
"Foo;")),
error(BAD_PACKAGE_PROPERTY_ACCESS));
}

Expand All @@ -2110,7 +2112,7 @@ public void testPackageFileOverviewVisibilityDoesNotApplyToNameWithExplicitPubli
"class Foo {}")),
SourceFile.fromCode(
Compiler.joinPathParts("baz", "quux.js"), //
"new Foo();")));
"Foo;")));
}

@Test
Expand All @@ -2128,10 +2130,11 @@ public void testPackageFileOverviewVisibilityAppliesToNameWithoutExplicitVisibil
"var Foo = class {};")),
SourceFile.fromCode(
Compiler.joinPathParts("baz", "quux.js"), //
"new Foo();")),
"Foo;")),
error(BAD_PACKAGE_PROPERTY_ACCESS));
}

@Test
public void
testPackageFileOverviewVisibilityDoesNotApplyToPropertyWithExplicitPublicVisibility() {
test(
Expand All @@ -2144,7 +2147,11 @@ public void testPackageFileOverviewVisibilityAppliesToNameWithoutExplicitVisibil
" * @package",
" */",
"",
"/** @public */", // The class must be visible.
"Foo = class {",
" /** @public */", // The constructor must be visible.
" constructor() { }",
"",
" /** @public */",
" bar() {}",
"};")),
Expand All @@ -2155,6 +2162,7 @@ public void testPackageFileOverviewVisibilityAppliesToNameWithoutExplicitVisibil
"foo.bar();"))));
}

@Test
public void
testPublicFileOverviewVisibilityDoesNotApplyToPropertyWithExplicitPackageVisibility() {
test(
Expand Down Expand Up @@ -2213,7 +2221,11 @@ public void testPackageFileOverviewVisibilityAppliesToPropertyWithoutExplicitVis
" * @package",
" */",
"",
"/** @public */", // The class must be visible.
"Foo = class {",
" /** @public */", // The constructor must be visible.
" constructor() { }",
"",
" bar() {}",
"}")),
SourceFile.fromCode(
Expand All @@ -2236,7 +2248,11 @@ public void testFileOverviewVisibilityComesFromDeclarationFileNotUseFile() {
" * @package",
" */",
"",
"/** @public */", // The class must be visible.
"Foo = class {",
" /** @public */", // The constructor must be visible.
" constructor() { }",
"",
" bar() {}",
"}")),
SourceFile.fromCode(
Expand All @@ -2252,6 +2268,96 @@ public void testFileOverviewVisibilityComesFromDeclarationFileNotUseFile() {
error(BAD_PACKAGE_PROPERTY_ACCESS));
}

@Test
public void testImplicitSubclassConstructor_doesNotInheritVisibility_andIsPublic() {
test(
srcs(
lines(
"class Foo {", //
" /** @private */",
" constructor() { }",
"}",
"",
// The implict constructor for `SubFoo` should be treated as public.
"class SubFoo extends Foo { }"),
// So we get no warning for using it here.
lines("new SubFoo();")));
}

@Test
public void testImplicitSubclassConstructor_doesNotInheritVisibility_andUsesFileOverview() {
test(
srcs(
SourceFile.fromCode(
Compiler.joinPathParts("foo", "bar.js"),
lines(
"/**",
" * @fileoverview",
" * @package",
" */",
"",
"class Foo {", //
" /** @private */",
" constructor() { }",
"}",
"",
// The implict constructor for `SubFoo` should be trated as package.
"/** @public */", // The class must be visible.
"class SubFoo extends Foo { }")),
SourceFile.fromCode(Compiler.joinPathParts("baz", "quux.js"), lines("new SubFoo();"))),
error(BAD_PACKAGE_PROPERTY_ACCESS));
}

@Test
public void testUnannotatedSubclassConstructor_doesNotInheritVisibility_andIsPublic() {
test(
srcs(
lines(
"class Foo {", //
" /** @private */",
" constructor() { }",
"}",
"",
"class SubFoo extends Foo {",
// The unannotated constructor for `SubFoo` should be treated as public.
" constructor() {",
" super();",
" }",
"}"),
// So we get no warning for using it here.
lines("new SubFoo();")));
}

@Test
public void testUnannotatedSubclassConstructor_doesNotInheritVisibility_andUsesFileOverview() {
test(
srcs(
SourceFile.fromCode(
Compiler.joinPathParts("foo", "bar.js"),
lines(
"/**",
" * @fileoverview",
" * @package",
" */",
"",
"class Foo {", //
" /** @private */",
" constructor() { }",
"}",
"",
"/** @public */", // The class must be visible.
"class SubFoo extends Foo {",
// The unannotated constructor for `SubFoo` should be treated as package.
" constructor() {",
" super();",
" }",
"}")),
SourceFile.fromCode(
Compiler.joinPathParts("baz", "quux.js"), //
lines("new SubFoo();"))),
error(BAD_PACKAGE_PROPERTY_ACCESS));
}

@Test
public void testNoExceptionsWithBadConstructors1() {
test(
Expand Down

0 comments on commit 931cea8

Please sign in to comment.