Add: string concatenation

[abh80]

Based on the discussions in #954
Added:

String concatenation

• Adds Binop.Add(...) support to String type.

constant c = "a" + "b" + "c" // gives "abc"
const a = "hello " + c  + "!" // gives "hello abc!"

• Concatenation Tests:

  • lib/src/test/scala/semantics/ValueSpec.scala -> add string + string add cases
  • [New]: tools/fpp-check/test/expr/string_concat_ok.fpp -> add string concatenation ok tests
  • [New]: tools/fpp-check/test/expr/string_concat_ok.ref.txt -> corresponding empty file
  • [???]: lib/src/test/input/syntax/lexer/ok/literals.fpp -> Add a string + string example; however, the lexer will accept it regardless. I think we can skip this.
  • tools/fpp-check/test/expr/tests.sh -> add test files definition for both concatenation and interpolation.
  • Cover edge error cases in fpp-check.

• Spec Tasks

  • Add section 10.11: String Concatenation Expressions (syntax, semantics, and examples)
  • Update section 10.13: Precedence and Associativity: remember that + means string concatenation
  • Update Arithmetic Expressions: add a cross-reference to String Concatenation for the + sign on strings
  • Update Type-Checking: Before doing a numeric common-type computation, add a string type rule for the + operator.
  • Update Values: keep in mind that string concatenation expressions can make string values.
  • Update defs.sh to include String-Concatenation-Expression.adoc
  • User guide: Add a new section 3.3.9. String Concatenation.

• Revised Tasks:

  • Add a new function compiler/lib/src/main/scala/analysis/CheckSemantics/CheckExprTypes.scala convertToNumericOrString. This will fix misleading error messages when adding to strings.
  • Implement and test new semantics: implemented in lib/src/main/scala/analysis/Semantics/Value.scala and tested in lib/src/test/scala/semantics/ValueSpec.scala

[abh80]

@bocchino Can you review the plan and share feedback, please? Also do we need to add fpp-to-cpp tests?

[bocchino]

Also do we need to add fpp-to-cpp tests?

I think we should extend the definition of value addition here: https://github.com/nasa/fpp/blob/main/compiler/lib/src/main/scala/analysis/Semantics/Value.scala. And test it here: https://github.com/nasa/fpp/blob/main/compiler/lib/src/test/scala/semantics/ValueSpec.scala. I think that may be enough functional testing on the FPP side. We could also add a functional test to FppTest in nasa/fprime.

[bocchino]

Can you review the plan and share feedback, please?

The plan looks good so far, but it is not complete. The plan should include the following high-level steps (from the new features wiki):

1. Revise the spec.
2. Implement and test new syntax.
3. Update analysis on the wiki if needed (it may not need any update, but we should check).
4. Implement and test new semantics.
5. Update the User's Guide.
6. Add functional testing to FppTest in nasa/fprime (this is a separate PR).

[abh80]

I understand, thanks for your review; Work to do 🚀

[abh80]

@bocchino
I think the implementation and testing part is done.

1. Revise the spec.

I have identified the following changes in spec:

• Add a section 10.13: String Concatenation Expressions to spec
• In the current section 10.12 Precedence and Associativity: update the precedence table to include + on String

2. Implement and test new syntax.

Not necessary here since syntax was not modified.

3. Update analysis on the wiki if needed (it may not need any update, but we should check).

I reviewed the analysis on the wiki and discovered no need to update it.

4. Implement and test new semantics.

Implemented at compiler/lib/src/main/scala/analysis/Semantics/Value.scala and tested at compiler/lib/src/test/scala/semantics/ValueSpec.scala

5. Update the User's Guide.

I'm a little confused about this one; one solution is to update and extend section 3.3.2 to provide a guide to the updated semantics. Another option is to create a new section 3.3.3 -> String Concatenation, which would necessitate changes to the subsequent sections. I do not believe this could be implemented in Section 3.3.8 Value Arithmetic Expressions.

[abh80]

We may need a new function convertToNumericOrString here. Otherwise the error message could be misleading. For example, if you try to add two things that are not numbers or strings, it will suggest that you can only add numbers.

Updated with clean function definition

@@ -159,7 +159,10 @@ object CheckExprTypes extends UseAnalyzer {
     for {
       a <- super.exprBinopNode(a, node, e)
       t <- a.commonType(e.e1.id, e.e2.id, loc)
-      _ <- convertToNumeric(loc, t)
+      _ <- e.op match {
+        case Ast.Binop.Add => convertToNumericOrString(loc, t)
+        case _ => convertToNumeric(loc, t)
+      }
     } yield a.assignType(node -> t)
   }

@@ -469,5 +472,13 @@ object CheckExprTypes extends UseAnalyzer {
       Left(error)
     }
   }
-
+
+  private def convertToNumericOrString(loc: Location, t: Type): Result.Result[Type] = {
+    t match {
+      case _: Type.String => Right(t)
+      case _ if t.isNumeric => Right(t)
+      case _ if t.isConvertibleTo(Type.Integer) => Right(Type.Integer)
+      case _ => Left(SemanticError.InvalidType(loc, s"cannot convert $t to a numeric or string type"))
+    }
+  }
 }

[abh80]

Forced push since had to rebase to keep latest docs

[bocchino]

Forced push since had to rebase to keep latest docs

To resolve docs conflicts, generally you should regenerate the docs. That way the updated docs will include your changes and the upstream changes.

[bocchino]

I think the implementation and testing part is done.

It looks like we may still need fpp-check tests to cover the error case(s).

[bocchino]

I'm a little confused about this one; one solution is to update and extend section 3.3.2 to provide a guide to the updated semantics. Another option is to create a new section 3.3.3 -> String Concatenation, which would necessitate changes to the subsequent sections. I do not believe this could be implemented in Section 3.3.8 Value Arithmetic Expressions.

I agree this isn't value arithmetic. Can we add a new section 3.3.9. String Concatenation? It looks a lot like value arithmetic, so it makes sense to have that section follow section 3.3.8. I think it shouldn't go 3.3.2, because that section just covers literal string values.

If you add the new section and regenerate the docs, then the current 3.3.9 will automatically be renumbered to 3.3.10.

[abh80]

Hey @bocchino, I have implemented the changes you asked for. Could you please review them?

[abh80]

I have noticed that in compiler/lib/src/main/scala/analysis/Semantics/Value.scala:
The + operator defined:

final def +(v: Value) = {
       def intOp(v1: BigInt, v2: BigInt) = v1 + v2
       def doubleOp(v1: Double, v2: Double) = v1 + v2
       binop(Value.Binop(intOp, doubleOp))(v)
}

And Value.String.binop is:

override def binop(op: Binop)(v: Value) = v match {
       case String(value2) => Some(String(value + value2))
       case _ => None
}

This would mean String.binop would work for any operator like +, -, / and *.
But this is enforced in:

• The type checker (convertToNumeric) blocks -, *, / on strings before evaluation ever runs
• Only Add uses convertToNumericOrString, so only + allows strings through

[bocchino]

In the spec, this material needs section needs to be merged into the section on Arithmetic Expressions. I think we should rename that section Binary Expressions (that's what they are called in the implementation). We need to have one syntax for binary expressions. We can't have two expressions sections with overlapping syntax.

[abh80]

Agreed, I will update it meanwhile

[bocchino]

This would mean String.binop would work for any operator

For consistency with what is already there, I think we should do the following:

• Add a member stringOp to Value.Binop.
• Implement stringOp for +.
• For the other stringOp cases, throw an InternalError (those functions should never be called).
• Implement binop for strings analogously to the way it is currently implemented for numbers.

[abh80]

I have removed the string concatenation expressions section and merged it with arithmetic sections under new binary expressions section

Before:

After:

[abh80]

I have implemented the changes you asked for, can you give the review @bocchino

[bocchino]

This is looking great! I made a few changes.

[abh80]

Thank you for letting me work on this feature, @bocchino. Would you like me to also work on the interpolation section? Of course, we must first discuss the overall implementation plan.

[abh80]

@bocchino I have implemented the functional testing for this feature in FppTest, kindly check and let me know if that's okay.
abh80/fprime@ff9cf21

For now I will open a draft pull request in nasa/fprime