Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

List assignment with undef and re-used variable regression #17816

Closed
Smylers opened this issue May 29, 2020 · 4 comments
Closed

List assignment with undef and re-used variable regression #17816

Smylers opened this issue May 29, 2020 · 4 comments

Comments

@Smylers
Copy link
Contributor

Smylers commented May 29, 2020

The return value of a list assignment involving both undef and a variable that appears on both sides of the assignment changed somewhere between perl 5.22 and 5.30.

This emits different output on v5.22.1 and v5.30.0:

my $part = 'zing';
my @both = (($part, undef) = (boo => $part));
say @both;

On 5.22, @both ends up containing ('boo', 'zing'),
where as on 5.30 it contains ('boo', 'boo').
The 5.22 behaviour is what I'd logically expect.

The bug doesn't appear if undef is replaced with an (otherwise unused) variable:

my $part = 'zing';
my @both = (($part, (my $ignored)) = (boo => $part));
say @both;

Nor if a separate scalar variable is assigned to, rather than overwriting the one being evaluated:

my $part = 'zing';
my @both = (my ($another, undef) = (boo => $part));
say @both;

Nor if the variable is used as part of an expression rather than evaluated directly, such as by stringifying it:

my $part = 'zing';
my @both = (($part, undef) = (boo => "$part"));
say @both;

Wrapping it in a do block isn't sufficient, though; this still exhibits the bug:

my $part = 'zing';
my @both = (($part, undef) = (boo => do { $part }));
say @both;

For anybody wondering why I was doing anything as convoluted as this (or why it would crop up in real-world code), the context is an optional initial positional argument to a sub:

classmethod BUILDARGS
(
  $id_type,
  $id = (($id_type, undef) = (old_id => $id_type))[1],
)

A class used to have a constructor which took an ID:

my $record = SomeProject::Record->new(123);

Then a new ID system was introduced, and the class was altered to accept either an old or a new ID:

my $record = SomeProject::Record->new(old_id => 123);
my $record = SomeProject::Record->new(new_id => 789);

For backwards compatibility with existing uses of the class, a single argument to ->new should be interpreted as an old-style ID; effectively the equivalent of:

unshift @_, 'old_id' if @_ == 1;

It uses subroutine signatures (they happen to be Function::Parameters, but a quick check shows it's the same with Perl's signatures feature), so there's nowhere handy to put such @_ tweaking; and when only one argument is supplied, it has to be the second parameter that's optional, not the first.

So in the case where only one argument, an old-style ID, is provided:

  • $id_type first has the ID assigned to it.

  • $id has no argument, so its default expression is evaluated. This expression needs to return the ID that has already been assigned to $id_type, and also overwrite $id_type to contain 'old_id'.

  • Using list assignment allows for both setting $id_type and assigning its value elsewhere. We can't assign to $id in the list, because the value of the entire expression will be assigned to $id. So instead assign to undef (so the list has the correct number of items in it), then use (...)[1] to obtain the value from the list.

And this was working.

(A nicer syntax for specifying, ‘if this sub has an odd number of arguments then treat the first one as being named argument foo’ would of course also be nice. But since the change in behaviour doesn't involve signatures, that's a separate matter.)



Flags:
category=core
severity=medium

Site configuration information for perl 5.30.0:

Configured by Ubuntu at Fri Mar 6 21:15:57 UTC 2020.

Summary of my perl5 (revision 5 version 30 subversion 0) configuration:

Platform:
osname=linux
osvers=4.19.0
archname=x86_64-linux-gnu-thread-multi
uname='linux localhost 4.19.0 #1 smp debian 4.19.0 x86_64 gnulinux '
config_args='-Dmksymlinks -Dusethreads -Duselargefiles -Dcc=x86_64-linux-gnu-gcc -Dcpp=x86_64-linux-gnu-cpp -Dld=x86_64-linux-gnu-gcc -Dccflags=-DDEBIAN -Wdate-time -D_FORTIFY_SOURCE=2 -g -O2 -fdebug-prefix-map=/build/perl-hOEDTI/perl-5.30.0=. -fstack-protector-strong -Wformat -Werror=format-security -Dldflags= -Wl,-Bsymbolic-functions -Wl,-z,relro -Dlddlflags=-shared -Wl,-Bsymbolic-functions -Wl,-z,relro -Dcccdlflags=-fPIC -Darchname=x86_64-linux-gnu -Dprefix=/usr -Dprivlib=/usr/share/perl/5.30 -Darchlib=/usr/lib/x86_64-linux-gnu/perl/5.30 -Dvendorprefix=/usr -Dvendorlib=/usr/share/perl5 -Dvendorarch=/usr/lib/x86_64-linux-gnu/perl5/5.30 -Dsiteprefix=/usr/local -Dsitelib=/usr/local/share/perl/5.30.0 -Dsitearch=/usr/local/lib/x86_64-linux-gnu/perl/5.30.0 -Dman1dir=/usr/share/man/man1 -Dman3dir=/usr/share/man/man3 -Dsiteman1dir=/usr/local/man/man1 -Dsiteman3dir=/usr/local/man/man3
-Duse64bitint -Dman1ext=1 -Dman3ext=3perl -Dpager=/usr/bin/sensible-pager -Uafs -Ud_csh -Ud_ualarm -Uusesfio -Uusenm -Ui_libutil -Ui_xlocale -Uversiononly -DDEBUGGING=-g -Doptimize=-O2 -dEs -Duseshrplib -Dlibperl=libperl.so.5.30.0'
hint=recommended
useposix=true
d_sigaction=define
useithreads=define
usemultiplicity=define
use64bitint=define
use64bitall=define
uselongdouble=undef
usemymalloc=n
default_inc_excludes_dot=define
bincompat5005=undef
Compiler:
cc='x86_64-linux-gnu-gcc'
ccflags ='-D_REENTRANT -D_GNU_SOURCE -DDEBIAN -fwrapv -fno-strict-aliasing -pipe -I/usr/local/include -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64'
optimize='-O2 -g'
cppflags='-D_REENTRANT -D_GNU_SOURCE -DDEBIAN -fwrapv -fno-strict-aliasing -pipe -I/usr/local/include'
ccversion=''
gccversion='9.2.1 20200304'
gccosandvers=''
intsize=4
longsize=8
ptrsize=8
doublesize=8
byteorder=12345678
doublekind=3
d_longlong=define
longlongsize=8
d_longdbl=define
longdblsize=16
longdblkind=3
ivtype='long'
ivsize=8
nvtype='double'
nvsize=8
Off_t='off_t'
lseeksize=8
alignbytes=8
prototype=define
Linker and Libraries:
ld='x86_64-linux-gnu-gcc'
ldflags =' -fstack-protector-strong -L/usr/local/lib'
libpth=/usr/local/lib /usr/lib/gcc/x86_64-linux-gnu/9/include-fixed /usr/include/x86_64-linux-gnu /usr/lib /lib/x86_64-linux-gnu /lib/../lib /usr/lib/x86_64-linux-gnu /usr/lib/../lib /lib
libs=-lgdbm -lgdbm_compat -ldb -ldl -lm -lpthread -lc -lcrypt
perllibs=-ldl -lm -lpthread -lc -lcrypt
libc=libc-2.31.so
so=so
useshrplib=true
libperl=libperl.so.5.30
gnulibc_version='2.31'
Dynamic Linking:
dlsrc=dl_dlopen.xs
dlext=so
d_dlsymun=undef
ccdlflags='-Wl,-E'
cccdlflags='-fPIC'
lddlflags='-shared -L/usr/local/lib -fstack-protector-strong'

Locally applied patches:
DEBPKG:debian/cpan_definstalldirs - Provide a sensible INSTALLDIRS default for modules installed from CPAN.
DEBPKG:debian/db_file_ver - https://bugs.debian.org/340047 Remove overly restrictive DB_File version check.
DEBPKG:debian/doc_info - Replace generic man(1) instructions with Debian-specific information.
DEBPKG:debian/enc2xs_inc - https://bugs.debian.org/290336 Tweak enc2xs to follow symlinks and ignore missing @inc directories.
DEBPKG:debian/errno_ver - https://bugs.debian.org/343351 Remove Errno version check due to upgrade problems with long-running processes.
DEBPKG:debian/libperl_embed_doc - https://bugs.debian.org/186778 Note that libperl-dev package is required for embedded linking
DEBPKG:fixes/respect_umask - Respect umask during installation
DEBPKG:debian/writable_site_dirs - Set umask approproately for site install directories
DEBPKG:debian/extutils_set_libperl_path - EU:MM: set location of libperl.a under /usr/lib
DEBPKG:debian/no_packlist_perllocal - Don't install .packlist or perllocal.pod for perl or vendor
DEBPKG:debian/fakeroot - Postpone LD_LIBRARY_PATH evaluation to the binary targets.
DEBPKG:debian/instmodsh_doc - Debian policy doesn't install .packlist files for core or vendor.
DEBPKG:debian/ld_run_path - Remove standard libs from LD_RUN_PATH as per Debian policy.
DEBPKG:debian/libnet_config_path - Set location of libnet.cfg to /etc/perl/Net as /usr may not be writable.
DEBPKG:debian/perlivp - https://bugs.debian.org/510895 Make perlivp skip include directories in /usr/local
DEBPKG:debian/squelch-locale-warnings - https://bugs.debian.org/508764 Squelch locale warnings in Debian package maintainer scripts
DEBPKG:debian/patchlevel - https://bugs.debian.org/567489 List packaged patches for 5.30.0-9build1 in patchlevel.h
DEBPKG:fixes/document_makemaker_ccflags - https://bugs.debian.org/628522 [rt.cpan.org #68613] Document that CCFLAGS should include $Config{ccflags}
DEBPKG:debian/find_html2text - https://bugs.debian.org/640479 Configure CPAN::Distribution with correct name of html2text
DEBPKG:debian/perl5db-x-terminal-emulator.patch - https://bugs.debian.org/668490 Invoke x-terminal-emulator rather than xterm in perl5db.pl
DEBPKG:debian/cpan-missing-site-dirs - https://bugs.debian.org/688842 Fix CPAN::FirstTime defaults with nonexisting site dirs if a parent is writable
DEBPKG:fixes/memoize_storable_nstore - [rt.cpan.org #77790] https://bugs.debian.org/587650 Memoize::Storable: respect 'nstore' option not respected
DEBPKG:debian/makemaker-pasthru - https://bugs.debian.org/758471 Pass LD settings through to subdirectories
DEBPKG:debian/makemaker-manext - https://bugs.debian.org/247370 Make EU::MakeMaker honour MANnEXT settings in generated manpage headers
DEBPKG:debian/kfreebsd-softupdates - https://bugs.debian.org/796798 Work around Debian Bug#796798
DEBPKG:fixes/autodie-scope - https://bugs.debian.org/798096 Fix a scoping issue with "no autodie" and the "system" sub
DEBPKG:fixes/memoize-pod - [rt.cpan.org #89441] Fix POD errors in Memoize
DEBPKG:debian/hurd-softupdates - https://bugs.debian.org/822735 Fix t/op/stat.t failures on hurd
DEBPKG:fixes/math_complex_doc_great_circle - https://bugs.debian.org/697567 [rt.cpan.org #114104] Math::Trig: clarify definition of great_circle_midpoint
DEBPKG:fixes/math_complex_doc_see_also - https://bugs.debian.org/697568 [rt.cpan.org #114105] Math::Trig: add missing SEE ALSO
DEBPKG:fixes/math_complex_doc_angle_units - https://bugs.debian.org/731505 [rt.cpan.org #114106] Math::Trig: document angle units
DEBPKG:fixes/cpan_web_link - https://bugs.debian.org/367291 CPAN: Add link to main CPAN web site
DEBPKG:debian/hppa_op_optimize_workaround - https://bugs.debian.org/838613 Temporarily lower the optimization of op.c on hppa due to gcc-6 problems
DEBPKG:debian/installman-utf8 - https://bugs.debian.org/840211 Generate man pages with UTF-8 characters
DEBPKG:fixes/getopt-long-4 - https://bugs.debian.org/864544 [rt.cpan.org #122068] Fix issue #122068.
DEBPKG:debian/hppa_opmini_optimize_workaround - https://bugs.debian.org/869122 Lower the optimization level of opmini.c on hppa
DEBPKG:debian/sh4_op_optimize_workaround - https://bugs.debian.org/869373 Also lower the optimization level of op.c and opmini.c on sh4
DEBPKG:debian/perldoc-pager - https://bugs.debian.org/870340 [rt.cpan.org #120229] Fix perldoc terminal escapes when sensible-pager is less
DEBPKG:debian/prune_libs - https://bugs.debian.org/128355 Prune the list of libraries wanted to what we actually need.
DEBPKG:debian/mod_paths - Tweak @inc ordering for Debian
DEBPKG:debian/configure-regen - https://bugs.debian.org/762638 Regenerate Configure et al. after probe unit changes
DEBPKG:debian/deprecate-with-apt - https://bugs.debian.org/747628 Point users to Debian packages of deprecated core modules
DEBPKG:debian/disable-stack-check - https://bugs.debian.org/902779 [perl #133327] Disable debugperl stack extension checks for binary compatibility with perl
DEBPKG:fixes/eumm-usrmerge - https://bugs.debian.org/913637 Avoid mangling /bin non-perl shebangs on merged-/usr systems
DEBPKG:debian/perlbug-editor - https://bugs.debian.org/922609 Use "editor" as the default perlbug editor, as per Debian policy
DEBPKG:fixes/gid-parsing - [79e302e] https://bugs.debian.org/941985 [perl #134169] (perl #134169) mg.c reset endptr after use


@inc for perl 5.30.0:
/etc/perl
/usr/local/lib/x86_64-linux-gnu/perl/5.30.0
/usr/local/share/perl/5.30.0
/usr/lib/x86_64-linux-gnu/perl5/5.30
/usr/share/perl5
/usr/lib/x86_64-linux-gnu/perl/5.30
/usr/share/perl/5.30
/usr/local/lib/site_perl
/usr/lib/x86_64-linux-gnu/perl-base


Environment for perl 5.30.0:
HOME=/home/smylers
LANG=en_GB.utf8
LANGUAGE=en_GB:en
LC_COLLATE=C
LD_LIBRARY_PATH (unset)
LOGDIR (unset)
PATH=/home/smylers/bin:/usr/local/sbin:/usr/local/bin:/snap/bin:/sbin:/bin:/usr/sbin:/usr/bin:/usr/X11R6/bin:/usr/games
PERL_BADLANG (unset)
PERL_CPANM_OPT=--sudo --prompt
SHELL=/bin/bash

@Smylers
Copy link
Contributor Author

Smylers commented May 29, 2020

Script demonstrating the issue: list_assignment_demo.txt

On perl v5.30.0 it emits:

booboo
boozing
boozing
boozing
booboo

On v5.22.1 there aren't any booboos.

@xenu
Copy link
Member

xenu commented May 29, 2020

The behavior changed in commit 808ce55:

commit 808ce55782035154ec42358256dbec2e226977fe
Author: David Mitchell <davem@iabyn.com>
Date:   Thu Aug 13 17:00:32 2015 +0100

    Optimise 1 arg in list assign

    Avoid setting common scalar flags in these cases:

    ($x) = (...);
    (...) = ($x);

@xenu
Copy link
Member

xenu commented May 29, 2020

CC @iabyn

iabyn added a commit that referenced this issue Aug 11, 2020
GH #17816

This code:

    my $x = 1;
    print (($x, undef) = (2 => $x));

was printing "22" when it should have been printing "21".
An optimisation skips the 'common values on both sides' test
when the LHS of an assign only contains a single var; as the example
above shows, this is not sufficient.

This was broken by v5.23.1-202-g808ce55782

This commit fixes it by counting undef's on the LHS towards the var
count if they don't appear first.
@iabyn
Copy link
Contributor

iabyn commented Aug 11, 2020

now fixed in blead by v5.33.0-235-g5b354d2a8a

@iabyn iabyn closed this as completed Aug 11, 2020
steve-m-hay pushed a commit that referenced this issue Jan 6, 2021
GH #17816

This code:

    my $x = 1;
    print (($x, undef) = (2 => $x));

was printing "22" when it should have been printing "21".
An optimisation skips the 'common values on both sides' test
when the LHS of an assign only contains a single var; as the example
above shows, this is not sufficient.

This was broken by v5.23.1-202-g808ce55782

This commit fixes it by counting undef's on the LHS towards the var
count if they don't appear first.

(cherry picked from commit 5b354d2)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants