Warning on duplicate key in literal hash definition #14736
Comments
From @epaCreated by @epamy %h = (a => 1, a => 2); The first value for key 'a' will immediately be overwritten by the - a hash is initialized with a list given directly in the program text, - two of the keys of the hash (that is, the even-numbered elements of the list) This bug report is not suggesting a warning on code like my %h = (a => 1); my %g = (%h, a => 2); but only on the case of a single literal hash initializer which (I suspect this warning would in fact find quite a lot of Perl Info |
From zefram@fysh.orgEd Avis wrote:
That's difficult to detect, due to list context. my %h = (a => foo(), a => bar()); looks like it has clashing keys, but it may not if foo() returns an -zefram |
|
The RT System itself - Status changed from 'new' to 'open' |
From @epaZefram <zefram <at> fysh.org> writes:
Yes, I agree that this case would have to be excluded from the check. (Sidetrack: it might have been a good idea for => to impose scalar context -- |
From @leonerdOn Fri, 5 Jun 2015 14:00:20 +0000 (UTC)
Sidetrack^2: I sometimes find it useful that => *doesn't* apply scalar Future->fail( "HTTP GET failed", http => $request, $response ); That said, if we're going to wish for alternate histories, then maybe BARE => SCALAR might be the spelling? -- leonerd@leonerd.org.uk |
From zefram@fysh.orgPaul "LeoNerd" Evans wrote:
Would make no difference in this case, because the RHS, $request, behaves -zefram |
From @TuxOn Fri, 5 Jun 2015 17:11:26 +0100, "Paul \"LeoNerd\" Evans"
s/sometimes/very often/ my $string = join "," => $a, $b, @foo, func (), "TAIL";
NOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOO! -- |
From @leonerdOn Fri, 5 Jun 2015 17:23:36 +0100
Huh. Ohyes, of course. In that case, ignore me :) -- leonerd@leonerd.org.uk |
From @leonerdOn Fri, 5 Jun 2015 18:32:36 +0200
What; you wanted to reserve ==> for the pipeline operator? @items ==> grep { ... } ==> map { ... } ==> my @result; perl6-style? -- leonerd@leonerd.org.uk |
From @TuxOn Fri, 5 Jun 2015 17:46:13 +0100, "Paul \"LeoNerd\" Evans"
Joke, right? I do not want => to enforce scalar on the right hand side
I'm also "doing" perl6 on an almost daily basis now, but perl5 still -- |
From moregan@stresscafe.com
A lot of work has been done on this by Kevin Ryde in Perl::Critic::Policy::ValuesAndExpressions::ProhibitDuplicateHashKeys : http://search.cpan.org/~kryde/Perl-Critic-Pulp-90/ Mike |
From @iabynOn Fri, Jun 05, 2015 at 06:28:00AM -0700, Ed Avis wrote:
The only way I can think to detect this is, at compile time where Which seems moderately expensive. Alternatively, perhaps we should enhance the HV API to provide a %h1 = %h2; could be done directly rather than pushing all %h2's keys and values onto Then with %h = (constkey, constval, ....); the (constkey, constval, ...) could be constant-folded to a single hash, -- |
From @epaI don't think the compile-time cost of checking for duplicate hash keys Constant-folding hashes does sound like a good idea, more so if it would -- |
From @demerphqOn 8 June 2015 at 11:23, Ed Avis <eda@waniasset.com> wrote:
Just curious, what should happen with code like this: my %hash= ( a=> 1, b=>2 ); my %other_hash= ( a=> 0, %hash ); would you expect a warning here? FWIW, personally I wouldn't, as this cheers, -- |
From @epademerphq <demerphq <at> gmail.com> writes:
As I mentioned in the original bug report, I do not suggest making any warning -- |
From @epaThanks for the mention of Perl::Critic::Pulp. I do use it. But nobody runs their program through perlcritic every single edit-test cycle. Although particularly heavyweight, controversial or specialized warnings are certainly better in perlcritic, I believe that checking for duplicate literal hash keys in the program is lightweight enough and universally useful enough to merit a warning in perl itself. |
From zefram@fysh.orgEd Avis via RT wrote:
There's a middle path that you seem to have overlooked: a compile-time -zefram |
From @demerphqOn 8 June 2015 at 13:34, Ed Avis <eda@waniasset.com> wrote:
Sorry I missed that point. Thanks for the explanation. This sounds Yves -- |
From @timbunceOn Mon, Jun 08, 2015 at 04:54:27AM -0700, Ed Avis via RT wrote:
[Just a data point] At $work we run perlcritic in a git commit hook via Tim. |
From @timbunceOn Mon, Jun 08, 2015 at 09:14:41AM +0100, Dave Mitchell wrote:
Couldn't we just compare the number of elements in the list with the Tim. |
From @epaTim Bunce <Tim.Bunce <at> pobox.com> writes:
That's certainly a start - it doesn't let you report exactly which hash key -- |
From @TuxOn Mon, 8 Jun 2015 06:58:10 -0600, Tim Bunce <Tim.Bunce@pobox.com>
I think perlcritic is valuable when manually run every 6 month or so There are too many edge cases broken to use it in commit hooks. A few @_{slice} != @_ local %_; results in [4 - Variables::RequireLocalizedPunctuationVars] grep { ... } @foo and action != void context grep { !defined } @_ and croak ($self->SetDiag (1004)); results in [3 - BuiltinFunctions::ProhibitVoidGrep] my ($var) inside sub { } is not the same variable $csv->callbacks (after_parse => sub { results in [3 - Variables::ProhibitReusedNames] -- |
From @epaZefram <zefram <at> fysh.org> writes:
Would that be able to warn on C<%h = (a => 1, a => 2)> but keep quiet on -- |
From zefram@fysh.orgEd Avis wrote:
Yes. It would actually be quite difficult to link the two statements in -zefram |
From @rcaputo
All the questions about edge cases imply that people expect this to be a lexically scoped feature not tightly coupled to compile time. Tying %h to a class that raises an exception on duplicates would work in all the exceptional cases. To limit the feature's scope, untie the hash after initialization. -- |
From zefram@fysh.orgRocco Caputo wrote:
No, the questions about edge cases imply that it's a semantically
That sounds messy, and would interfere with ordinary use of the hash. -zefram |
From @epaRocco Caputo <rcaputo <at> pobox.com> writes:
Speaking for myself, that is not the intention. I envisage a strictly -- |
From @jkeenanOn Mon Jun 08 08:23:28 2015, eda@waniasset.com wrote:
My sense of the discussion so far in this thread is that, in order to minimize side effects, the number of cases in which the warning would actually be generated would have to be strictly limited to the simplest cases. If that is so, then I doubt the benefit of this warning would outweigh the cost in human work-hours incurred in: * Implementing the warning in the core; I note that no patch was submitted. * More importantly, the future maintenance cost for the Porters. How much hand-holding do we have to do? |
From @epaJames E Keenan - you are right, the warning would be limited to the simplest cases (which are also those which are completely uncontroversial, IMHO). I suggest that despite the simple-mindedness of the check it would actually catch a fair few bugs in practice. This is because a programmer will often cut and paste to add a new entry to a configuration hash, and forget to change the hash key. This is just my experience from my own code and reading code written by others. Although the check is conceptually simple to explain (to reiterate: when the two duplicate keys are literally present in the source code in the same hash assignment, and when the determination of odd and even positions is also obvious at compile time), I did not have much idea how difficult it would be to implement. If straightforward, it is an easy win; otherwise it may not be worth the development effort (which in any case is decided by whoever does the work, unless others are interested in funding this proposal). Should the bug be closed as WONTFIX or WONTBOTHER, that is fine of course. If the perl5-porters would prefer not to receive feature requests which aren't backed by patches, I will stop sending them. |
From @ikegamiOn Fri, Jun 5, 2015 at 12:23 PM, Zefram <zefram@fysh.org> wrote:
Better example: system(cat => @files) |
From @epaThings like system(cat => @files) are interesting but outside the scope of this feature, since it isn't a hash assignment; there isn't the same literal key appearing twice in the sequence; and the number of elements isn't known at compile time. I would only check fixed lists of k=>v pairs, or (as a slightly enhanced version of the check) lists which contain only k=>v pairs and other hashes as %h = (%i, a => 1, b => 2, %j, a => 3) since in this case the odd-even of each position is known at compile time, even if the total number of elements isn't. |
From @rcaputo
If it were a runtime feature of list assignment to a hash, it would work in all cases, and the very specific case you'd like to happen at compile time could as an optimization. -- |
From @epaI suggest a runtime warning wouldn't really work because (unless I am %c = (a => 1); In both cases the RHS of the assignment is the list ('a',1,'a',2) at run -- |
From @rcaputo
If the feature were lexically scoped, or scope-able, one could disable the warning where the duplicates were deliberate. Or, I suppose, enable it where a problem was anticipated. That's why I mentioned lexical scoping before. -- |
From @iabynOn Thu, Jun 11, 2015 at 02:33:41AM -0700, Ed Avis via RT wrote:
I think the best approach to implementing this is to optimise assigning a
I think the ticket should be left open. I'll add constant folding hashes
Speaking purely for myself, I'm happy for people to suggest features that -- |
From zefram@fysh.orgDave Mitchell wrote:
That would miss cases such as %h = (a => $a, a => $b); which I think Ed wants to be covered. -zefram |
From @kentfredricOn 6 June 2015 at 01:28, Ed Avis <perlbug-followup@perl.org> wrote:
Question: What would happen here: my %h = ( a => 1, @list, a => 2, @list ); Given @list can be an odd sized list, which may in fact expand as: my %h = ( a => 1, b => a, 2 => b ); ( Apologies if it was covered elsewhere, I didn't see this exact concern -- *KENTNL* - https://metacpan.org/author/KENTNL |
From @epa
Sadly that would have to be excluded from the check since the There is an interesting issue here, and perhaps a case for Only cases where the number of elements in the list is known |
From @rgsOn 12 June 2015 at 15:29, Ed Avis via RT <perlbug-followup@perl.org> wrote:
I was just thinking about |
From @kentfredricOn 13 June 2015 at 01:29, Ed Avis via RT <perlbug-followup@perl.org> wrote:
Can I read that to interpret such that: ( key1 => value, key2 => value, ARRAY_OR_SUBCALL, .* ) will process key1 That is 1: for hashes that are entirely static declarations, full key analysis can -- *KENTNL* - https://metacpan.org/author/KENTNL |
From zefram@fysh.orgRafael Garcia-Suarez wrote:
That seems a perverse way of changing the value ascribed to b. -zefram |
From @epa
That's one option and the one I envisaged. Then the warning is always sound. I suppose an alternative is just to assume that any subroutine call or array included in the hash initializer will return an even number of elements (since after all, what crazy person would mix => and odd-sized arrays in a hash initializer expr?) and warn on that basis. That makes the warning technically unsound - since you *could* concoct an example program which triggers it while not having a duplicate hash key - but perhaps more useful in practice. |
From @rjbs* Kent Fredric <kentfredric@gmail.com> [2015-06-12T09:59:12]
This is getting out of hand. If this is going ot happen, it needs to be straightforward and have no false * scalar constants count as one thing If the sum of things is even, no warning. If the sum of things is odd, -- |
From @ikegamiOn Thu, Jun 11, 2015 at 3:58 PM, Rocco Caputo <rcaputo@pobox.com> wrote:
Which leads to the reason I wouldn't like it: %a = (b => @c); would be different than @d =(b => @c); |
From @epaEd Avis <eda <at> waniasset.com> writes:
Just to mention an interesting bug that arose because programmers weren't http://blog.gerv.net/2014/10/new -- |
From @AbigailOn Mon, Aug 03, 2015 at 12:12:08PM +0000, Ed Avis wrote:
Well, it's a very good thing the RHS of => is in list context, %h = (a => 1, b => 2); would be a one element hash (with key 'a', and value 2). (This is also directed to the early subthread which suggests that the In the blog post, the code would have failed if => provided scalar - => has a higher precedence than , Just changing => so it provides scalar context on its RHS only helps %h = (key => foo()); with foo() returning a multi-element list in list context. (Which On top of changing the RHS behavior (or introducing a new operator) But I've yet to encounter someone who says "well, I'm being baffled Abigail |
From @epaAbigail <abigail <at> abigail.be> writes:
Those assumptions are both wrong but I think they are the mental model of The authors of Bugzilla must be in the top 20% of Perl programmers, and they But I do agree that it is not an easy thing to solve from where Perl is now. -- |
Migrated from rt.perl.org#125332 (status was 'open')
Searchable as RT125332$
The text was updated successfully, but these errors were encountered: