Skip to content

Commit

Permalink
Disable all visibility checks on ES6 class constructor declarations.
Browse files Browse the repository at this point in the history
We don't consider them overrides, so there's no way they should ever be in violation. Only accesses to a class constructor can be violations.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=243157588
  • Loading branch information
nreid260 authored and lauraharker committed Apr 12, 2019
1 parent 03d791d commit a240bdd
Show file tree
Hide file tree
Showing 2 changed files with 65 additions and 9 deletions.
42 changes: 33 additions & 9 deletions src/com/google/javascript/jscomp/CheckAccessControls.java
Expand Up @@ -631,7 +631,7 @@ private boolean isPackageAccessAllowed(Var var, Node name) {
return srcPackage != null && refPackage != null && Objects.equals(srcPackage, refPackage);
}

private void checkOverriddenPropertyVisibilityMismatch(
private void checkPropertyOverrideVisibilityIsSame(
Visibility overriding,
Visibility overridden,
@Nullable Visibility fileOverview,
Expand Down Expand Up @@ -762,8 +762,32 @@ private ObjectType boxedOrUnknown(@Nullable JSType type) {
return typeOrUnknown(dereference(type));
}

/** Reports an error if the given property is not visible in the current context. */
/**
* Reports an error if the given property is not visible in the current context.
*
* <p>This method covers both:
*
* <ul>
* <li>accesses to properties during execution
* <li>overrides of properties during declaration
* </ul>
*
* TODO(nickreid): Things would probably be a lot simpler, though a bit duplicated, if these two
* concepts were separated. Much of the underlying logic could stop checking various inconsistent
* definitions of "is this an override".
*/
private void checkPropertyVisibility(PropertyReference propRef) {
if (NodeUtil.isEs6ConstructorMemberFunctionDef(propRef.getSourceNode())) {
// Class ctor *declarations* can never violate visibility restrictions. They are not
// accesses and we don't consider them overrides.
//
// TODO(nickreid): It would be a lot cleaner if we could model this using `PropertyReference`
// rather than defining a special case here. I think the problem is that the current
// implementation of this method conflates "override" with "declaration". But that only works
// because it ignores cases where there's no overridden definition.
return;
}

JSType rawReferenceType = typeOrUnknown(propRef.getReceiverType()).autobox();
ObjectType referenceType = castToObject(rawReferenceType);

Expand Down Expand Up @@ -798,7 +822,7 @@ private void checkPropertyVisibility(PropertyReference propRef) {
if (isOverride) {
Visibility overriding = getOverridingPropertyVisibility(propRef);
if (overriding != null) {
checkOverriddenPropertyVisibilityMismatch(
checkPropertyOverrideVisibilityIsSame(
overriding, visibility, fileOverviewVisibility, propRef);
}
}
Expand All @@ -824,10 +848,10 @@ private void checkPropertyVisibility(PropertyReference propRef) {
if (isOverride) {
boolean sameInput = referenceSource != null
&& referenceSource.getName().equals(definingSource.getName());
checkOverriddenPropertyVisibility(
checkPropertyOverrideVisibility(
propRef, visibility, fileOverviewVisibility, reportType, sameInput);
} else {
checkNonOverriddenPropertyVisibility(
checkPropertyAccessVisibility(
propRef, visibility, reportType, referenceSource, definingSource);
}
}
Expand All @@ -853,7 +877,7 @@ private void checkEs6ConstructorInvocationVisibility(Node target) {

// 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.
// `checkPropertyAccessVisibility` method which actually reports violations.
PropertyReference fauxCtorRef =
PropertyReference.builder()
.setSourceNode(target)
Expand All @@ -878,7 +902,7 @@ private void checkEs6ConstructorInvocationVisibility(Node target) {
? Visibility.PUBLIC
: annotatedCtorVisibility;

checkNonOverriddenPropertyVisibility(
checkPropertyAccessVisibility(
fauxCtorRef,
effectiveCtorVisibility,
ctorType,
Expand All @@ -899,7 +923,7 @@ private static boolean propertyIsDeclaredButNotPrivate(PropertyReference propRef
return true;
}

private void checkOverriddenPropertyVisibility(
private void checkPropertyOverrideVisibility(
PropertyReference propRef,
Visibility visibility,
Visibility fileOverviewVisibility,
Expand Down Expand Up @@ -930,7 +954,7 @@ private void checkOverriddenPropertyVisibility(
}
}

private void checkNonOverriddenPropertyVisibility(
private void checkPropertyAccessVisibility(
PropertyReference propRef,
Visibility visibility,
JSType objectType,
Expand Down
Expand Up @@ -2180,6 +2180,38 @@ public void testOverrideWithVisibilityRedeclInFileWithFileOverviewVisibilityOk_T
"};")));
}

@Test
public void testConstructorVisibility_canBeNarrowed() {
test(
srcs(
lines(
"class Foo {", //
" /** @public */",
" constructor() {}",
"};"),
lines(
"class Bar extends Foo {", //
" /** @private */",
" constructor() {}",
"};")));
}

@Test
public void testConstructorVisibility_canBeExpanded() {
test(
srcs(
lines(
"class Foo {", //
" /** @protected */",
" constructor() {}",
"};"),
lines(
"class Bar extends Foo {", //
" /** @public */",
" constructor() {}",
"};")));
}

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

0 comments on commit a240bdd

Please sign in to comment.