[GR-20471] Split Rope#withEncoding() in withEncoding7bit() and withBinaryEncoding()

* This makes it easier to reason about them.
* Also fixes the bug for the character length with ConcatRope#withEncoding(BINARY, CR_VALID).
* Reduce visibility of these methods since they should only be used by WithEncodingNode.

Author: eregon

CHANGELOG.md

@@ -69,6 +69,7 @@ Bug fixes:
 * Fixed `ENV.replace` implementation.
 * Fixed `ENV.udpate` implementation.
 * Fixed argument handling in `Kernel.printf`.
+* Fixed character length after conversion to binary from a non-US-ASCII String.
 
 Compatibility:
 

src/main/java/org/truffleruby/core/rope/AsciiOnlyLeafRope.java

@@ -23,12 +23,13 @@ public AsciiOnlyLeafRope(byte[] bytes, Encoding encoding) {
     }
 
     @Override
-    public Rope withEncoding(Encoding newEncoding, CodeRange newCodeRange) {
-        if (newCodeRange != getCodeRange()) {
-            CompilerDirectives.transferToInterpreterAndInvalidate();
-            throw new UnsupportedOperationException("Cannot fast-path updating encoding with different code range.");
-        }
-
+    Rope withEncoding7bit(Encoding newEncoding) {
         return new AsciiOnlyLeafRope(getRawBytes(), newEncoding);
     }
+
+    @Override
+    Rope withBinaryEncoding() {
+        CompilerDirectives.transferToInterpreterAndInvalidate();
+        throw new UnsupportedOperationException("Must only be called for CR_VALID Strings");
+    }
 }

src/main/java/org/truffleruby/core/rope/ConcatRope.java

@@ -11,8 +11,8 @@
 
 import org.jcodings.Encoding;
 
-import com.oracle.truffle.api.CompilerDirectives;
 import com.oracle.truffle.api.CompilerDirectives.TruffleBoundary;
+import org.jcodings.specific.ASCIIEncoding;
 
 public class ConcatRope extends ManagedRope {
 
@@ -27,22 +27,23 @@ public ConcatRope(
             CodeRange codeRange,
             int depth,
             boolean balanced) {
-        this(left, right, encoding, codeRange, depth, null, balanced);
+        this(left, right, encoding, codeRange, left.characterLength() + right.characterLength(), depth, null, balanced);
     }
 
     private ConcatRope(
             ManagedRope left,
             ManagedRope right,
             Encoding encoding,
             CodeRange codeRange,
+            int characterLength,
             int depth,
             byte[] bytes,
             boolean balanced) {
         super(
                 encoding,
                 codeRange,
                 left.byteLength() + right.byteLength(),
-                left.characterLength() + right.characterLength(),
+                characterLength,
                 depth,
                 bytes);
         this.left = left;
@@ -51,13 +52,31 @@ private ConcatRope(
     }
 
     @Override
-    public Rope withEncoding(Encoding newEncoding, CodeRange newCodeRange) {
-        if (newCodeRange != getCodeRange()) {
-            CompilerDirectives.transferToInterpreterAndInvalidate();
-            throw new UnsupportedOperationException("Cannot fast-path updating encoding with different code range.");
-        }
+    Rope withEncoding7bit(Encoding newEncoding) {
+        assert getCodeRange() == CodeRange.CR_7BIT;
+        return new ConcatRope(
+                getLeft(),
+                getRight(),
+                newEncoding,
+                CodeRange.CR_7BIT,
+                characterLength(),
+                depth(),
+                getRawBytes(),
+                balanced);
+    }
 
-        return new ConcatRope(getLeft(), getRight(), newEncoding, newCodeRange, depth(), getRawBytes(), balanced);
+    @Override
+    Rope withBinaryEncoding() {
+        assert getCodeRange() == CodeRange.CR_VALID;
+        return new ConcatRope(
+                getLeft(),
+                getRight(),
+                ASCIIEncoding.INSTANCE,
+                CodeRange.CR_VALID,
+                byteLength(),
+                depth(),
+                getRawBytes(),
+                balanced);
     }
 
     @Override

src/main/java/org/truffleruby/core/rope/InvalidLeafRope.java

@@ -23,13 +23,14 @@ public InvalidLeafRope(byte[] bytes, Encoding encoding, int characterLength) {
     }
 
     @Override
-    public Rope withEncoding(Encoding newEncoding, CodeRange newCodeRange) {
-        if (newCodeRange != getCodeRange()) {
-            CompilerDirectives.transferToInterpreterAndInvalidate();
-            throw new UnsupportedOperationException("Cannot fast-path updating encoding with different code range.");
-        }
-
-        final int newCharacterLength = RopeOperations.strLength(newEncoding, getRawBytes(), 0, byteLength());
-        return new InvalidLeafRope(getRawBytes(), newEncoding, newCharacterLength);
+    Rope withEncoding7bit(Encoding newEncoding) {
+        CompilerDirectives.transferToInterpreterAndInvalidate();
+        throw new UnsupportedOperationException("Must only be called for ASCII-only Strings");
+    }
+
+    @Override
+    Rope withBinaryEncoding() {
+        CompilerDirectives.transferToInterpreterAndInvalidate();
+        throw new UnsupportedOperationException("Must only be called for CR_VALID Strings");
     }
 }

src/main/java/org/truffleruby/core/rope/LazyIntRope.java

@@ -55,15 +55,17 @@ private static int length(int value) {
     }
 
     @Override
-    public Rope withEncoding(Encoding newEncoding, CodeRange newCodeRange) {
-        if (newCodeRange != getCodeRange()) {
-            CompilerDirectives.transferToInterpreterAndInvalidate();
-            throw new UnsupportedOperationException("Cannot fast-path updating encoding with different code range.");
-        }
-
+    Rope withEncoding7bit(Encoding newEncoding) {
+        assert getCodeRange() == CodeRange.CR_7BIT;
         return new LazyIntRope(value, newEncoding, length(value));
     }
 
+    @Override
+    Rope withBinaryEncoding() {
+        CompilerDirectives.transferToInterpreterAndInvalidate();
+        throw new UnsupportedOperationException("Must only be called for CR_VALID Strings");
+    }
+
     @Override
     public byte[] fulfill() {
         if (bytes == null) {

src/main/java/org/truffleruby/core/rope/NativeRope.java

@@ -189,11 +189,16 @@ public String toString() {
     }
 
     @Override
-    public Rope withEncoding(Encoding newEncoding, CodeRange newCodeRange) {
+    Rope withEncoding7bit(Encoding newEncoding) {
         return withEncoding(newEncoding);
     }
 
-    public NativeRope withEncoding(Encoding newEncoding) {
+    @Override
+    Rope withBinaryEncoding() {
+        return withEncoding(ASCIIEncoding.INSTANCE);
+    }
+
+    NativeRope withEncoding(Encoding newEncoding) {
         return new NativeRope(pointer, byteLength(), newEncoding, UNKNOWN_CHARACTER_LENGTH, CodeRange.CR_UNKNOWN);
     }
 

src/main/java/org/truffleruby/core/rope/RepeatingRope.java

@@ -11,6 +11,7 @@
 package org.truffleruby.core.rope;
 
 import org.jcodings.Encoding;
+import org.jcodings.specific.ASCIIEncoding;
 
 /**
  * A RepeatingRope always has the same encoding as its child
@@ -33,10 +34,17 @@ public RepeatingRope(ManagedRope child, int times) {
     }
 
     @Override
-    public Rope withEncoding(Encoding newEncoding, CodeRange newCodeRange) {
+    Rope withEncoding7bit(Encoding newEncoding) {
+        assert getCodeRange() == CodeRange.CR_7BIT;
         return new RepeatingRope((ManagedRope) RopeOperations.withEncoding(child, newEncoding), times);
     }
 
+    @Override
+    Rope withBinaryEncoding() {
+        assert getCodeRange() == CodeRange.CR_VALID;
+        return new RepeatingRope((ManagedRope) RopeOperations.withEncoding(child, ASCIIEncoding.INSTANCE), times);
+    }
+
     @Override
     protected byte[] getBytesSlow() {
         if (child.getRawBytes() != null) {

src/main/java/org/truffleruby/core/rope/Rope.java

@@ -39,12 +39,18 @@ protected Rope(Encoding encoding, int byteLength, int ropeDepth, byte[] bytes) {
     }
 
     /**
-     * Fast-path encoding change when there is little to do.
-     *
-     * @param newEncoding the new Encoding
-     * @param newCodeRange always CR-7BIT, except when newEncoding is BINARY can also be CR-VALID
+     * Only used internally by WithEncodingNode.
+     * Returns a Rope with the given Encoding.
+     * Both the original and new Encodings must be ASCII-compatible and the rope must be {@link #isAsciiOnly()}.
      */
-    public abstract Rope withEncoding(Encoding newEncoding, CodeRange newCodeRange);
+    abstract Rope withEncoding7bit(Encoding newEncoding);
+
+    /**
+     * Only used internally by WithEncodingNode.
+     * Returns a Rope with the BINARY Encoding.
+     * The original Encoding must be ASCII-compatible and {@link #getCodeRange()} must be {@link CodeRange#CR_VALID} to call this.
+     */
+    abstract Rope withBinaryEncoding();
 
     public abstract int characterLength();
 

src/main/java/org/truffleruby/core/rope/RopeNodes.java

@@ -1070,12 +1070,14 @@ protected Rope withEncodingAsciiCompatible(ManagedRope rope, Encoding encoding,
             if (asciiCompatibleProfile.profile(encoding.isAsciiCompatible())) {
                 if (asciiOnlyProfile.profile(rope.isAsciiOnly())) {
                     // ASCII-only strings can trivially convert to other ASCII-compatible encodings.
-                    return cachedRopeClass.cast(rope).withEncoding(encoding, CR_7BIT);
+                    return cachedRopeClass.cast(rope).withEncoding7bit(encoding);
                 } else if (binaryEncodingProfile.profile(encoding == ASCIIEncoding.INSTANCE &&
                         rope.getCodeRange() == CR_VALID &&
                         rope.getEncoding().isAsciiCompatible())) {
                     // ASCII-compatible CR_VALID strings are also CR_VALID in binary, but they might change character length.
-                    return cachedRopeClass.cast(rope).withEncoding(ASCIIEncoding.INSTANCE, CR_VALID);
+                    final Rope binary = cachedRopeClass.cast(rope).withBinaryEncoding();
+                    assert binary.getCodeRange() == CR_VALID;
+                    return binary;
                 } else {
                     // The rope either has a broken code range or isn't ASCII-compatible. In the case of a broken
                     // code range, we must perform a new code range scan with the target encoding to see if it's still

src/main/java/org/truffleruby/core/rope/RopeOperations.java

@@ -39,7 +39,6 @@
 import org.truffleruby.collections.Memo;
 import org.truffleruby.core.Hashing;
 import org.truffleruby.core.encoding.EncodingManager;
-import org.truffleruby.core.rope.RopeNodes.WithEncodingNode;
 import org.truffleruby.core.rope.RopeNodesFactory.WithEncodingNodeGen;
 import org.truffleruby.core.string.StringAttributes;
 import org.truffleruby.core.string.StringOperations;