PiranhaJava - remove flag methods that have no arguments (that don't use enum)

[pranavsb]

My codebase defines toggles like this:

public class MockToggle {
    public static boolean isToggleEnabled() {
        return true;
    }
}

And they are used like this:

public class ApplicationCode {
   if (MockToggle.isToggleEnabled()) {
       // do something
   } else {
       // do something else 
   }
}

What's the easiest way to make Piranha work? Should I modify Piranha to consider empty method calls that match the method specified in the properties file? (is it just a matter of changing an if condition somewhere?)

I'd like to contribute a PR if I get this working!

[mkr-plse]

In the properties file, if you include your toggle method in the treatedMethods, then depending on the input to Piranha (treated/control), the then or else branch will be kept.

[pranavsb]

Hi!
I put all possibilities in piranha.properties
treatedMethods=isToggleEnabled,MockToggle.isToggleEnabled,MockToggle

but it doesn't work.

But I know piranha is configured correctly because dummy branches if (true) are getting removed.

Update:
Here are my piranha args, in case it helps
<arg>-Xplugin:ErrorProne -Xep:Piranha:WARN -XepPatchChecks:Piranha -XepPatchLocation:IN_PLACE -XepOpt:Piranha:FlagName=PIRANHA_PLACEHOLDER -XepOpt:Piranha:IsTreated=true -XepOpt:Piranha:Config=piranha/config/piranha.properties</arg>

[mkr-plse]

Ah, I see the problem. There is no flagname for these APIs. You will need to modify the code for the no flag name case.

A broader fix for this is discussed here and here. A PR contribution for this will be appreciated.

[pranavsb]

I got it working by adding a mit.getArguments().size() == 0 check to getXPAPI, will raise a PR soon.

I'm wondering if it makes sense to extend this so that it checks the invocation to make sure its the correct toggle class and not just match by method name.

Something like this:
piranha.properties
treatedMethods=MockToggle1.isEnabled

Application.java

if (MockToggle1.isEnabled() {
    // this if should be removed
}
...
if (MockToggle2.isEnabled()) {
   // this one should not be removed
}

[mkr-plse]

Instead of an interim fix, I suggest creating a json properties file with the following attributes:

1. method name
2. argument index (0, 1, .. or don't care)
3. return type (boolean, void, ..)
4. receiver type (MockToggle1, MockToggle2, ...)
5. flag type (treated, control)

Processing these attributes as part of updateConfig and checking for it in getXPAPI will be ideal. We can then get rid of the piranha.properties file.

[pranavsb]

Ah, I see what you mean. I'll try that.

[pranavsb]

This is the properties.json structure I'm planning, let me know if I've missed something.

{
  "linkURL": "<provide_your_url>",
  "annotations": "",
  "piranhaMethodProperties":
    [
      {
        "methodName": "isToggleEnabled",
        "argumentIndex": null, //0,1, -1 for dont care
        "returnType": null, // boolean void etc
        "receiverType": null, // how to determine? null for dont care?
        "flagType": null // treated, control, empty, what abt treatmentGroup?
      },
      {
        "methodName": "isFlagDisabled",
        "argumentIndex": null,
        "returnType": null,
        "receiverType": null,
        "flagType": null
      },
      {
        "methodName": null,
        "argumentIndex": null,
        "returnType": null,
        "receiverType": null,
        "flagType": null
      }
    ]
}

[lazaroclapp]

Closed by #39 ! (thanks again, @pranavsb )