noverify icon indicating copy to clipboard operation
noverify copied to clipboard

Report non-literal key duplicates in dupArrayKeys diagnostic

Open quasilyte opened this issue 5 years ago • 4 comments

Code Example

// 1. Constants
$example1 = [
  C1 => 1, 
  C2 => 2, 
  C1 => 3, // Duplicate key C1
];
$example2 = [
  T::C1 => 1, 
  T::C2 => 2, 
  T::C1 => 3, // Duplicate key T1::C1
];

// 2. Expressions
$example3 = [
  $k[0] => 1,
  $k[1] => 2,
  $k[0] => 3, // Duplicate key $k[0]
];
// Note: if $k is a class that implements ArrayAccess, it's
// safer to ignore it or check whether element fetching
// is a side-effect-free method.

// 3. Optional: anything that satisfies sideEffectFree()
$example3 = [
  'a' . $s => 1,
  'b' . $s => 2,
  'a' . $s => 3, // Duplicate key 'a'.$s
];

Currently, dupArrayKeys only handles simple literal keys (ints and strings).

Note that only int and string typed keys are valid in PHP (all other types are converted to them). If an illegal-typed value is used, dupArrayKeys should ignore it as it should be caught by another diagnostic. That being said, it's debatable whether the code below should be reported as having duplicated keys:

$example4 = [
  new T() => 1,
  new T() => 2,
];

Syntactically it does contain duplicated new T() key but in practice we should warn about illegal key type.

Actual Behavior

No warnings.

Expected Behavior

Duplicate array key warning.

quasilyte avatar Dec 12 '19 09:12 quasilyte

Only constant expressions should be counted though as there exist things like this (in theory):

<?php
[ rand() => 1, rand() => 2 ];

This code is not great but it usually does not contain duplicate keys :)

YuriyNasretdinov avatar Dec 12 '19 09:12 YuriyNasretdinov

We have sideEffectFree(expr) that can reject non-pure calls and other expressions that can have side effects. If it's not good enough, we can adjust it to be good enough.

quasilyte avatar Dec 12 '19 09:12 quasilyte

Maybe we can use nodeSet for this, but maybe it's still easier to support a few types of nodes:

  • expr.ConstFetch
  • expr.ClassConstFetch
  • scalar.Lnumber
  • scalar.Dnumber
  • scalar.String

(Maybe something else?)

quasilyte avatar May 21 '20 15:05 quasilyte

Guys, sorry about this spam, I thought that testing pull requests in my fork won't mention original repo issue. :smile: Please don't look at this commits, they are in dead branches, just look at PR I will have my pull request now opened here

hazzus avatar Jun 03 '20 15:06 hazzus