[ruby] Rework Conditional Assignments (#5226)

The more semantically sound version of the current lowering checks for all "falsey" states of a variable instead of only the nil condition. Thus, the lowering is changed as follows: `aaa ||= bbb` becomes `aaa = bbb if !aaa` `aaa &&= bbb` becomes aaa = bbb if aaa`
joernio · Jan 14, 2025 · ef1f547 · ef1f547
1 parent 03980ff
commit ef1f547
Show file tree

Hide file tree

Showing 2 changed files with 19 additions and 39 deletions.
diff --git a/...ntends/rubysrc2cpg/src/main/scala/io/joern/rubysrc2cpg/astcreation/AstCreatorHelper.scala b/...ntends/rubysrc2cpg/src/main/scala/io/joern/rubysrc2cpg/astcreation/AstCreatorHelper.scala
@@ -8,6 +8,8 @@ import io.joern.rubysrc2cpg.astcreation.RubyIntermediateAst.{
   MemberAccess,
   RubyExpression,
   RubyFieldIdentifier,
+  RubyIdentifier,
+  SimpleIdentifier,
   SingleAssignment,
   StatementList,
   TextSpan,
@@ -160,41 +162,19 @@ trait AstCreatorHelper(implicit withSchemaValidation: ValidationMode) { this: As
     member
   }
 
-  /** Lowers the `||=` and `&&=` assignment operators to the respective `.nil?` checks
+  /** Lowers the `||=` and `&&=` assignment operators to the respective conditional checks, e.g, `aaa ||= bbb` becomes
+    * `aaa = bbb if !aaa` `aaa &&= bbb` becomes aaa = bbb if aaa`
     */
   def lowerAssignmentOperator(lhs: RubyExpression, rhs: RubyExpression, op: String, span: TextSpan): RubyExpression &
     ControlFlowStatement = {
-    val condition  = nilCheckCondition(lhs, op, "nil?", span)
-    val thenClause = nilCheckThenClause(lhs, rhs, span)
-    nilCheckIfStatement(condition, thenClause, span)
-  }
-
-  /** Generates the required `.nil?` check condition used in the lowering of `||=` and `&&=`
-    */
-  private def nilCheckCondition(lhs: RubyExpression, op: String, memberName: String, span: TextSpan): RubyExpression = {
-    val memberAccess =
-      MemberAccess(lhs, op = ".", memberName = "nil?")(span.spanStart(s"${lhs.span.text}.nil?"))
-    if op == "||=" then memberAccess
-    else UnaryExpression(op = "!", expression = memberAccess)(span.spanStart(s"!${memberAccess.span.text}"))
-  }
-
-  /** Generates the assignment and the `thenClause` used in the lowering of `||=` and `&&=`
-    */
-  private def nilCheckThenClause(lhs: RubyExpression, rhs: RubyExpression, span: TextSpan): RubyExpression = {
-    StatementList(List(SingleAssignment(lhs, "=", rhs)(span.spanStart(s"${lhs.span.text} = ${rhs.span.text}"))))(
-      span.spanStart(s"${lhs.span.text} = ${rhs.span.text}")
-    )
-  }
-
-  /** Generates the if statement for the lowering of `||=` and `&&=`
-    */
-  private def nilCheckIfStatement(
-    condition: RubyExpression,
-    thenClause: RubyExpression,
-    span: TextSpan
-  ): RubyExpression & ControlFlowStatement = {
+    val condition =
+      if op == "||=" then UnaryExpression(op = "!", expression = lhs)(span.spanStart(s"!${lhs.span.text}"))
+      else lhs
+    val thenClause = StatementList(
+      List(SingleAssignment(lhs, "=", rhs)(span.spanStart(s"${lhs.span.text} = ${rhs.span.text}")))
+    )(span.spanStart(s"${lhs.span.text} = ${rhs.span.text}"))
     IfExpression(condition = condition, thenClause = thenClause, elsifClauses = List.empty, elseClause = None)(
-      span.spanStart(s"if ${condition.span.text} then ${thenClause.span.text} end")
+      span.spanStart(s"${thenClause.span.text} if ${condition.span.text}")
     )
   }
 

diff --git a/...ends/rubysrc2cpg/src/test/scala/io/joern/rubysrc2cpg/querying/SingleAssignmentTests.scala b/...ends/rubysrc2cpg/src/test/scala/io/joern/rubysrc2cpg/querying/SingleAssignmentTests.scala
@@ -45,15 +45,15 @@ class SingleAssignmentTests extends RubyCode2CpgFixture {
 
   "`||=` is represented by a lowered if call to .nil?" in {
     val cpg = code("""
-        |def foo
+        |def foo(x)
         |  x ||= false
         |end
         |""".stripMargin)
 
     inside(cpg.method.name("foo").controlStructure.l) {
       case ifStruct :: Nil =>
         ifStruct.controlStructureType shouldBe ControlStructureTypes.IF
-        ifStruct.condition.code.l shouldBe List("x.nil?")
+        ifStruct.condition.code.l shouldBe List("!x")
 
         inside(ifStruct.whenTrue.ast.isCall.name(Operators.assignment).l) {
           case assignmentCall :: Nil =>
@@ -70,15 +70,15 @@ class SingleAssignmentTests extends RubyCode2CpgFixture {
 
   "`&&=` is represented by lowered if call to .nil?" in {
     val cpg = code("""
-        |def foo
+        |def foo(x)
         |  x &&= true
         |end
         |""".stripMargin)
 
     inside(cpg.method.name("foo").controlStructure.l) {
       case ifStruct :: Nil =>
         ifStruct.controlStructureType shouldBe ControlStructureTypes.IF
-        ifStruct.condition.code.l shouldBe List("!x.nil?")
+        ifStruct.condition.code.l shouldBe List("x")
 
         inside(ifStruct.whenTrue.ast.isCall.name(Operators.assignment).l) {
           case assignmentCall :: Nil =>
@@ -319,7 +319,7 @@ class SingleAssignmentTests extends RubyCode2CpgFixture {
     inside(cpg.method.name("foo").controlStructure.l) {
       case ifStruct :: Nil =>
         ifStruct.controlStructureType shouldBe ControlStructureTypes.IF
-        ifStruct.condition.code.l shouldBe List("(<tmp-0> = hash[:id]).nil?")
+        ifStruct.condition.code.l shouldBe List("!hash[:id]")
 
         inside(ifStruct.whenTrue.ast.isCall.name(Operators.assignment).l) {
           case assignmentCall :: Nil =>
@@ -363,7 +363,7 @@ class SingleAssignmentTests extends RubyCode2CpgFixture {
     inside(cpg.method.name("foo").controlStructure.l) {
       case ifStruct :: Nil =>
         ifStruct.controlStructureType shouldBe ControlStructureTypes.IF
-        ifStruct.condition.code.l shouldBe List("!hash[:id].nil?")
+        ifStruct.condition.code.l shouldBe List("hash[:id]")
 
         inside(ifStruct.whenTrue.ast.isCall.name(Operators.assignment).l) {
           case assignmentCall :: Nil =>
@@ -408,7 +408,7 @@ class SingleAssignmentTests extends RubyCode2CpgFixture {
     inside(cpg.method.name("foo").controlStructure.l) {
       case ifStruct :: Nil =>
         ifStruct.controlStructureType shouldBe ControlStructureTypes.IF
-        ifStruct.condition.code.l shouldBe List("(<tmp-0> = A.B).nil?")
+        ifStruct.condition.code.l shouldBe List("!A.B")
 
         inside(ifStruct.whenTrue.ast.isCall.name(Operators.assignment).l) {
           case assignmentCall :: Nil =>
@@ -436,7 +436,7 @@ class SingleAssignmentTests extends RubyCode2CpgFixture {
     inside(cpg.method.name("foo").controlStructure.l) {
       case ifStruct :: Nil =>
         ifStruct.controlStructureType shouldBe ControlStructureTypes.IF
-        ifStruct.condition.code.l shouldBe List("!A.B.nil?")
+        ifStruct.condition.code.l shouldBe List("A.B")
 
         inside(ifStruct.whenTrue.ast.isCall.name(Operators.assignment).l) {
           case assignmentCall :: Nil =>