intellij-community icon indicating copy to clipboard operation
intellij-community copied to clipboard

IDEA-323330

Open AB-xdev opened this issue 2 years ago • 7 comments

Fixes IDEA-323330

Summary of changes:

  • Made the affected classes and constructors public
  • Also changed the visibility of methods from private to protected so that they can be overriden

AB-xdev avatar Jun 22 '23 14:06 AB-xdev

Why do you need these classes public? What functionality are you trying to achieve in your plugin?

BasLeijdekkers avatar Jul 10 '23 15:07 BasLeijdekkers

The answers for these questions are already inside IDEA-323330.

Why do you need these classes public?

So that I don't get a warning when publishing to your marketplace. Also all other classes inside these packages are public...

What functionality are you trying to achieve in your plugin?

Code formatting when e.g. saving & much more. Please read the plugin description (in the issue) for more information.

AB-xdev avatar Jul 11 '23 06:07 AB-xdev

Please point out the plugin description in the issue, I don't see it.

These classes have nothing to do with either code formatting or saving. And there are likely many ways to avoid the warning (including not using the classes). So what are you doing with these classes that you need them public? What changes do you make in their behaviour?

Making the classes public is acceptable, but making every member protected seems overkill. These members are not designed to be overridden. Perhaps there is some other way to achieve what you need.

BasLeijdekkers avatar Jul 11 '23 07:07 BasLeijdekkers

Please point out the plugin description in the issue, I don't see it.

Here is our plugin and here the original one that we forked.

How to get there

grafik

grafik or grafik (the Readme contains the same description as on the marketplace)

These classes have nothing to do with either code formatting or saving.

The classes are used for running inspections and fixing them. They are used inside the following actions of the plugin:

  • Add class qualifier to static member access outside declaring class uses UnqualifiedStaticCallVisitor
    • https://github.com/xdev-software/intellij-plugin-save-actions/blob/456210101ad7466aa8d6a0f25e224d397dbccea6/src/main/java/software/xdev/saveactions/processors/java/JavaProcessor.java#L74-L75
  • Change visibility of field or method to lower access uses AccessCanBeTightenedInspection
    • https://github.com/xdev-software/intellij-plugin-save-actions/blob/456210101ad7466aa8d6a0f25e224d397dbccea6/src/main/java/software/xdev/saveactions/processors/java/JavaProcessor.java#L111-L112

And there are likely many ways to avoid the warning (including not using the classes).

If we shouldn't use these classes then what are alternatives?

So what are you doing with these classes that you need them public?

So that we can instantiate them and don't have to copy the code.

What changes do you make in their behavior?

As far as I have seen there were currently no changes made to the behavior by the original authors. We also didn't modify their behavior.

Making the classes public is acceptable, but making every member protected seems overkill. These members are not designed to be overridden. Perhaps there is some other way to achieve what you need.

I was thinking that maybe others might need to do changes to these members or we do somewhere in the future that's why I made them overrideable. Otherwise a new PR would be required then.

However I can revert making the protected if this is fine for you :)

AB-xdev avatar Jul 11 '23 08:07 AB-xdev

I looked into this some more, and it seems to me you could just move your CustomUnqualifiedStaticUsageInspection to a different package without any problems. It doesn't use any package local members.

Your CustomAccessCanBeTightenedInspection uses just a single package local method which I now have public (will appear in the repo soon). Or you could just reimplement this one rather short method. Then this class will also be moveable to a different package.

Doing the changes above will avoid the verification warnings when publishing the plugin. Therefore I don't think the changes in this pull request are necessary. Do you disagree?

BasLeijdekkers avatar Jul 11 '23 09:07 BasLeijdekkers

This could be a workaround however the code would still be duplicated, which has disadvantages like bloating the plugin and needing to update the duplicated code regularly.

I still think that the best solution would be if we can use the code that is also used inside IntelliJ and by that just simply making it public.


As far as I have seen there were currently no changes made to the behavior by the original authors. We also didn't modify their behavior.

I also rechecked the classes in detail and came to the following exact changes that are required so that we can remove the duplicated code:

  • UnqualifiedStaticCallVisitor needs to be public (currently private) so that it can be instantiated
    • isUnqualifiedStaticAccess needs to be overriden (currently private) -> it must at least be protected
  • AccessCanBeTightenedInspection needs to be public (currently package-private) so that it can be instantiated
    • Additionally I just found out that AccessCanBeTightenedInspection uses package-private members of VisibilityInspection which are not accessible due to different classloaders (as is the case when using plugins), they likely should also be public (see also https://github.com/xdev-software/intellij-plugin-save-actions/issues/14); Affected:
      • int getMinVisibilityLevel(PsiMember)
      • boolean containsReferenceTo(PsiElement, PsiElement) (static)

AB-xdev avatar Jul 11 '23 15:07 AB-xdev

I rebased and updated the PR accordingly.

AB-xdev avatar Aug 02 '23 09:08 AB-xdev