Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

馃彈 馃悰 Enable accessControls checks for JS code (and fix errors) #26328

Merged
merged 8 commits into from
Jan 15, 2020
Merged

馃彈 馃悰 Enable accessControls checks for JS code (and fix errors) #26328

merged 8 commits into from
Jan 15, 2020

Conversation

rsimha
Copy link
Contributor

@rsimha rsimha commented Jan 13, 2020

The accessControls closure compiler visibility check rule is currently ignored during gulp check-types. Turning on the rule results in about 350+ annotation errors.

This PR enables the rule and fixes all incorrect annotation errors. It took only ~25 annotation changes to fix all 350+ errors.

Reference: https://github.com/google/closure-compiler/wiki/Annotating-JavaScript-for-the-Closure-Compiler#visibility-checks

Fixes #26307

@amp-owners-bot
Copy link

Hey @newmuis, these files were changed:

  • extensions/amp-story/0.1/amp-story-share.js
  • extensions/amp-story/0.1/amp-story-store-service.js
  • extensions/amp-story/0.1/embed-mode.js
  • extensions/amp-story/1.0/amp-story-draggable-drawer.js
  • extensions/amp-story/1.0/amp-story-embedded-component.js
  • extensions/amp-story/1.0/amp-story-share.js
  • extensions/amp-story/1.0/amp-story-store-service.js
  • extensions/amp-story/1.0/embed-mode.js
  • extensions/amp-story/1.0/story-analytics.js

@erwinmombay
Copy link
Member

@rsimha is protected the right visibility? it says that @protected should only be visible in subclasses or w/in the same file correct? most of these seem to be functions exported and used in a different module

@rsimha
Copy link
Contributor Author

rsimha commented Jan 13, 2020

@rsimha is protected the right visibility? it says that @protected should only be visible in subclasses or w/in the same file correct? most of these seem to be functions exported and used in a different module

I believe the second rule under @protected is at play here (static methods and instance methods of any subclass of the class on which the property is defined).

Since changing the annotations to @protected satisfies gulp check-types, I didn't think it was necessary to make the access @public.

@erwinmombay
Copy link
Member

@rsimha hmm i guess this might be a problem with nomenclature for me. those exported functions aren't static methods/instance methods to me (implies attached to a class)

Copy link

@dreamofabear dreamofabear left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for fixing this.

Copy link
Contributor

@gmajoulet gmajoulet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Story OWNERS approval

extensions/amp-story/1.0/story-analytics.js Outdated Show resolved Hide resolved
Copy link
Contributor

@jridgewell jridgewell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think every top level value that's marked @protected should be @public (or better yet, just delete the annotation) if @package does not work for it.

ads/google/a4a/line-delimited-response-handler.js Outdated Show resolved Hide resolved
src/service/owners-interface.js Outdated Show resolved Hide resolved
@rsimha
Copy link
Contributor Author

rsimha commented Jan 14, 2020

I think every top level value that's marked @protected should be @public (or better yet, just delete the annotation) if @package does not work for it.

Done. There were a few places where @package worked. For the rest, I removed the @protected annotation.

@rsimha
Copy link
Contributor Author

rsimha commented Jan 15, 2020

Pretty sure I've addressed all feedback. Can I get a final review?

@rsimha rsimha merged commit 5842687 into ampproject:master Jan 15, 2020
@rsimha rsimha deleted the 2020-01-13-CheckAccessControls branch January 15, 2020 20:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enable some form of protected/package/private access control in check-type
8 participants