Rename utf8::is_utf8() (and other functions)

[p5pRT]

Migrated from rt.perl.org#131685 (status was 'open')

Searchable as RT131685$

[p5pRT]

From @pali

Hi!

This is continuation from original discussion about renaming
utf8​::is_utf8() to utf8​::is_upgraded() which can be found at​:
https://www.nntp.perl.org/group/perl.perl5.porters/2017/02/msg243068.html

Problem is that in more perl modules is used this incorrect code
pattern​:

  use utf8;

  my $value = func();
  if (utf8​::is_utf8($value)) {
  utf8​::encode($value);
  }

In most cases module developers think that utf8​::is_utf8() returns true
when it is needed to manually encode argument into UTF-8 bytes. Which is
of course wrong.

Reason for this is poor name of function utf8​::is_utf8() and also poor
documentation about this function.

Functions utf8​::is_utf8(), utf8​::upgrade() and utf8​::downgrade() changes
internal string representation, which is fully invisible for pure perl
code, and therefore I think all those functions should be in Internals
namespace.

I'm proposing following rename of functions​:

utf8​::is_utf8() --> Internals​::uses_string_wide_storage()
utf8​::upgrade() --> Internals​::upgrade_string_to_wide_storage()
utf8​::downgrade() --> Internals​::downgrade_string_from_wide_storage()

Plus adding backward compatible aliases to make existing code works like
before.

As all those functions should be used only for debugging purposes (e.g.
test cases for XS code) or when dealing with buggy XS module, I'm
proposing starting to throw warning (e.g. since v5.28.0) when those
functions are called. For those who are dealing with internals, can turn
warning off by no warnings 'experimental​::internal';

I'm attaching patches which​:

* Add new warning category 'experimental​::internal'
* Rename utf8 functions
* Update perldoc utf8 documentation

[p5pRT]

From @pali

0001-Add-new-warning-category-experimental-internal.patch

From c7b1fcfd26a2500662a10e345691eda3f3f32039 Mon Sep 17 00:00:00 2001
From: Pali <pali@cpan.org>
Date: Sat, 1 Jul 2017 17:33:45 +0200
Subject: [PATCH 1/3] Add new warning category experimental::internal

This category is for internal perl functions which should not be used in
normal perl code, unless dealing with perl internals.
---
 lib/warnings.pm   |   19 +++++++++++++------
 regen/warnings.pl |    2 ++
 warnings.h        |    4 ++++
 3 files changed, 19 insertions(+), 6 deletions(-)

diff --git a/lib/warnings.pm b/lib/warnings.pm
index 2ae1bb4..7b27e4a 100644
--- a/lib/warnings.pm
+++ b/lib/warnings.pm
@@ -96,10 +96,13 @@ our %Offsets = (
 
     # Warnings Categories added in Perl 5.025
     'experimental::declared_refs'	=> 132,
+
+    # Warnings Categories added in Perl 5.028
+    'experimental::internal'		=> 134,
 );
 
 our %Bits = (
-    'all'				=> "\x55\x55\x55\x55\x55\x55\x55\x55\x55\x55\x55\x55\x55\x55\x55\x55\x15", # [0..66]
+    'all'				=> "\x55\x55\x55\x55\x55\x55\x55\x55\x55\x55\x55\x55\x55\x55\x55\x55\x55", # [0..67]
     'ambiguous'				=> "\x00\x00\x00\x00\x00\x00\x00\x04\x00\x00\x00\x00\x00\x00\x00\x00\x00", # [29]
     'bareword'				=> "\x00\x00\x00\x00\x00\x00\x00\x10\x00\x00\x00\x00\x00\x00\x00\x00\x00", # [30]
     'closed'				=> "\x00\x10\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00", # [6]
@@ -109,10 +112,11 @@ our %Bits = (
     'digit'				=> "\x00\x00\x00\x00\x00\x00\x00\x40\x00\x00\x00\x00\x00\x00\x00\x00\x00", # [31]
     'exec'				=> "\x00\x40\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00", # [7]
     'exiting'				=> "\x40\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00", # [3]
-    'experimental'			=> "\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x40\x55\x51\x15\x10", # [51..56,58..62,66]
+    'experimental'			=> "\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x40\x55\x51\x15\x50", # [51..56,58..62,66,67]
     'experimental::bitwise'		=> "\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x10\x00\x00", # [58]
     'experimental::const_attr'		=> "\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x40\x00\x00", # [59]
     'experimental::declared_refs'	=> "\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x10", # [66]
+    'experimental::internal'		=> "\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x40", # [67]
     'experimental::lexical_subs'	=> "\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x01\x00\x00\x00", # [52]
     'experimental::postderef'		=> "\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x40\x00\x00\x00", # [55]
     'experimental::re_strict'		=> "\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x01\x00", # [60]
@@ -169,7 +173,7 @@ our %Bits = (
 );
 
 our %DeadBits = (
-    'all'				=> "\xaa\xaa\xaa\xaa\xaa\xaa\xaa\xaa\xaa\xaa\xaa\xaa\xaa\xaa\xaa\xaa\x2a", # [0..66]
+    'all'				=> "\xaa\xaa\xaa\xaa\xaa\xaa\xaa\xaa\xaa\xaa\xaa\xaa\xaa\xaa\xaa\xaa\xaa", # [0..67]
     'ambiguous'				=> "\x00\x00\x00\x00\x00\x00\x00\x08\x00\x00\x00\x00\x00\x00\x00\x00\x00", # [29]
     'bareword'				=> "\x00\x00\x00\x00\x00\x00\x00\x20\x00\x00\x00\x00\x00\x00\x00\x00\x00", # [30]
     'closed'				=> "\x00\x20\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00", # [6]
@@ -179,10 +183,11 @@ our %DeadBits = (
     'digit'				=> "\x00\x00\x00\x00\x00\x00\x00\x80\x00\x00\x00\x00\x00\x00\x00\x00\x00", # [31]
     'exec'				=> "\x00\x80\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00", # [7]
     'exiting'				=> "\x80\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00", # [3]
-    'experimental'			=> "\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x80\xaa\xa2\x2a\x20", # [51..56,58..62,66]
+    'experimental'			=> "\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x80\xaa\xa2\x2a\xa0", # [51..56,58..62,66,67]
     'experimental::bitwise'		=> "\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x20\x00\x00", # [58]
     'experimental::const_attr'		=> "\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x80\x00\x00", # [59]
     'experimental::declared_refs'	=> "\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x20", # [66]
+    'experimental::internal'		=> "\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x80", # [67]
     'experimental::lexical_subs'	=> "\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x02\x00\x00\x00", # [52]
     'experimental::postderef'		=> "\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x80\x00\x00\x00", # [55]
     'experimental::re_strict'		=> "\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x02\x00", # [60]
@@ -240,8 +245,8 @@ our %DeadBits = (
 
 # These are used by various things, including our own tests
 our $NONE				=  "\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0";
-our $DEFAULT				=  "\x10\x01\x00\x00\x00\x50\x04\x00\x00\x00\x00\x00\x00\x55\x51\x55\x10", # [2,4,22,23,25,52..56,58..63,66]
-our $LAST_BIT				=  134 ;
+our $DEFAULT				=  "\x10\x01\x00\x00\x00\x50\x04\x00\x00\x00\x00\x00\x00\x55\x51\x55\x50", # [2,4,22,23,25,52..56,58..63,66,67]
+our $LAST_BIT				=  136 ;
 our $BYTES				=  17 ;
 
 our $All = "" ; vec($All, $Offsets{'all'}, 2) = 3 ;
@@ -732,6 +737,8 @@ The current hierarchy is:
          |                 |
          |                 +- experimental::declared_refs
          |                 |
+         |                 +- experimental::internal
+         |                 |
          |                 +- experimental::lexical_subs
          |                 |
          |                 +- experimental::postderef
diff --git a/regen/warnings.pl b/regen/warnings.pl
index 5721c17..36ce14b 100644
--- a/regen/warnings.pl
+++ b/regen/warnings.pl
@@ -107,6 +107,8 @@ my $tree = {
                                     [ 5.021, DEFAULT_ON ],
                                 'experimental::declared_refs' =>
                                     [ 5.025, DEFAULT_ON ],
+                                'experimental::internal' =>
+                                    [ 5.028, DEFAULT_ON ],
                         }],
 
         'missing'       => [ 5.021, DEFAULT_OFF],
diff --git a/warnings.h b/warnings.h
index 0166837..72e27a2 100644
--- a/warnings.h
+++ b/warnings.h
@@ -115,6 +115,10 @@
 
 #define WARN_EXPERIMENTAL__DECLARED_REFS 66
 
+/* Warnings Categories added in Perl 5.028 */
+
+#define WARN_EXPERIMENTAL__INTERNAL	 67
+
 #define WARNsize			 17
 #define WARN_ALLstring			 "\125\125\125\125\125\125\125\125\125\125\125\125\125\125\125\125\125"
 #define WARN_NONEstring			 "\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"
-- 
1.7.9.5

[p5pRT]

From @pali

0002-Mark-functions-utf8-is_utf8-utf8-upgrade-utf8-downgr.patch

From d763a8a4b85b53ebc5b05ba1b0a64daf9df6c2e2 Mon Sep 17 00:00:00 2001
From: Pali <pali@cpan.org>
Date: Sat, 1 Jul 2017 17:41:15 +0200
Subject: [PATCH 2/3] Mark functions utf8::is_utf8(), utf8::upgrade(),
 utf8::downgrade() as Internal

Move all those functions into Internals namespace, throw new warning
experimental::internal warning when used and provide backward compatible
deprecated aliases (for make existing code still work).

In most cases all those functions are incorrectly used due to poor names
and not proper documentation. Those functions are internal and should not
be used unless debugging perl or dealing with broken XS modules.
---
 universal.c |   65 ++++++++++++++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 55 insertions(+), 10 deletions(-)

diff --git a/universal.c b/universal.c
index be39310..20f1d53 100644
--- a/universal.c
+++ b/universal.c
@@ -422,10 +422,24 @@ XS(XS_UNIVERSAL_DOES)
     }
 }
 
-XS(XS_utf8_is_utf8); /* prototype to pass -Wmissing-prototypes */
-XS(XS_utf8_is_utf8)
+XS(XS_Internals_uses_string_wide_storage); /* prototype to pass -Wmissing-prototypes */
+XS(XS_Internals_uses_string_wide_storage)
 {
-     dXSARGS;
+    dXSARGS;
+    const GV *const gv = CvGV(cv);
+    const HV *const stash = gv ? GvSTASH(gv) : NULL;
+    const char *const hvname = stash ? HvNAME(stash) : NULL;
+
+    if (hvname && strcmp(hvname, "utf8") == 0) {
+        Perl_ck_warner_d(aTHX_
+            packWARN(WARN_DEPRECATED),
+            "utf8::is_utf8() is internal and deprecated function, look into perldoc utf8");
+    } else {
+        Perl_ck_warner_d(aTHX_
+            packWARN(WARN_INTERNAL),
+            "Internals::uses_string_wide_storage() is experimental internal perl function");
+    }
+
      if (items != 1)
 	 croak_xs_usage(cv, "sv");
      else {
@@ -485,10 +499,24 @@ XS(XS_utf8_decode)
     XSRETURN(1);
 }
 
-XS(XS_utf8_upgrade); /* prototype to pass -Wmissing-prototypes */
-XS(XS_utf8_upgrade)
+XS(XS_Internals_upgrade_string_to_wide_storage); /* prototype to pass -Wmissing-prototypes */
+XS(XS_Internals_upgrade_string_to_wide_storage)
 {
     dXSARGS;
+    const GV *const gv = CvGV(cv);
+    const HV *const stash = gv ? GvSTASH(gv) : NULL;
+    const char *const hvname = stash ? HvNAME(stash) : NULL;
+
+    if (hvname && strcmp(hvname, "utf8") == 0) {
+        Perl_ck_warner_d(aTHX_
+            packWARN(WARN_DEPRECATED),
+            "utf8::upgrade() is internal and deprecated function, look into perldoc utf8");
+    } else {
+        Perl_ck_warner_d(aTHX_
+            packWARN(WARN_INTERNAL),
+            "Internals::upgrade_string_to_wide_storage() is experimental internal perl function");
+    }
+
     if (items != 1)
 	croak_xs_usage(cv, "sv");
     else {
@@ -502,10 +530,24 @@ XS(XS_utf8_upgrade)
     XSRETURN(1);
 }
 
-XS(XS_utf8_downgrade); /* prototype to pass -Wmissing-prototypes */
-XS(XS_utf8_downgrade)
+XS(XS_Internals_downgrade_string_from_wide_storage); /* prototype to pass -Wmissing-prototypes */
+XS(XS_Internals_downgrade_string_from_wide_storage)
 {
     dXSARGS;
+    const GV *const gv = CvGV(cv);
+    const HV *const stash = gv ? GvSTASH(gv) : NULL;
+    const char *const hvname = stash ? HvNAME(stash) : NULL;
+
+    if (hvname && strcmp(hvname, "utf8") == 0) {
+        Perl_ck_warner_d(aTHX_
+            packWARN(WARN_DEPRECATED),
+            "utf8::downgrade() is internal and deprecated function, look into perldoc utf8");
+    } else {
+        Perl_ck_warner_d(aTHX_
+            packWARN(WARN_EXPERIMENTAL__INTERNAL),
+            "Internals::downgrade_string_from_wide_storage() is experimental internal function");
+    }
+
     if (items < 1 || items > 2)
 	croak_xs_usage(cv, "sv, failok=0");
     else {
@@ -1000,14 +1042,17 @@ static const struct xsub_details details[] = {
 #define VXS_XSUB_DETAILS
 #include "vxs.inc"
 #undef VXS_XSUB_DETAILS
-    {"utf8::is_utf8", XS_utf8_is_utf8, NULL},
+    {"utf8::is_utf8", XS_Internals_uses_string_wide_storage, NULL},
     {"utf8::valid", XS_utf8_valid, NULL},
     {"utf8::encode", XS_utf8_encode, NULL},
     {"utf8::decode", XS_utf8_decode, NULL},
-    {"utf8::upgrade", XS_utf8_upgrade, NULL},
-    {"utf8::downgrade", XS_utf8_downgrade, NULL},
+    {"utf8::upgrade", XS_Internals_upgrade_string_to_wide_storage, NULL},
+    {"utf8::downgrade", XS_Internals_downgrade_string_from_wide_storage, NULL},
     {"utf8::native_to_unicode", XS_utf8_native_to_unicode, NULL},
     {"utf8::unicode_to_native", XS_utf8_unicode_to_native, NULL},
+    {"Internals::uses_string_wide_storage", XS_Internals_uses_string_wide_storage, NULL},
+    {"Internals::upgrade_string_to_wide_storage", XS_Internals_upgrade_string_to_wide_storage, NULL},
+    {"Internals::downgrade_string_from_wide_storage", XS_Internals_downgrade_string_from_wide_storage, NULL},
     {"Internals::SvREADONLY", XS_Internals_SvREADONLY, "\\[$%@];$"},
     {"Internals::SvREFCNT", XS_Internals_SvREFCNT, "\\[$%@];$"},
     {"Internals::hv_clear_placeholders", XS_Internals_hv_clear_placehold, "\\%"},
-- 
1.7.9.5

[p5pRT]

From @pali

0003-Update-documentation-in-perldoc-utf8.patch

From e5b0bbd18075ea178708f5da32beee3570751f0e Mon Sep 17 00:00:00 2001
From: Pali <pali@cpan.org>
Date: Sat, 1 Jul 2017 17:46:05 +0200
Subject: [PATCH 3/3] Update documentation in perldoc utf8

Add information about new internal functions and update documentation for
wide string storage functions.
---
 lib/utf8.pm |   93 ++++++++++++++++++++++++++++++++++-------------------------
 1 file changed, 54 insertions(+), 39 deletions(-)

diff --git a/lib/utf8.pm b/lib/utf8.pm
index 324cb87..84a96ae 100644
--- a/lib/utf8.pm
+++ b/lib/utf8.pm
@@ -31,14 +31,8 @@ utf8 - Perl pragma to enable/disable UTF-8 (or UTF-EBCDIC) in source code
  use utf8;
  no utf8;
 
- # Convert the internal representation of a Perl scalar to/from UTF-8.
-
- $num_octets = utf8::upgrade($string);
- $success    = utf8::downgrade($string[, $fail_ok]);
-
  # Change each character of a Perl scalar to/from a series of
  # characters that represent the UTF-8 bytes of each original character.
-
  utf8::encode($string);  # "\x{100}"  becomes "\xc4\x80"
  utf8::decode($string);  # "\xc4\x80" becomes "\x{100}"
 
@@ -51,7 +45,6 @@ utf8 - Perl pragma to enable/disable UTF-8 (or UTF-EBCDIC) in source code
                                                # platforms; 193 on
                                                # EBCDIC
 
- $flag = utf8::is_utf8($string); # since Perl 5.8.1
  $flag = utf8::valid($string);
 
 =head1 DESCRIPTION
@@ -105,39 +98,46 @@ you should not say that unless you really want to have UTF-8 source code.
 
 =item * C<$num_octets = utf8::upgrade($string)>
 
-(Since Perl v5.8.0)
-Converts in-place the internal representation of the string from an octet
-sequence in the native encoding (Latin-1 or EBCDIC) to UTF-8. The
-logical character sequence itself is unchanged.  If I<$string> is already
-stored as UTF-8, then this is a no-op. Returns the
-number of octets necessary to represent the string as UTF-8.  Can be
-used to make sure that the UTF-8 flag is on, so that C<\w> or C<lc()>
-work as Unicode on strings containing non-ASCII characters whose code points
-are below 256.
+[INTERNAL] (Since Perl v5.8.0) Deprecated compatibility-supporting alias of
+C<Internals::upgrade_string_to_wide_storage>
 
-B<Note that this function does not handle arbitrary encodings>;
-use L<Encode> instead.
+=item * C<$num_octets = Internals::upgrade_string_to_wide_storage($string)>
 
-=item * C<$success = utf8::downgrade($string[, $fail_ok])>
+[INTERNAL] (Since Perl v5.28.0)
 
-(Since Perl v5.8.0)
-Converts in-place the internal representation of the string from
-UTF-8 to the equivalent octet sequence in the native encoding (Latin-1
-or EBCDIC). The logical character sequence itself is unchanged. If
-I<$string> is already stored as native 8 bit, then this is a no-op.  Can
-be used to
-make sure that the UTF-8 flag is off, e.g. when you want to make sure
-that the substr() or length() function works with the usually faster
-byte algorithm.
-
-Fails if the original UTF-8 sequence cannot be represented in the
-native 8 bit encoding. On failure dies or, if the value of I<$fail_ok> is
-true, returns false. 
+Converts in-place the internal representation of the string to wide storage
+(which can store characters above U+0000FF).  The logical character sequence
+itself is unchanged.  If I<$string> is already stored in wide storage then
+this is a no-op.  Returns the number of bytes necessary to represent the
+string in wide storage.
+
+Internal string storage is invisible for pure perl code and perl itself call
+this function automatically when needed.  Therefore there is no reason to call
+this function unless you are debugging internal perl C or XS code.
+
+=item * C<$num_octets = Internals::downgrade_string_from_wide_storage($string[, $fail_ok])>
+
+[INTERNAL] (Since Perl v5.28.0)
+
+Converts in-place the internal representation of the string from wide storage
+(which can store characters above U+0000FF) to small non-wide 8 bit storage
+(which can store only 8 bit characters).  The logical character sequence
+itself is unchanged.  If I<$string> is already stored in non-wide 8 bit storage,
+then this is a no-op.
+
+Fails if the original I<$string> cannot be represented in the native 8 bit
+encoding.  On failure dies or, if the value of I<$fail_ok> is true, returns false.
 
 Returns true on success.
 
-B<Note that this function does not handle arbitrary encodings>;
-use L<Encode> instead.
+Internal string storage is invisible for pure perl code and perl itself call
+this function automatically when needed.  Therefore there is no reason to call
+this function unless you are debugging internal perl C or XS code.
+
+=item * C<$success = utf8::downgrade($string[, $fail_ok])>
+
+[INTERNAL] (Since Perl v5.8.0)  Deprecated compatibility-supporting alias of
+C<Internals::downgrade_string_from_wide_storage>
 
 =item * C<utf8::encode($string)>
 
@@ -207,17 +207,32 @@ platforms, so there is no performance hit in using it there.
 
 =item * C<$flag = utf8::is_utf8($string)>
 
-(Since Perl 5.8.1)  Test whether I<$string> is marked internally as encoded in
-UTF-8.  Functionally the same as C<Encode::is_utf8()>.
+[INTERNAL] (Since Perl v5.8.1)  Deprecated compatibility-supporting (but poorly
+named) alias of C<Internals::uses_string_wide_storage()>.  It does B<not> check
+if string is encoded in UTF-8.
+
+=item * C<$flag = Internals::uses_string_wide_storage($string)>
+
+[INTERNAL] (Since Perl v5.28.0)
+
+Test whether C<$string>'s internal representation storage is wide (which can
+store characters above U+0000FF).  Note that C<$string> can, but does not have
+to contain wide characters.  It bears no impact on whether that string is
+actually utf8 or not.
+
+Internal string storage is invisible for pure perl code and perl itself call
+change storage automatically when needed.  This function should not be used
+unless you are debugging internal perl C or XS code.
 
 =item * C<$flag = utf8::valid($string)>
 
-[INTERNAL] Test whether I<$string> is in a consistent state regarding
+[INTERNAL] (Since Perl v5.8.0)
+
+Test whether I<$string> is in a consistent state regarding
 UTF-8.  Will return true if it is well-formed UTF-8 and has the UTF-8 flag
 on B<or> if I<$string> is held as bytes (both these states are 'consistent').
 Main reason for this routine is to allow Perl's test suite to check
-that operations have left strings in a consistent state.  You most
-probably want to use C<utf8::is_utf8()> instead.
+that operations have left strings in a consistent state.
 
 =back
 
-- 
1.7.9.5

[p5pRT]

From @Tux

On Sat, 01 Jul 2017 09​:03​:18 -0700, (via RT)
<perlbug-followup@​perl.org> wrote​:

# New Ticket Created by
# Please include the string​: [perl #131685]
# in the subject line of all future correspondence about this issue.
# <URL​: https://rt-archive.perl.org/perl5/Ticket/Display.html?id=131685 >

Hi!

This is continuation from original discussion about renaming
utf8​::is_utf8() to utf8​::is_upgraded() which can be found at​:
https://www.nntp.perl.org/group/perl.perl5.porters/2017/02/msg243068.html

Problem is that in more perl modules is used this incorrect code
pattern​:

use utf8;

my $value = func();
if (utf8​::is_utf8($value)) {
utf8​::encode($value);
}

In most cases module developers think that utf8​::is_utf8() returns true
when it is needed to manually encode argument into UTF-8 bytes. Which is
of course wrong.

Reason for this is poor name of function utf8​::is_utf8() and also poor
documentation about this function.

Functions utf8​::is_utf8(), utf8​::upgrade() and utf8​::downgrade() changes
internal string representation, which is fully invisible for pure perl
code, and therefore I think all those functions should be in Internals
namespace.

I'm proposing following rename of functions​:

utf8​::is_utf8() --> Internals​::uses_string_wide_storage()
utf8​::upgrade() --> Internals​::upgrade_string_to_wide_storage()
utf8​::downgrade() --> Internals​::downgrade_string_from_wide_storage()

I am still objecting, as this will also break code that uses those
functions as intended and correctly.

As these are not XS, Devel​::PPPort won't help (assuming authors use
D​::P on XS modules to guarantee backward compat)

I'd loath to change/fix every occurrence of code that uses any of these
three correctly, as that code is brittle to start with and probably
hard to fix when broken.

Plus adding backward compatible aliases to make existing code works like
before.

Then why add new functions in the first place?

As all those functions should be used only for debugging purposes (e.g.
test cases for XS code) or when dealing with buggy XS module, I'm
proposing starting to throw warning (e.g. since v5.28.0) when those
functions are called. For those who are dealing with internals, can turn
warning off by no warnings 'experimental​::internal';

No, please. Most correct uses will be in dark distant corners, hidden
in modules you don't want to touch anyway.

I'm attaching patches which​:

* Add new warning category 'experimental​::internal'
* Rename utf8 functions
* Update perldoc utf8 documentation

--
H.Merijn Brand http​://tux.nl Perl Monger http​://amsterdam.pm.org/
using perl5.00307 .. 5.27 porting perl5 on HP-UX, AIX, and openSUSE
http​://mirrors.develooper.com/hpux/ http​://www.test-smoke.org/
http​://qa.perl.org http​://www.goldmark.org/jeff/stupid-disclaimers/

[p5pRT]

The RT System itself - Status changed from 'new' to 'open'

[p5pRT]

From @Leont

On Sat, Jul 1, 2017 at 6​:03 PM, via RT <perlbug-followup@​perl.org> wrote​:

Hi!

This is continuation from original discussion about renaming
utf8​::is_utf8() to utf8​::is_upgraded() which can be found at​:
https://www.nntp.perl.org/group/perl.perl5.porters/2017/02/msg243068.html

Problem is that in more perl modules is used this incorrect code
pattern​:

use utf8;

my $value = func();
if (utf8​::is_utf8($value)) {
utf8​::encode($value);
}

In most cases module developers think that utf8​::is_utf8() returns true
when it is needed to manually encode argument into UTF-8 bytes. Which is
of course wrong.

Reason for this is poor name of function utf8​::is_utf8() and also poor
documentation about this function.

Functions utf8​::is_utf8(), utf8​::upgrade() and utf8​::downgrade() changes
internal string representation, which is fully invisible for pure perl
code, and therefore I think all those functions should be in Internals
namespace.

I'm proposing following rename of functions​:

utf8​::is_utf8() --> Internals​::uses_string_wide_storage()
utf8​::upgrade() --> Internals​::upgrade_string_to_wide_storage()
utf8​::downgrade() --> Internals​::downgrade_string_from_wide_storage()

Plus adding backward compatible aliases to make existing code works like
before.

As all those functions should be used only for debugging purposes (e.g.
test cases for XS code) or when dealing with buggy XS module, I'm
proposing starting to throw warning (e.g. since v5.28.0) when those
functions are called. For those who are dealing with internals, can turn
warning off by no warnings 'experimental​::internal';

I'm attaching patches which​:

* Add new warning category 'experimental​::internal'
* Rename utf8 functions
* Update perldoc utf8 documentation

I don't see how this is an option. I'll grant you that something like this
would have been a better option back then but you're 15 years too late.
"This would have been better" is no excuse to break a decade and a half of
software.

Leon

[p5pRT]

From @pali

On Saturday 01 July 2017 19​:13​:30 you wrote​:

to break a decade and a half of software.

Hm? What you mean with to break? Existing functions would still work,
just there are also new functions under new names. Usage of old
functions is just removed from documentation.

[p5pRT]

From @pali

On Saturday 01 July 2017 18​:54​:24 you wrote​:

Plus adding backward compatible aliases to make existing code works
like before.

Then why add new functions in the first place?

From discussion it was clear that current name utf8​::is_utf8() is poor
and is reason why it is incorrectly used.

[p5pRT]

From @Leont

On Sat, Jul 1, 2017 at 7​:45 PM, <pali@​cpan.org> wrote​:

On Saturday 01 July 2017 19​:13​:30 you wrote​:

to break a decade and a half of software.

Hm? What you mean with to break? Existing functions would still work,
just there are also new functions under new names. Usage of old
functions is just removed from documentation.

Then I misunderstood your proposal, "rename" suggested to me that the old
ones disappear. In that case I'm not sure I see the benefit of your
proposal. Why would anyone want to use an interface that won't work on
perls older than 5.28, and could disappear in a future version of perl
(since that's the point of Internals​::)? This isn't making sense to me.

Leon

[p5pRT]

From @xsawyerx

On 07/01/2017 01​:52 PM, Leon Timmermans wrote​:

On Sat, Jul 1, 2017 at 7​:45 PM, <pali@​cpan.org <mailto​:pali@​cpan.org>>
wrote​:

On Saturday 01 July 2017 19​:13​:30 you wrote​:
> to break a decade and a half of software\.

Hm? What you mean with to break? Existing functions would still work\,
just there are also new functions under new names\. Usage of old
functions is just removed from documentation\.

Then I misunderstood your proposal, "rename" suggested to me that the
old ones disappear. In that case I'm not sure I see the benefit of
your proposal. Why would anyone want to use an interface that won't
work on perls older than 5.28, and could disappear in a future version
of perl (since that's the point of Internals​::)? This isn't making
sense to me.

You could support it with Devel​::PPPort. It's a simple addition.

However, the problem remains that if someone were to use these new
functions without PPPort, their code would not work on older versions. I
can't see a way around that.

[p5pRT]

From @tonycoz

On Mon, Jul 03, 2017 at 01​:03​:37PM -0400, Sawyer X wrote​:

On 07/01/2017 01​:52 PM, Leon Timmermans wrote​:

On Sat, Jul 1, 2017 at 7​:45 PM, <pali@​cpan.org <mailto​:pali@​cpan.org>>
wrote​:

On Saturday 01 July 2017 19​:13​:30 you wrote​:
> to break a decade and a half of software\.

Hm? What you mean with to break? Existing functions would still work\,
just there are also new functions under new names\. Usage of old
functions is just removed from documentation\.

Then I misunderstood your proposal, "rename" suggested to me that the
old ones disappear. In that case I'm not sure I see the benefit of
your proposal. Why would anyone want to use an interface that won't
work on perls older than 5.28, and could disappear in a future version
of perl (since that's the point of Internals​::)? This isn't making
sense to me.

You could support it with Devel​::PPPort. It's a simple addition.

However, the problem remains that if someone were to use these new
functions without PPPort, their code would not work on older versions. I
can't see a way around that.

These are perl functions (as documented in utf8.pm), not C functions,
Devel​::PPPort does nothing for us.

The patch retains the old names, so that isn't an issue.

But it does deprecate the old names, which is an issue, I can't
imagine us removing these functions.

As a side note, the original thread refers to​:

https://metacpan.org/source/SHAY/perl-5.24.1/cpan/Archive-Tar/lib/Archive/Tar.pm#L1501

which I could see as correct because of the way perl's unicode support
(fails to) deal with filenames.

Tony

[p5pRT]

From @Grinnz

On Mon, Jul 3, 2017 at 8​:38 PM, Tony Cook <tony@​develop-help.com> wrote​:

As a side note, the original thread refers to​:

https://metacpan.org/source/SHAY/perl-5.24.1/cpan/Archive-
Tar/lib/Archive/Tar.pm#L1501

which I could see as correct because of the way perl's unicode support
(fails to) deal with filenames.

Tony

Not entirely correct IMO. If the intent is that filenames be encoded to
UTF-8, this will fail to encode downgraded names with non-ascii characters.

-Dan

[p5pRT]

From @tonycoz

On Mon, Jul 03, 2017 at 09​:35​:06PM -0400, Dan Book wrote​:

On Mon, Jul 3, 2017 at 8​:38 PM, Tony Cook <tony@​develop-help.com> wrote​:

As a side note, the original thread refers to​:

https://metacpan.org/source/SHAY/perl-5.24.1/cpan/Archive-
Tar/lib/Archive/Tar.pm#L1501

which I could see as correct because of the way perl's unicode support
(fails to) deal with filenames.

Tony

Not entirely correct IMO. If the intent is that filenames be encoded to
UTF-8, this will fail to encode downgraded names with non-ascii characters.

If the caller creates a file using the name they pass in, encoding the
name (which might not be utf-8 marked) may make the later -e or -l
check fail.

Perl functions such as open and stat currently ignore the the UTF-8
flag, which makes this pretty messy.

The code in Archive​::Tar seems a reasonable workaround to me, I don't
think the author had much choice.

Tony

[p5pRT]

From @pali

On Monday 03 July 2017 21​:35​:06 Dan Book wrote​:

On Mon, Jul 3, 2017 at 8​:38 PM, Tony Cook <tony@​develop-help.com> wrote​:

As a side note, the original thread refers to​:

https://metacpan.org/source/SHAY/perl-5.24.1/cpan/Archive-
Tar/lib/Archive/Tar.pm#L1501

which I could see as correct because of the way perl's unicode support
(fails to) deal with filenames.

Tony

Not entirely correct IMO. If the intent is that filenames be encoded to
UTF-8, this will fail to encode downgraded names with non-ascii characters.

-Dan

See bug​: https://rt.perl.org/Public/Bug/Display.html?id=130831