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

Remove unused suppress warning annotations removes USED supress warning annotations. #87

Open
Delfic opened this issue Jun 27, 2017 · 30 comments

Comments

@Delfic
Copy link

Delfic commented Jun 27, 2017

Example

public Button getActionButton(final String actionButtonId) {
    Button button = null;
    final ActionToolbar actionToolbar = this.getActionToolbar();

    if (actionToolbar != null && CollectionUtils.isNotEmpty(actionToolbar.getChildren())) {
        for (final Component component : new ArrayList<Component>(actionToolbar.getChildren())) {
            if ((component instanceof Button) && (StringUtils.equals(actionButtonId, component.getId()))) {
                button = (Button) component;

                break;
            }
        }
    }

    return button;
}

actionToolbar.getChildren() is unchecked but the @SuppressWarnings("unchecked") was removed upon save even thought it was being used.

@dubreuia
Copy link
Owner

Thanks @Delfic I'll look into it.

This is somewhat similar to #79: I don't actually control what the processor does, I'm just calling Intellij code. We'll have two choices either (1) open a bug a Jetbrains (2) re implement the processor myself

@NikolayMetchev
Copy link

Have you opened a bug with Jetbrains?

@Delfic
Copy link
Author

Delfic commented Oct 18, 2017

I didn't open a ticked. I've searched and did not find one. https://youtrack.jetbrains.com/issues?q=supressWarnings

@NikolayMetchev
Copy link

I didn't see an inspection that corresponded to the functionality you are referring to. The unused code inspection has properties for fields, methods, etc... but no annotations. It makes me think that whatever code you are invoking is unused. Can you reproduce the bug in Idea?

@Delfic
Copy link
Author

Delfic commented Oct 18, 2017

Actually I cannot, I've never found such functionality on IDEA. The closest I've found was an inspection for suppression that suggests to remove the annotations. Maybe it's invoking such inspection with the suggested resolution...

@NikolayMetchev
Copy link

So is it possible that it is not a bug in the intellij code but there is some extra context that needs to be set up or passed in somehow? As it stands this feature in the save actions plugin cannot be enabled and therefore is useless.

@Delfic
Copy link
Author

Delfic commented Oct 18, 2017

@dubreuia What's your say in this? Is there any documentation on the API for the IDEA code you used? Is it the feature that I think it is?

@NikolayMetchev Not quite useless. One might want to not use any suppress warnings and might want to go through code and remove it. Not useless but might need a rebranding to "remove suppressWarnings annotations"

@dubreuia
Copy link
Owner

Hey @Delfic I'm using the quick fix class https://github.com/JetBrains/intellij-community/blob/306d705e1829bd3c74afc2489bfb7ed59d686b84/plugins/InspectionGadgets/testsrc/com/siyeh/ig/maturity/SuppressionAnnotationInspectionTest.java

I have no idea why it doesn't work the intended way, and honestly this is really low in the task list right now. You can look into it if you want! My best guess would be I'm not using the quick fix properly.

@Delfic
Copy link
Author

Delfic commented Oct 22, 2017

Hello @dubreuia. I would gladly help out on this one. Quick question: is the link you provided the right one? That seems like an almost empty test case and not a "quick fix class".

I've tried looking for the LightInspectionTestCase to check on the doTest method but to no avail :(

Help me out here: where, in save-actions, is the call to this particular fix?

@dubreuia
Copy link
Owner

Sorry, this is the good link: https://github.com/JetBrains/intellij-community/blob/d19918722bcc19d449f0fdd762027db9cec3bf10/plugins/InspectionGadgets/src/com/siyeh/ig/maturity/SuppressionAnnotationInspection.java

  • it is instancited here: com/dubreuia/processors/java/ProcessorFactory.java:58
  • called here: com/dubreuia/processors/java/InspectionProcessor.java:96

I'll add some documentation to help you setup the environment

@dubreuia
Copy link
Owner

@Delfic
Copy link
Author

Delfic commented Oct 22, 2017

@dubreuia Thank for the documentation. I think I've found the bug.

You're calling a class that implements the fixes for this inspections:
inspectionsuppresion

Regarding the description of the action and what started this bug report in the first place, I believe what you wanted to achieve was using this code analysis (not direct inspection, only available through Analyze)
redundantsuppression

I've made tests and this is the one we want.

Which is implemented here
com.intellij.codeInspection.RedundantSuppressInspection

Could not find this one on the git project you linked but it's present in the intellij-core-analysis.jar in the intellij-core-163.7743.44.zip which is found on the link you provided in the documentation.

This is a GlobalInspectionTool rather than a LocalInspection so some tweaking will be necessary.

@Delfic
Copy link
Author

Delfic commented Oct 22, 2017

@dubreuia I've created a PR.
#115

If anything is amiss, let me know. Thanks

@dubreuia
Copy link
Owner

Thanks @Delfic I'll look into it.

dubreuia added a commit that referenced this issue Oct 23, 2017
@Delfic
Copy link
Author

Delfic commented Oct 23, 2017

Thank you for the opportunity :)

@Delfic Delfic closed this as completed Oct 23, 2017
@dubreuia dubreuia reopened this Oct 23, 2017
@dubreuia
Copy link
Owner

NPE on save #115

@dubreuia
Copy link
Owner

@Delfic I can't find why it doesn't work, I've reverted the change on master, you'll need to do a new PR

@dubreuia dubreuia reopened this Oct 26, 2017
@dubreuia dubreuia mentioned this issue Oct 27, 2017
@nikowitt
Copy link

nikowitt commented Jan 11, 2018

From what I read from the thread, this issue is still completely open, right? I've just stumbled across this one with activated "Remove unused suppress warning annotation"

@Delfic
Copy link
Author

Delfic commented Jan 11, 2018

Yup, Still Open since we haven't figured out yet why the NPE occurs.

@dubreuia
Copy link
Owner

Not sure either, you need to debug it and see with it occurs in intellij code. I'm pretty sure it has to do with the fact that instead of using com.intellij.codeInspection.ex.LocalInspectionToolWrapper like the other inspections, you're using the GlobalToolWrapper. There's probably a context variable to setup somewhere.

@pablomusumeci
Copy link

Is there any workaround for this issue?

@dubreuia
Copy link
Owner

Hey @pablomusumeci, not for now no. But @Delfic submited a PR that was a step in the right direction, if you want to look at it and resubmit, I gladly take PRs.

@dubreuia
Copy link
Owner

dubreuia commented Aug 8, 2018

I stumbled on this one again today: this annotation got deleted com/dubreuia/processors/java/InspectionProcessor.java:90

@MartinX3
Copy link

MartinX3 commented Apr 5, 2019

+1
IntelliJ Ultimate 2019.1
Save Actions v1.4.0

(Had it last year too.)

@koflerdavidgrawe
Copy link

koflerdavidgrawe commented Jul 17, 2019

+1
IntelliJ Ultime 2019.1.3
Save Action 1.5.0

A workaround would be necessary asap :)

@Delfic
Copy link
Author

Delfic commented May 24, 2020

+1
IntelliJ Ultime 2019.1.3
Save Action 1.5.0

A workaround would be necessary asap :)

As or a workaround you can run the inspection by name "Redundant suppression"

@Delfic
Copy link
Author

Delfic commented May 24, 2020

I did a couple of attempts, one of which I didn't get far and opened a draft PR about it.
The other one, I applied the same strategy of doing a refactor on the processor and doing a refactor and I've hit two bumps:
One it doesn't even find @SuppressWarnings it only finds @java.lang.SuppressWarnings
I changed the unit test files to have the fully qualified version and continued with the debugging and got to a point where it finds the scopes but now it needs some inspection tools available in the inspection manager so that they actually check if the scope is being wrongfully suppressed or not. So maybe the way the inspection is called it just won't work because it needs all the inspections of all the possible suppressed scopes to be there to confirm if those scopes can be safely removed from the SuppressWarnings annotation.

Are those inspections missing from the overall save-actions context or just from the unit test context?

Tomorrow I'll try building and installing the plug-in from my branch to see if it works in spite of the unit tests failing.

@Delfic
Copy link
Author

Delfic commented May 24, 2020

So it was easier to build and install, just install the zip inside the distribution folder. So... It doesn't work installing the distributed plugin after the changes I'm testing. Happy thought is that the unit test works :)

So to solve this we would need to have all the tools that inspect for the suppressed scopes loaded.
Is that feasible?

@dubreuia
Copy link
Owner

@dubreuia dubreuia reopened this Sep 11, 2020
@AndroidDeveloperLB
Copy link

I can confirm this issue. See attached if this helps:

2020-11-26_15-08-19.zip
My Application.zip

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

8 participants