Repository: uber/piranha (language: python)

Issue:
Fix disjunction_rule in Java and Kotlin
## Something **or** `true`

The query `simplify_something_or_true` for Java and Kotlin is currently wrong.

https://github.com/uber/piranha/blob/de2d604e54f7a361b584586b49337aa511409bbb/polyglot/piranha/src/cleanup_rules/java/rules.toml#L217-L227

`@lhs` **or** `true` should always be true.

Simplifying just for `@lhs` is behavior-changing. 
In the code below, consider if `something()` (`@lhs`) ends up returning `false`.

```java
if (something() || true) {
    System.out.println("always taken");
}
```

Simplifying to `something()` would lead to the branch **not** be taken.

## Something **and** `false`

If we want to be safe in such side-effects scenarios, we also need to consider `simplify_something_and_false`.

https://github.com/uber/piranha/blob/de2d604e54f7a361b584586b49337aa511409bbb/polyglot/piranha/src/cleanup_rules/java/rules.toml#L196-L208

Simplifying only to `false` will skip `something()` from executing.

```java
if (something() && false) {}
```

## Discussion
 
My main idea is to simplify as before (without considering side-effects) **only** when we are dealing with identifiers-like nodes (e.g., identifier, field access).

This leads to larger cleanups as more if conditions would eventually reach `if_cleanup`. A plus is also that our rewrites are safe since we're not discarding calls.

I can think of two downsides:
- Piranha Polyglot users may not care about side effects. That could be the case if the goal is to maximize cleanups.
- We need to write additional `if_cleanup` rules. In the code below, we also need to clean up the else branch.

```java
if (something() || true) {
    // keep
} else if (somethingElse) {
    // whole `else if` should be removed
}
else {
    // whole `else` should be removed
}
```
---

Another idea, in addition to the proposed solution above, would be to have kind of a global argument to ignore side effects. If this argument is `true`, we could use our straightforward rules, just as before. If `false`, we use the solution above.
The downsides are that we need to maintain: 
-  two different sets of rules;
- some logic to select between the rules depending on the `ignore-side-effects` argument value.



Discussion / hints:

Fix patch:
diff --git a/src/cleanup_rules/go/rules.toml b/src/cleanup_rules/go/rules.toml
index 1effa60e1..17fe8cc96 100644
--- a/src/cleanup_rules/go/rules.toml
+++ b/src/cleanup_rules/go/rules.toml
@@ -120,7 +120,7 @@ groups = ["boolean_expression_simplify"]
 is_seed_rule = false
 
 # Before :
-#  abc() && false
+#  abc && false
 # After :
 #  false
 #
@@ -131,9 +131,13 @@ query = """
     (binary_expression
         left : ([
                 (identifier)
+                (parenthesized_expression (identifier))
                 (true)
+                (parenthesized_expression (true))
                 (false)
+                (parenthesized_expression (false))
                 (selector_expression)
+                (parenthesized_expression (selector_expression))
             ]) @lhs
         operator : "&&"
         right: [(false) (parenthesized_expression (false))]
@@ -158,9 +162,13 @@ query = """
     (binary_expression
         left : ([
                 (identifier)
+                (parenthesized_expression (identifier))
                 (true)
+                (parenthesized_expression (true))
                 (false)
+                (parenthesized_expression (false))
                 (selector_expression)
+                (parenthesized_expression (selector_expression))
             ]) @lhs
         operator:"||"
         right: [(true) (parenthesized_expression (true))]


Test patch:
diff --git a/test-resources/go/feature_flag/builtin_rules/boolean_expression_simplify/expected/sample.go b/test-resources/go/feature_flag/builtin_rules/boolean_expression_simplify/expected/sample.go
index 062c6f2a1..827934c7f 100644
--- a/test-resources/go/feature_flag/builtin_rules/boolean_expression_simplify/expected/sample.go
+++ b/test-resources/go/feature_flag/builtin_rules/boolean_expression_simplify/expected/sample.go
@@ -25,8 +25,8 @@ func simplify_not() {
 }
 
 // simplify `!true` and `!false` and also:
-// true && something -> true
-// something && true -> true
+// true && something -> something
+// something && true -> something
 func simplify_true_and_something(something bool) {
     if something {
         fmt.Println("only something")
@@ -45,7 +45,7 @@ func simplify_true_and_something(something bool) {
 
 // simplify `!true` and `!false` and also:
 // false && something -> false
-// something && false -> true
+// something && false -> false
 func simplify_false_and_something(something bool) {
     fmt.Println("else 1")
     fmt.Println("else 2")
@@ -59,6 +59,16 @@ func simplify_false_and_something(something bool) {
     } else {
         fmt.Println("keep 2")
     }
+
+    // function call && false
+    if f1() && false {
+        fmt.Println("keep as it is")
+    }
+
+    // function call || true
+    if f1() || true {
+        fmt.Println("keep as it is")
+    }
 }
 
 // simplify `!true` and `!false` and also:
diff --git a/test-resources/go/feature_flag/builtin_rules/boolean_expression_simplify/input/sample.go b/test-resources/go/feature_flag/builtin_rules/boolean_expression_simplify/input/sample.go
index b453ca59b..e3c13eb28 100644
--- a/test-resources/go/feature_flag/builtin_rules/boolean_expression_simplify/input/sample.go
+++ b/test-resources/go/feature_flag/builtin_rules/boolean_expression_simplify/input/sample.go
@@ -42,8 +42,8 @@ func simplify_not() {
 }
 
 // simplify `!true` and `!false` and also:
-// true && something -> true
-// something && true -> true
+// true && something -> something
+// something && true -> something
 func simplify_true_and_something(something bool) {
     if exp.BoolValue("true") && something {
         fmt.Println("only something")
@@ -62,7 +62,7 @@ func simplify_true_and_something(something bool) {
 
 // simplify `!true` and `!false` and also:
 // false && something -> false
-// something && false -> true
+// something && false -> false
 func simplify_false_and_something(something bool) {
     if exp.BoolValue("false") && something {
         fmt.Println("only false")
@@ -97,6 +97,15 @@ func simplify_false_and_something(something bool) {
     } else {
         fmt.Println("keep 2")
     }
+    // function call && false
+    if f1() && exp.BoolValue("false") {
+        fmt.Println("keep as it is")
+    }
+
+    // function call || true
+    if f1() || exp.BoolValue("true") {
+        fmt.Println("keep as it is")
+    }
 }
 
 // simplify `!true` and `!false` and also: