From ef1f54708656139579a513f749397485a9391b71 Mon Sep 17 00:00:00 2001 From: David Baker Effendi Date: Tue, 14 Jan 2025 12:21:31 +0200 Subject: [PATCH] [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` --- .../astcreation/AstCreatorHelper.scala | 42 +++++-------------- .../querying/SingleAssignmentTests.scala | 16 +++---- 2 files changed, 19 insertions(+), 39 deletions(-) diff --git a/joern-cli/frontends/rubysrc2cpg/src/main/scala/io/joern/rubysrc2cpg/astcreation/AstCreatorHelper.scala b/joern-cli/frontends/rubysrc2cpg/src/main/scala/io/joern/rubysrc2cpg/astcreation/AstCreatorHelper.scala index 405935b15b6a..0ec3f2e629a3 100644 --- a/joern-cli/frontends/rubysrc2cpg/src/main/scala/io/joern/rubysrc2cpg/astcreation/AstCreatorHelper.scala +++ b/joern-cli/frontends/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/joern-cli/frontends/rubysrc2cpg/src/test/scala/io/joern/rubysrc2cpg/querying/SingleAssignmentTests.scala b/joern-cli/frontends/rubysrc2cpg/src/test/scala/io/joern/rubysrc2cpg/querying/SingleAssignmentTests.scala index 1e73d0c983f5..7a40275daeb9 100644 --- a/joern-cli/frontends/rubysrc2cpg/src/test/scala/io/joern/rubysrc2cpg/querying/SingleAssignmentTests.scala +++ b/joern-cli/frontends/rubysrc2cpg/src/test/scala/io/joern/rubysrc2cpg/querying/SingleAssignmentTests.scala @@ -45,7 +45,7 @@ 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) @@ -53,7 +53,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("x.nil?") + ifStruct.condition.code.l shouldBe List("!x") inside(ifStruct.whenTrue.ast.isCall.name(Operators.assignment).l) { case assignmentCall :: Nil => @@ -70,7 +70,7 @@ 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) @@ -78,7 +78,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("!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("( = 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("( = 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 =>