QCharRef/QByteRef: warn when triggering the resizing operator= behavior

The whole reason of QCharRef/QByteRef existence is to have a magic
operator=. This operator= delays the actual detach up to
the moment something is written into the Ref, thus avoiding
spurious detaches to the underlying string/byte array.

operator= has also an extra feature, it allows this code to
succeed:

  QString s("abc");
  s[10] = 'z';
  assert(s == "abc      z");

This last behavior is *extremely* surprising.

The problem with all of this is that this extra convenience is
outweighted by the massive pessimization in the codegen for
operator[]; by the maintenance burden (QChar APIs need to be
mirrored in QCharRef, etc.), and, for the automatic resize, by the
fact that it's an super-niche use case.

Cherry on top, std::basic_string does not do that, and no Qt or std
container does that. In other words: any other container-like class
exhibits UB for out of bounds access.

We can't just go and change behavior, though. This is something
coming all the way back from Qt 2 (maybe even Qt 1), which means we
can't deprecate it at short notice.

This patch simply adds a warning in debug builds in case the special
resizing behavior is triggered. While at it, removes some code
duplication in QByteRef.

[ChangeLog][QtCore][QString] The behavior of operator[] to allow
implicit resizing of the string has been deprecated, and will be
removed in a future version of Qt.

[ChangeLog][QtCore][QByteArray] The behavior of operator[] to allow
implicit detaching and resizing of the byte array has been
deprecated, and will be removed in a future version of Qt.

Change-Id: I3b5c5191167f12a606bcf6e513e6f304b220d675
Reviewed-by: Marc Mutz <marc.mutz@kdab.com>
 
 
 
 
 
 
 
 
 

5 files changed, 102 insertions, 12 deletions

diff --git a/src/corelib/doc/snippets/code/src_corelib_tools_qbytearray.cpp b/src/corelib/doc/snippets/code/src_corelib_tools_qbytearray.cpp
index ec63e64fe9..11ab50687d 100644
--- a/src/corelib/doc/snippets/code/src_corelib_tools_qbytearray.cpp
+++ b/src/corelib/doc/snippets/code/src_corelib_tools_qbytearray.cpp
@@ -133,10 +133,10 @@ while (*data) {
 
 
 //! [9]
-QByteArray ba;
-for (int i = 0; i < 10; ++i)
-    ba[i] = 'A' + i;
-// ba == "ABCDEFGHIJ"
+QByteArray ba("Hello, world");
+cout << ba[0]; // prints H
+ba[7] = 'W';
+// ba == "Hello, World"
 //! [9]
 
 
diff --git a/src/corelib/tools/qbytearray.cpp b/src/corelib/tools/qbytearray.cpp
index 720a91af49..c89fb078f7 100644
--- a/src/corelib/tools/qbytearray.cpp
+++ b/src/corelib/tools/qbytearray.cpp
@@ -1537,6 +1537,12 @@ QByteArray &QByteArray::operator=(const char *str)
     will apply to the character in the QByteArray from which you got
     the reference.
 
+    \note Before Qt 5.14 it was possible to use this operator to access
+    a character at an out-of-bounds position in the byte array, and
+    then assign to such position, causing the byte array to be
+    automatically resized. This behavior is deprecated, and will be
+    changed in a future version of Qt.
+
     \sa at()
 */
 
@@ -5054,4 +5060,32 @@ QByteArray QByteArray::toPercentEncoding(const QByteArray &exclude, const QByteA
     \sa QStringLiteral
 */
 
+namespace QtPrivate {
+namespace DeprecatedRefClassBehavior {
+void warn(EmittingClass c)
+{
+    const char *emittingClassName = nullptr;
+    const char *containerClassName = nullptr;
+    switch (c) {
+    case EmittingClass::QByteRef:
+        emittingClassName = "QByteRef";
+        containerClassName = "QByteArray";
+        break;
+    case EmittingClass::QCharRef:
+        emittingClassName = "QCharRef";
+        containerClassName = "QString";
+        break;
+    }
+
+    qWarning("Using %s with an index pointing outside"
+             " the valid range of a %s."
+             " The corresponding behavior is deprecated, and will be changed"
+             " in a future version of Qt.",
+             emittingClassName,
+             containerClassName);
+}
+} // namespace DeprecatedRefClassBehavior
+} // namespace QtPrivate
+
+
 QT_END_NAMESPACE
diff --git a/src/corelib/tools/qbytearray.h b/src/corelib/tools/qbytearray.h
index d21cb9b363..5c7229e40d 100644
--- a/src/corelib/tools/qbytearray.h
+++ b/src/corelib/tools/qbytearray.h
@@ -528,6 +528,17 @@ inline void QByteArray::squeeze()
     }
 }
 
+namespace QtPrivate {
+namespace DeprecatedRefClassBehavior {
+    enum class EmittingClass {
+        QByteRef,
+        QCharRef,
+    };
+
+    Q_CORE_EXPORT Q_DECL_COLD_FUNCTION void warn(EmittingClass c);
+} // namespace DeprecatedAssignmentOperatorBehavior
+} // namespace QtPrivate
+
 class
 #if QT_VERSION < QT_VERSION_CHECK(6, 0, 0)
 Q_CORE_EXPORT
@@ -540,13 +551,33 @@ QByteRef {
     friend class QByteArray;
 public:
     inline operator char() const
-        { return i < a.d->size ? a.d->data()[i] : char(0); }
+    {
+        using namespace QtPrivate::DeprecatedRefClassBehavior;
+        if (Q_LIKELY(i < a.d->size))
+            return a.d->data()[i];
+#ifdef QT_DEBUG
+        warn(EmittingClass::QByteRef);
+#endif
+        return char(0);
+    }
     inline QByteRef &operator=(char c)
-        { if (i >= a.d->size) a.expand(i); else a.detach();
-          a.d->data()[i] = c;  return *this; }
+    {
+        using namespace QtPrivate::DeprecatedRefClassBehavior;
+        if (Q_UNLIKELY(i >= a.d->size)) {
+#ifdef QT_DEBUG
+            warn(EmittingClass::QByteRef);
+#endif
+            a.expand(i);
+        } else {
+            a.detach();
+        }
+        a.d->data()[i] = c;
+        return *this;
+    }
     inline QByteRef &operator=(const QByteRef &c)
-        { if (i >= a.d->size) a.expand(i); else a.detach();
-          a.d->data()[i] = c.a.d->data()[c.i];  return *this; }
+    {
+        return operator=(char(c));
+    }
     inline bool operator==(char c) const
     { return a.d->data()[i] == c; }
     inline bool operator!=(char c) const
diff --git a/src/corelib/tools/qstring.cpp b/src/corelib/tools/qstring.cpp
index a5396bfb69..c73474684b 100644
--- a/src/corelib/tools/qstring.cpp
+++ b/src/corelib/tools/qstring.cpp
@@ -5782,6 +5782,12 @@ QString QString::trimmed_helper(QString &str)
     were a QChar &. If you assign to it, the assignment will apply to
     the character in the QString from which you got the reference.
 
+    \note Before Qt 5.14 it was possible to use this operator to access
+    a character at an out-of-bounds position in the string, and
+    then assign to such position, causing the string to be
+    automatically resized. This behavior is deprecated, and will be
+    changed in a future version of Qt.
+
     \sa at()
 */
 
diff --git a/src/corelib/tools/qstring.h b/src/corelib/tools/qstring.h
index 1abb91eabe..1feb8e186c 100644
--- a/src/corelib/tools/qstring.h
+++ b/src/corelib/tools/qstring.h
@@ -1060,10 +1060,29 @@ public:
 
     // all this is not documented: We just say "like QChar" and let it be.
     inline operator QChar() const
-    { return i < s.d->size ? s.d->data()[i] : 0; }
+    {
+        using namespace QtPrivate::DeprecatedRefClassBehavior;
+        if (Q_LIKELY(i < s.d->size))
+            return s.d->data()[i];
+#ifdef QT_DEBUG
+        warn(EmittingClass::QCharRef);
+#endif
+        return 0;
+    }
     inline QCharRef &operator=(QChar c)
-    { if (i >= s.d->size) s.resize(i + 1, QLatin1Char(' ')); else s.detach();
-      s.d->data()[i] = c.unicode(); return *this; }
+    {
+        using namespace QtPrivate::DeprecatedRefClassBehavior;
+        if (Q_UNLIKELY(i >= s.d->size)) {
+#ifdef QT_DEBUG
+            warn(EmittingClass::QCharRef);
+#endif
+            s.resize(i + 1, QLatin1Char(' '));
+        } else {
+            s.detach();
+        }
+        s.d->data()[i] = c.unicode();
+        return *this;
+    }
 
     // An operator= for each QChar cast constructors
 #ifndef QT_NO_CAST_FROM_ASCII