Repository: hylo-lang/hylo (swift)

Issue:
Modules with non-valid C++ identifiers create faulty programs
The transpiler naively uses the name of the Val module in the generated C++ output but this name may not always be a valid C++ identifier. `CXXIdentifier` should be improved to take care of this.

Pull request:
Fix #433

Patch:
diff --git a/Sources/CodeGen/CXX/CXXCodeWriter.swift b/Sources/CodeGen/CXX/CXXCodeWriter.swift
index 5f87dc860..e126d7778 100644
--- a/Sources/CodeGen/CXX/CXXCodeWriter.swift
+++ b/Sources/CodeGen/CXX/CXXCodeWriter.swift
@@ -42,7 +42,7 @@ public struct CXXCodeWriter {
     target.writeNewline()
 
     // Create a namespace for the entire module.
-    target.write("namespace \(source.name)")
+    target.write("namespace \(cxx: source.name)")
     target.beginBrace()
     target.writeNewline()
 
@@ -81,7 +81,7 @@ public struct CXXCodeWriter {
     target.writeNewline()
 
     // Create a namespace for the entire module.
-    target.write("namespace \(source.name)")
+    target.write("namespace \(cxx: source.name)")
     target.beginBrace()
     target.writeNewline()
 
@@ -147,13 +147,13 @@ public struct CXXCodeWriter {
   private func writeSignature(function decl: CXXFunctionDecl, into target: inout CodeFormatter) {
     write(typeExpr: decl.output, into: &target)
     target.writeSpace()
-    write(identifier: decl.identifier, into: &target)
+    target.write(decl.identifier)
     target.write("(")
     for i in 0 ..< decl.parameters.count {
       if i != 0 { target.write(", ") }
       write(typeExpr: decl.parameters[i].type, into: &target)
       target.writeSpace()
-      write(identifier: decl.parameters[i].name, into: &target)
+      target.write(decl.parameters[i].name)
     }
     target.write(")")
   }
@@ -169,7 +169,7 @@ public struct CXXCodeWriter {
 
   private func writeSignature(type decl: CXXClassDecl, into target: inout CodeFormatter) {
     target.write("class ")
-    write(identifier: decl.name, into: &target)
+    target.write(decl.name.description)
   }
   private func writeDefinition(type decl: CXXClassDecl, into target: inout CodeFormatter) {
     writeSignature(type: decl, into: &target)
@@ -212,10 +212,10 @@ public struct CXXCodeWriter {
     // and the type of the attribute needs to be native.
     if dataMembers.count == 1 && dataMembers[0].type.isNative {
       // Write implicit conversion constructor
-      target.write("\(source.name.description)(")
+      target.write("\(source.name)(")
       write(typeExpr: dataMembers[0].type, into: &target)
       target.write(" v) : ")
-      write(identifier: dataMembers[0].name, into: &target)
+      target.write(dataMembers[0].name)
       target.writeLine("(v) {}")
     }
   }
@@ -223,7 +223,7 @@ public struct CXXCodeWriter {
   private func write(classAttribute decl: CXXClassAttribute, into target: inout CodeFormatter) {
     write(typeExpr: decl.type, into: &target)
     target.writeSpace()
-    write(identifier: decl.name, into: &target)
+    target.write(decl.name)
     if let value = decl.initializer {
       target.write(" = ")
       write(expr: value, into: &target)
@@ -234,7 +234,7 @@ public struct CXXCodeWriter {
   private func write(localVar decl: CXXLocalVarDecl, into target: inout CodeFormatter) {
     write(typeExpr: decl.type, into: &target)
     target.writeSpace()
-    write(identifier: decl.name, into: &target)
+    target.write(decl.name)
     if let value = decl.initializer {
       target.write(" = ")
       write(expr: value, into: &target)
@@ -330,7 +330,7 @@ public struct CXXCodeWriter {
     case CXXIntegerLiteralExpr.self:
       write(integerLiteralExpr: expr as! CXXIntegerLiteralExpr, into: &target)
     case CXXIdentifier.self:
-      write(identifier: expr as! CXXIdentifier, into: &target)
+      target.write(expr as! CXXIdentifier)
     case CXXReceiverExpr.self:
       write(receiverExpr: expr as! CXXReceiverExpr, into: &target)
     case CXXTypeExpr.self:
@@ -361,20 +361,21 @@ public struct CXXCodeWriter {
   ) {
     target.write(expr.value ? "true" : "false")
   }
+
   private func write(
     integerLiteralExpr expr: CXXIntegerLiteralExpr, into target: inout CodeFormatter
   ) {
     target.write(expr.value)
   }
-  private func write(identifier expr: CXXIdentifier, into target: inout CodeFormatter) {
-    target.write(expr.description)
-  }
+
   private func write(receiverExpr expr: CXXReceiverExpr, into target: inout CodeFormatter) {
     target.write("this")
   }
+
   private func write(typeExpr expr: CXXTypeExpr, into target: inout CodeFormatter) {
     target.write(expr.text)
   }
+
   private func write(infixExpr expr: CXXInfixExpr, into target: inout CodeFormatter) {
     // TODO: handle precedence and associativity; as of writing this comment, infix operators cannot be properly tested.
     write(expr: expr.lhs, into: &target)
diff --git a/Sources/CodeGen/CXX/CodeFormatter.swift b/Sources/CodeGen/CXX/CodeFormatter.swift
index 589472d53..f8baa6edc 100644
--- a/Sources/CodeGen/CXX/CodeFormatter.swift
+++ b/Sources/CodeGen/CXX/CodeFormatter.swift
@@ -23,6 +23,11 @@ struct CodeFormatter: TextOutputStream {
     code.write(text)
   }
 
+  /// Adds `n` to the output code.
+  mutating func write(_ n: CXXIdentifier) {
+    write(n.description)
+  }
+
   /// Adds new text to the output code, with a new line at the end
   mutating func writeLine(_ text: String) {
     writeIndentationAtLineStart()
diff --git a/Sources/CodeGen/CXX/Expr/CXXIdentifier.swift b/Sources/CodeGen/CXX/Expr/CXXIdentifier.swift
index 5deeb2a3e..70fc6b3f4 100644
--- a/Sources/CodeGen/CXX/Expr/CXXIdentifier.swift
+++ b/Sources/CodeGen/CXX/Expr/CXXIdentifier.swift
@@ -4,23 +4,38 @@ struct CXXIdentifier: CXXExpr {
   /// The value of the identifier.
   let description: String
 
-  init(_ identifier: String) {
-    description = CXXIdentifier.sanitize(identifier)
+  /// Creates an instance sanitizing `n` for use as a C++ identifier.
+  ///
+  /// - Requires: `n` is not empty.
+  init<S: StringProtocol>(_ n: S) {
+    description = CXXIdentifier.sanitize(n)
   }
 
-  var precedence: Int {
-    0
-  }
+  var precedence: Int { 0 }
+
   var isLeftToRight: Bool {
     true  // doesn't really matter
   }
 
-  /// Sanitizes `identifier` and returns a valid C++ identifier.
-  static func sanitize(_ identifier: String) -> String {
-    // Append an underscore to reserved identifiers.
-    reserved.contains(identifier)
-      ? identifier + "_"
-      : identifier
+  /// Returns a copy of `n` sanitized for use as a C++ identifier.
+  ///
+  /// - Requires: `n` is not empty.
+  static func sanitize<S: StringProtocol>(_ n: S) -> String {
+    let s = n.reduce(into: "") { (s, c) in
+      if c.isAllowedInCXXIdentitifer {
+        s.append(c)
+      } else {
+        s.append(c.utf16.reduce(into: "u") { (u, point) in u += String(point, radix: 16) })
+      }
+    }
+
+    if s.first!.isNumber {
+      return "a" + s
+    } else if reserved.contains(s) {
+      return "_" + s
+    } else {
+      return s
+    }
   }
 
   /// The set of reserved keywords in C++.
@@ -38,3 +53,29 @@ struct CXXIdentifier: CXXExpr {
   ])
 
 }
+
+extension CXXIdentifier: CustomStringConvertible {}
+
+extension String.StringInterpolation {
+
+  /// Appends a copy of `n` sanitized for use as a C++ identifier.
+  ///
+  /// - Requires: `n` is not empty.
+  public mutating func appendInterpolation<S: StringProtocol>(cxx n: S) {
+    appendLiteral(CXXIdentifier(n).description)
+  }
+
+}
+
+extension Character {
+
+  /// Indicates whether the character is allowed to appear in a C/C++ identifier.
+  fileprivate var isAllowedInCXXIdentitifer: Bool {
+    guard let code = asciiValue else { return false }
+    return (0x5f == code)  // underscore
+      || (0x61 ... 0x7a).contains(code)  // a ... z
+      || (0x41 ... 0x5a).contains(code)  // A ... Z
+      || (0x30 ... 0x39).contains(code)  // 0 ... 9
+  }
+
+}
diff --git a/Sources/Core/AST/DeclLocator.swift b/Sources/Core/AST/DeclLocator.swift
index 34fbd85f5..a0a996ee0 100644
--- a/Sources/Core/AST/DeclLocator.swift
+++ b/Sources/Core/AST/DeclLocator.swift
@@ -95,17 +95,17 @@ public struct DeclLocator: Hashable {
     public var mangled: String {
       switch self {
       case .conformance(let target, let trait):
-        return "C\(target)\(trait.mangled)"
+        return "C\(target)\(trait)"
 
       case .extension(let target):
         return "E\(target)"
 
       case .function(let name, let labels, let notation):
-        let labels = labels.map(\.mangled).joined()
+        let labels = labels.joined()
         if let n = notation {
-          return "O\(String(describing: n).mangled)\(name.mangled)\(labels.count)\(labels)a"
+          return "O\(n)\(name)\(labels.count)\(labels)a"
         } else {
-          return "F\(name.mangled)\(labels.count)\(labels)a"
+          return "F\(name)\(labels.count)\(labels)a"
         }
 
       case .methodImpl(let introducer):
@@ -118,20 +118,20 @@ public struct DeclLocator: Hashable {
         }
 
       case .module(let name):
-        return "M\(name.mangled)"
+        return "M\(name)"
 
       case .namespace(let name):
-        return "N\(name.mangled)"
+        return "N\(name)"
 
       case .lambda(let discriminator):
         return "L\(discriminator.rawValue)"
 
       case .product(let name):
-        return "P\(name.mangled)"
+        return "P\(name)"
 
       case .subscript(let name, let labels):
-        let ls = labels.map(\.mangled).joined()
-        return "S\(name.mangled)\(labels.count)\(ls)"
+        let ls = labels.joined()
+        return "S\(name)\(labels.count)\(ls)"
 
       case .subscriptImpl(let introducer):
         switch introducer {
@@ -143,7 +143,7 @@ public struct DeclLocator: Hashable {
         }
 
       case .trait(let name):
-        return "N\(name.mangled)"
+        return "N\(name)"
       }
     }
 
@@ -232,41 +232,3 @@ extension DeclLocator.Component: CustomStringConvertible {
   }
 
 }
-
-extension String {
-
-  fileprivate var mangled: String {
-    // Substitute non alphanumeric character.
-    var result = ""
-    result.reserveCapacity(count)
-
-    for character in self {
-      if character.isMangledAllowed {
-        result.append(character)
-      } else {
-        result.append(
-          character.utf16.reduce(
-            into: "u",
-            { (u, point) in
-              u += String(point, radix: 16)
-            }))
-      }
-    }
-
-    return String(describing: result.count) + result
-  }
-
-}
-
-extension Character {
-
-  /// Indicates whether the character is allowed to appear in a mangled identifier.
-  fileprivate var isMangledAllowed: Bool {
-    guard let code = asciiValue else { return false }
-    return (0x61 ... 0x7a).contains(code)  // a ... z
-      || (0x41 ... 0x5a).contains(code)  // A ... Z
-      || (0x30 ... 0x39).contains(code)  // 0 ... 9
-      || (0x5f == code)  // _
-  }
-
-}


Test patch:
diff --git a/Tests/ValTests/CXXIdentifierTests.swift b/Tests/ValTests/CXXIdentifierTests.swift
new file mode 100644
index 000000000..ca0c0f808
--- /dev/null
+++ b/Tests/ValTests/CXXIdentifierTests.swift
@@ -0,0 +1,20 @@
+import XCTest
+
+@testable import CodeGenCXX
+
+final class CXXIdentifierTests: XCTestCase {
+
+  func testIdentity() {
+    XCTAssertEqual(CXXIdentifier("foo").description, "foo")
+    XCTAssertEqual(CXXIdentifier("_bar").description, "_bar")
+  }
+
+  func testReservedKeyword() {
+    XCTAssertEqual(CXXIdentifier("template").description, "_template")
+  }
+
+  func testOperator() {
+    XCTAssertEqual(CXXIdentifier("++").description, "u2bu2b")
+  }
+
+}