eclipse-cs icon indicating copy to clipboard operation
eclipse-cs copied to clipboard

NeedBracesCheck: Quickfix is broken since 8.36.1

Open rnveach opened this issue 4 years ago • 7 comments

Since 8.36.1, you can no longer do a quick fix on NeedBracesCheck violations in the problem view. Quick fix is completely grayed out and does nothing when clicking.

Confirmed by installing 8.35 , verifying quick fix works, resetting eclipse (from pre-Checkstyle install save), installing 8.36.1, verifying quick fix does not work. Both versions showed the same checkstyle error after doing a clean build.

Input file:

package test;

/** Main. */
public class Main {
  /** main. */
  public static void main(String[] args) {
    // if (true)
    // System.out.println("test");
    if (true)
      System.out.println("test");
  }
}

Configuration: Google checks.

rnveach avatar Sep 01 '21 02:09 rnveach

I am assuming all quick fixes are now not working. I only confirmed the one.

8.36.1 still has the XML files that detail the quick fix classes that made it work in 8.35. The only difference should be the inclusion of checkstyle's meta data.

I am assuming this is happening because the meta data from checkstyle is taking priority over anything else that is imported. Since checkstyle has no clue about quick fixes, it is defaulting to null (no quick fix available) and leaving it as that with no type of merging.

It looks like commit 527a848987a403f54a06c17c87073fc5f07a05a3 (when made compilable) still supports quick fixes before the final meta data commit. This may be because of the existence of the updateRuleMetadata method which looks like it does do some kind of merge.

To me, we need a new way of maintaining quick fixes and merging them into checks we pull in from 3rd parties.

rnveach avatar Sep 01 '21 02:09 rnveach

@romani @Calixte @lkoe Does eclipse-cs have support for 3rd party (like sevntu) quick fixes where the code is in the other library? Or is the only support what is coded directly into eclipse-cs? I am not seeing any quick fixes in sevntu but wanted to confirm if eclipse-cs had the support for 3rd party quick fixes.

rnveach avatar Sep 01 '21 03:09 rnveach

We first noticed this here: https://github.com/checkstyle/eclipse-cs/pull/286#discussion_r619450042 The metadata project intent was to have this ability to merge metadata from checkstyle with custom metadata (quickfixes) from the plugin, but as you noticed, it doesn't work right now.

Does eclipse-cs have support for 3rd party (like sevntu) quick fixes where the code is in the other library?

I don't know, though I see no reason why we wouldn't be able to support that.

Calixte avatar Sep 01 '21 13:09 Calixte

Thanks. I am assuming as long as the classpath is in the old XML file, and it can be instantiated, quickfixes can be access from a 3rd party library.

That makes restoration a bit trickier. We want to continue to support users adding new quickfixes, but it doesn't really make sense to be given a partial XML file in checkstyle's case with only the quickfix information. Even if we did this, we may run into ordering issues if we load quickfix only data before loading full checks from checkstyle.

It sounds like we need something new.

rnveach avatar Sep 01 '21 13:09 rnveach

Quick fix is possible only when you/plugin know how it works, it might for standard set of Checks but not third-party

, this is why we have big future project for Checkstyle to create auto fix possibilities for each Check.

romani avatar Sep 03 '21 17:09 romani

@romani We can discuss this more when checkstyle starts to work on that project, but it would be a good idea to keep this project in mind when checkstyle does do it. Eclipse users have an option to click on errors/warning and do a "Quickfix" to resolve the issue. It would be great if things can be made so that eclipse-cs can also hook into that functionality in checkstyle to provide it to eclipse-cs users too. Then we aren't duplicating things and becoming more uniformed.

rnveach avatar Sep 03 '21 17:09 rnveach

Yes, meanwhile, eclipsecs can have their own quick fix logic.

romani avatar Sep 04 '21 14:09 romani