From #65,

/Users/samhith/project/featuremanager/FeatureManager.h:101:29: error: cannot find
      interface declaration for 'NSObject', superclass of 'FeatureManager'
@interface FeatureManager : NSObject
~~~~~~~~~~~~~~~~~~~~~~~~~   
/Users/samhith/project/featuremanager/FeatureManager.h:103:3: error: expected a type
+(BOOL)isAppTypeNew;
  ^
/Users/samhith/project/featuremanager/FeatureManager.h:104:3: error: expected a type
+(BOOL)loadFile;

Is there anything I can do to overcome these errors? Let me know if you need more info on this. Thanks!

Originally posted by @Samhithgb in #65 (comment)

When header files are missing in the input files to PiranhaObjC, a fatal error is thrown as follows:

tests/OptimisticNamed.m:19:10: fatal error: 'TestHeader.h' file not found
#include "TestHeader.h"
         ^~~~~~~~~~~~~~

where OptimisticNamed.m is updated to include a non-existent TestHeader.h.

The correct fix will be to call SetSuppressIncludeNotFoundError from the Piranha implementation. (see #50 (comment))

Piranha simplification will need manual review and fixing of testcases, but I think it's useful to delete lines like:
expect(true).andReturn(true);
assertFalse(true);
which Piranha leaves behind in test classes after simplifying toggle expressions.

while working on #57 and #35 i noticed we do not clean up empty methods.

Ideally we should delete methods which result in no-code and also delete their references in parent methods

PiranhaJava directs through the steps for running piranha with flagName specified in the pom.xml but the Piranha paper states that there is a Piranha pipeline that runs automatically in some scheduled intervals to detect state flags. Is there a sample project to get started with the Piranha pipeline ? Also is it possible to trigger the pipeline manually ?

I am trying the sample java gradle project and after running clean build piranha only removes the SAMPLE_STALE_FLAG enum but not the code checks. This is the output -

public class MyClass {

  enum TestExperimentName {
    //REMOVED FROM HERE BUT NOT BELOW
  }

  private XPTest expt;

  public void foo() {
    if (expt.flagEnabled(TestExperimentName.SAMPLE_STALE_FLAG)) {
      System.out.println("Hello World");
    }

  }

  public void bar() {
    if (expt.flagDisabled(TestExperimentName.SAMPLE_STALE_FLAG)) {
      System.out.println("Hi World");
    }
  }

  static class XPTest {
    public boolean flagEnabled(TestExperimentName x) {
      return true;
    }

    public boolean enableFlag(TestExperimentName x) {
      return true;
    }

    public boolean disableFlag(TestExperimentName x) {
      return true;
    }

    public boolean flagDisabled(TestExperimentName x) {
      return true;
    }
  }
}

Here is my gradle file - https://github.com/vihanparmar/testRepo/blob/master/test/build.gradle

Please let me know what i am doing wrong or if this is an issue.

Currently PiranhaSwift APIs are specified using a properties file as shown here. It will be helpful to migrate this to use a json file so that more properties can be specified easily. For instance, it could be an array of methodProperties where the methodName, flagType, returnType and argumentIndex of the flag are specified. Example:

{ "methodProperties": [ { "methodName": "isToggleEnabled", "flagType": "treated", "returnType": "boolean", "argumentIndex": 0 }, ... ], }

This properties file will be parsed and the listed APIs will be used for refactoring.

This is a known issue in Piranha.

Piranha determines deletion of the specified flag variable and the methods that use it in a single pass. But by specifying returnType and receiverType in properties.json, it is possible to have certain usages deleted and others remain. Thus, deleting the variable would be a mistake because there is at least one undeleted method that still uses it.

This scenario would rarely occur in most codebases, since toggle methods have same/similar return and receiver types. But in order to be accurate, Piranha would have to delete all usages in the first pass based on user specified criteria and then delete the variable in the second pass if no usages remain.

For more context and details see: #39 (comment)

If and when this issue is fixed, return and receiver related tests in XPFlagCleanerTest would need to be modified to ensure that the STALE_FLAG variable is not being deleted.

Version 0.0.4 moves to a JSON format for the Piranha config.
When users upgrade, they would have their old piranha.properties files lying around.
Piranha should print a warning and point them to the documentation so they can move to a json config.

For context:
#39 (comment)

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!

Given input:

if(cond1 || (cond2 && experiments.isToggleEnabled(STALE_FLAG)) {
  // do something
}

the output of PiranhaJava is

if(cond1 || (cond2)) {
  // do something
}

The additional paranthesis around cond2 should be eliminated.

Hey 👋

Do you have any plans to support JavaScript/TypeScript?

Thanks 👍

how to run sample app in intelliJ locally?

This will help in understanding and contributing the code. Post this we can add setup and run guidelines to the README.

The old configuration format has been deprecated. It was intended that PiranhaJava crashed when seeing such configuration file as opposed to the new .json format. See https://github.com/uber/piranha/blob/master/java/piranha/src/main/java/com/uber/piranha/XPFlagCleaner.java#L262

This warning is currently not triggering when deploying Piranha 0.1.0 with the old configuration format, rather Piranha silently fails to pick up any flag API methods from the configuration.

Running Piranha with -XepOpt:Piranha:FlagName="" results in the deletion of all static imports.

Two places in XPFlagCleaner can cause this, because the code is .endsWith(xpFlagName) when comparing with expressions. Since xpFlagName is "", that always evaluates to true.

The specific places are:

and

The second case seems to be rarer.

The can be fixed with another check !xpFlagName.equals(EMPTY)

Piranha changed return(true); to returntrue;.
This is sort of related to #35 - in the sense that PiranhaJava doesn't validate the parens/spaces of the output generated.

This error probably would not have occured if the code had been Google Java formatted.

However, I thought I will bring it to your attention, since this is a case of Piranha introducing a "bug" instead of erring on the side of caution, like #35

Currently, the list of treatment groups are customized to a specific usage pattern. e.g., see pull request 12. It will be better to make this configurable

configMethodProperties in XPFlagCleaner is currently a Map<String, List<Map<String, Object>>>.
The Map<String, Object> should be replaced with a custom class, like PiranhaMethodRecord so it is decoupled from the config file and Piranha can be easily extended to other config specification formats.

For context and details see these comments:
#39 (comment)
#39 (comment)

Is it planned for Piranha to support refactoring/removing the declaration of the identifier argument?
e.g. removing all of the code in this block, instead of just the conditional block:

const featureFlag = "feature-flag"
if (isFlagTreated(featureFlag)) {
 // do stuff
}

If we had that functionality, we could likely make the assumption that the configured API argument index always refers to a string literal and expect the flag name to be a string literal node. Being able to search across multiple files/imports would likely be required for some of the more complex usages of feature flags

Originally posted by @atrakh in #91 (comment)

Tracker for enabling NullAway for Piranha.

was working on #35 but it seems there is an issue in the current master branch where expressions are not correctly handled in if statements

to reproduce simply change the implementation in sample app (MyClass.bak)

  public void bar(boolean base) {
    if (base && expt.flagDisabled(TestExperimentName.SAMPLE_STALE_FLAG)) {
      System.out.println("Hi World");
    }
  }

The output diff is,

  public void bar(boolean y) {

  }

SwiftSyntax is at version 5.2 (release tag is 0.50200.0). Piranha uses the 5.1 version and this may be causing issues to users who are on the latest versions of Xcode. See compatibility error here.

This issue is to upgrade PiranhaSwift to use the latest version.

Steps to repro:

./gradlew clean build
git status

Shows changes to sample/src/main/java/com/uber/mylib/MyClass.java those changes should be reverted after testing, or better yet, happen only on a temporary file/directory for testing.

There seems to be a few things missing in Objective C documentation, like :

1. How do we pass piranha.properties file or list of method names corresponding to feature flags (like in PiranhaJava)?
2. What does 'FlagType' mean in the argument list?
3. Is it possible to trigger piranha for the entire project at once, instead of passing each file?

Hi,

I'm defining an annotation :

@Retention(RetentionPolicy.RUNTIME)
@Target({ElementType.METHOD})
public @interface FlagTesting {
    MyClass.TestExperimentName[] treated();
}

and using it in my unit test :

@Test
@FlagTesting(treated = MyClass.TestExperimentName.SAMPLE_STALE_FLAG)
public void addition_isCorrect() throws Exception {
...
}

When I build this, although the flag and its corresponding methods in the product code get removed as expected, unit test build fails with the error :

Execution failed for task ':app:compileInternalChromeBeta64DebugUnitTestJavaWithJavac'.
> Piranha is not a valid checker name

Any pointers on why this might happen will be really great.

PS : Using this on Android code that uses gradle.

This is needed, e.g. to remove methods under emptyMethods=... for which the experiment symbol is not the first parameter.

Command executed:

node src/piranha.js -s flag.js -f my-flag -p config/properties.json --treated

Input file (flag.js)

export const myFlag = isFlagTreated('my-flag');
if (myFlag) {
  console.log('Enabled');
} else {
  console.log('Disabled');
}

Expected refactor:

console.log('Enabled');

Actual refactor:

export {};
console.log('Enabled');

Following up on issue 25, the following extensions were requested by @sivacoder:

But, we found a few extension cases we need where the current removal code is not supporting:

1. Ability to use string-literal
   getldvalue("dummy-flag")
2. Ability to use constants
   private static final String STALE_FLAG_NAME = "dummy-flag"; getldvalue(STALE_FLAG_NAME)
3. Control methods with more than 1 argument
   getldvalue(STALE_FLAG_NAME, paramter1, parameter 2)
4. Removing dangling boolean variable
   boolean isFlagEnabled = getldvalue(STALE_FLAG_NAME);
if (isFlagEnabled) {
System.out.println("success");
}

Piranha converts to
boolean isFlagEnabled = true;
if(isFlagEnabled) {
System.out.println("success");
}

@lazaroclapp - Happy to contribute if we can have a quick sync on how to go about it. Looking forward

Filing this issue to continue the discussions here.

Considering that ruby is not statically typed and has meta-programming where it's hard to find the references of a particular flag. Just curious about the possibilities.

This feature request is applicable to all implementations, but I'm submitting it for PiranhaJS as I plan to implement this myself.

PiranhaJS could be enhanced to support deep-cleaning flag imports from multiple files.

e.g. PiranhaJS should remove all of the following code given the input my-flag

flags.js

export const myFlag = isFlagTreated('my-flag')

app.js

import { myFlag as isFlagEnabled } from 'flags'
if (isFlagEnabled) {
  // do thing
}

In your experience, what is the popular usage style in JS? We can have PiranhaJS use that style (literal/identifier) as the default.

Internally we use string literal calls for JS, but I can see both methods being common. Ideally, every Piranha implementation would support both.

If necessary, that can be overridden by an option in properties.json?

My initial thought was to make this a command line argument, since usage can very on a per-flag or per-file basis (e.g. using an identifier to call a flag API in one file, and a literal in another). Perhaps we can search for both by default, and add a new argument to limit search to one node type?

Originally posted by @atrakh in #91 (comment)

When the input. code is

 public void foo (boolean x) {
    if (x) {
      System.out.println("if block");
    } else if (expt.flagEnabled(TestExperimentName.SAMPLE_STALE_FLAG)) {
      System.out.println("else if block");
    } else {
      System.out.println("else block");
    }
  }

and IsTreated=true

PiranhaJava emits following output,

public void foo (boolean x) {
    if (x){
      System.out.println("if block");
    } else {
      System.out.println("else if block");
    }
  }

Notice the removed space character in if (x){

I see a pending task of removing enum TestExperimentName from tests
This is also marked as todo in code as // Ideally we would remove this too, fix later
This acts as a boilerplate when adding new test cases and should be removed.

This can be fixed by simply creating a class similar to XPTest under test resources and creating handler function like assertFeatureFlags (String flags...)
This new function will create the output enum file with provided input flags.

Unsure if this is a trivial fix or not, but I was unable to get the sample application included in this repository to work with Java version 9.0 or higher. Downgrading to Java 8 resolved the issue for me

$ java -version
java version "9.0.4"
Java(TM) SE Runtime Environment (build 9.0.4+11)
Java HotSpot(TM) 64-Bit Server VM (build 9.0.4+11, mixed mode)

$ ./gradlew test
Starting a Gradle Daemon, 1 incompatible Daemon could not be reused, use --status for details

> Task :piranha:test FAILED
Error: Could not create the Java Virtual Machine.
-Xbootclasspath/p is no longer a supported option.

Currently PiranhaObjC APIs are hardcoded. See here.
Introduce a configuration json file that contains an array of methodProperties with methodName, flagType, returnType and argumentIndex. See below.

{ "methodProperties": [ { "methodName": "isToggleEnabled", "flagType": "treated", "returnType": "boolean", "argumentIndex": 0 }, ... ], }

This properties file will be parsed and the listed APIs will be used for refactoring.

Hi, in our (Airbnb) codebase we have slightly duplicative but separate experiment and feature flag ("trebuchet") systems. Additionally, we often combine assignments from both of these systems to produce a combined flag ("feature") which we consume in our product code. We're going to consolidate these systems in the future and are interested in using Piranha to with both our current and future experiment infrastructure. Would you be able to advise on the potential cost/benefit ratio we'd be able to see ("benefit": the levels of nesting ("deep cleaning?") that we'd be able to remove in our diffs)?

Here is an example diagram of our experimentation infrastructure https://app.lucidchart.com/publicSegments/view/cc1c7c43-fc9b-4c6e-bf16-86babff5cb65/image.png

1 serialize-javascript vulnerability found in javascript/package-lock.json 4 minutes ago

Remediation

Upgrade serialize-javascript to version 3.1.0 or later. For example:

"dependencies": {
  "serialize-javascript": ">=3.1.0"
}
or…
"devDependencies": {
  "serialize-javascript": ">=3.1.0"
}

    if x, cachedExperiments.isTreated(forExperiment: ExperimentNamesSwift.test_experiment) {
           // do something
    }

is refactored into

    if x, {
       // do something
    }

when isTreated returns true. The additional comma should be removed.

Amazing tool !
Is the kotlin support is planned ?

In XPFlagCleaner, mutable structures are currently used.

I'd prefer ImmutableMap and ImmutableList here and builders. But since the original code was also using mutable collections, I am absolutely fine leaving that for a follow up issue you two can assign to me 😅

Originally posted by @lazaroclapp in #39

#81 enhanced Piranha for JavaScript as suggested in #24. This issue is to provide support for TypeScript by add TS specific tests and enhancing the implementation.

Are there any plans for support in .NET?

I love this idea. Have anyone looked in to a integration with Unleash?

When Piranha is run, output the summary of the run that includes the configuration under which it ran, the number of APIs considered for refactoring, the number of APIs refactored, etc.
Related - issue #207

Can we start supporting golang?

In XPFlagCleaner, the public Description matchIf(IfTree ifTree, VisitorState visitorState) method uses the equality operator == for String comparison instead of .equals

if (update) { if (replacementPrefix != EMPTY) { replacementString = replacementPrefix + replacementString; }

Map<String, List<Map<String, Object>>>

to something like:

MultiMap<String, PiranhaMethod>

See #1 comment on the PiranhaUtils class.

See #2 and this comment thread.

What is the best way for external users to integrate Piranha? EP_PATCH_CHECK followed by file clean up script like we do? Something else?

See a well explained Gradle integration example. Would be good

1. To confirm if the tool would work with maven
2. If yes, a sample project would help.

BTW, tried on my maven project and struck with below error. Any help would be appreciated.

[ERROR] Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:3.8.0:compile (default-compile) on project myapp: Compilation failure [ERROR] javac: invalid flag: -Xep:PatchChecks:Piranha [ERROR] Usage: javac <options> <source files> [ERROR] use --help for a list of possible options