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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RestrictTo] @RestrictTo cannot be used outside the support library #812

Closed
vanniktech opened this issue Nov 6, 2017 · 9 comments
Closed

Comments

@vanniktech
Copy link

The following class gets flagged:

@RestrictTo(LIBRARY) public final class Utils {
  public static void raiseEvent(final GeoFire geoFire, final Runnable runnable) {
    geoFire.raiseEvent(runnable);
  }

  private Utils() {
    throw new AssertionError("No instances.");
  }
}

with [RestrictTo] @RestrictTo cannot be used outside the support library. The package of the utility class is com.firebase.geofire while all of the other things are in a different package. I basically need the Utility class to call a package private function. Is this a false positive?

@liutikas
Copy link

RestrictTo check essentially breaks use of @RestrictTo annotation since it can only be used by android.support.* with this on by default. This annotation is general purpose and devs should be able to use it in their own projects.

ronshapiro pushed a commit that referenced this issue Nov 29, 2017
Fixes #812

RELNOTES: Disable RestrictToEnforcer by default (fixes #812)

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=177328012
@eaftan eaftan closed this as completed in b8fd9bf Nov 30, 2017
@ZacSweers
Copy link

What was the resolution on this? Disabling it unblocks people but the check itself seems still wrong, unless google internally prevents anyone from the support library from using that annotation?

@liutikas
Copy link

liutikas commented Dec 3, 2017

It was simply disabled. The intention of this rule was to prevent apps within Google from using internal android.support.* library methods. Sadly, as implemented it blocks anyone from using @RestrictTo annotation. However, it is hard to implement this rule properly without deeper knowledge of the build system. If you have no concept of "library" or "library group" (both maven concepts) then you cannot know what uses should be restricted. As is, error-prone does not know which class is part of which library.

@vanniktech
Copy link
Author

I'm also not happy with the 'fix' of disabling this rule.

@liutikas
Copy link

liutikas commented Dec 3, 2017 via email

@vanniktech
Copy link
Author

I'd like to understand why this was even added in the first place and what's the benefit.

@JakeWharton
Copy link
Contributor

JakeWharton commented Dec 3, 2017 via email

@eaftan
Copy link
Contributor

eaftan commented Dec 4, 2017

I'll try to summarize the internal discussion on this.

@RestrictTo was not intended to be general purpose. It has semantics that depend on Gradle-specific things like LIBRARY or LIBRARY_GROUP. We wanted to enforce this inside Google, where we use the Bazel build system, and the only solution was to restrict its use to libraries where we know their structure, such as the support library.

Android team is also planning to move away from @RestrictTo: https://issuetracker.google.com/issues/68153195

Given all that, I don't this this makes sense for open-source users, and we should make it Google-internal.

@eaftan eaftan reopened this Dec 4, 2017
facebook-github-bot pushed a commit to facebook/litho that referenced this issue Jan 8, 2018
Summary:
Fixes #299

See google/error-prone#812 (comment) for
further explanation.

Reviewed By: IanChilds

Differential Revision: D6674126

fbshipit-source-id: c811e9fd0039835488bad70751f02a33f2e9efcd
@vanniktech
Copy link
Author

Has there been any progress on this issue? Marking it as Google internal seems fine to me and should be trivial.

ronshapiro pushed a commit that referenced this issue Dec 18, 2018
Fixes #812

RELNOTES: Make RestrictToEnforcer internal-only.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=225617145
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants