Repository: PyCQA/pyflakes
Pull request #72 (2016-06-11 17:48:51)

Check for duplicate dictionary keys

geokala: Is there anything else I can do to improve this and help get it to the point where it can be merged?
jayvdb: My guess is we've caught everything.
Ill do some testing on it today.
sigmavirus24: So I did some light testing on this today and it seems to work just fine. I'm in favor of merging it and will do so tonight if no one objects.

I tested against openstack/nova to see if there was a performance difference between the two versions (as the project is very large) and there was nothing statistically significant in the difference between 1.2.3 on PyPI and what's here in the repository.
jayvdb: Sorry I havent finished testing this, yet.

So far I have only found one significant problem.

It is reporting the line number of the beginning of the fict for all instances of the repeated key.

```bash
cpython$ pyflakes . 2>&1 | grep 'dictionary key'
./Lib/mimetypes.py:402: dictionary key .cdf repeated with different values
./Lib/mimetypes.py:402: dictionary key .xls repeated with different values
./Lib/encodings/mac_arabic.py:441: dictionary key 32 repeated with different values
./Lib/encodings/mac_arabic.py:441: dictionary key 33 repeated with different values
./Lib/encodings/mac_arabic.py:441: dictionary key 34 repeated with different values
./Lib/encodings/mac_arabic.py:441: dictionary key 35 repeated with different values
./Lib/encodings/mac_arabic.py:441: dictionary key 36 repeated with different values
./Lib/encodings/mac_arabic.py:441: dictionary key 38 repeated with different values
./Lib/encodings/mac_arabic.py:441: dictionary key 39 repeated with different values
./Lib/encodings/mac_arabic.py:441: dictionary key 40 repeated with different values
./Lib/encodings/mac_arabic.py:441: dictionary key 41 repeated with different values
./Lib/encodings/mac_arabic.py:441: dictionary key 42 repeated with different values
./Lib/encodings/mac_arabic.py:441: dictionary key 43 repeated with different values
./Lib/encodings/mac_arabic.py:441: dictionary key 45 repeated with different values
./Lib/encodings/mac_arabic.py:441: dictionary key 46 repeated with different values
./Lib/encodings/mac_arabic.py:441: dictionary key 47 repeated with different values
./Lib/encodings/mac_arabic.py:441: dictionary key 58 repeated with different values
./Lib/encodings/mac_arabic.py:441: dictionary key 60 repeated with different values
./Lib/encodings/mac_arabic.py:441: dictionary key 61 repeated with different values
./Lib/encodings/mac_arabic.py:441: dictionary key 62 repeated with different values
./Lib/encodings/mac_arabic.py:441: dictionary key 91 repeated with different values
./Lib/encodings/mac_arabic.py:441: dictionary key 92 repeated with different values
./Lib/encodings/mac_arabic.py:441: dictionary key 93 repeated with different values
./Lib/encodings/mac_arabic.py:441: dictionary key 94 repeated with different values
./Lib/encodings/mac_arabic.py:441: dictionary key 95 repeated with different values
./Lib/encodings/mac_arabic.py:441: dictionary key 123 repeated with different values
./Lib/encodings/mac_arabic.py:441: dictionary key 124 repeated with different values
./Lib/encodings/mac_arabic.py:441: dictionary key 125 repeated with different values
./Lib/test/test_mailbox.py:2236: dictionary key Received repeated with different values
./Lib/test/test_mailbox.py:2236: dictionary key Delivered-To repeated with different values
```

`./Lib/mimetypes.py` is fairly easy to spot
```python
    types_map = {  # line 402
        ...
        # Duplicates :(
        '.cdf'    : 'application/x-cdf',
        '.cdf'    : 'application/x-netcdf',
        ...
    }
```

`./Lib/test/test_mailbox.py` is: less easy to see:
```python
_sample_headers = {  # line 2236
    "Return-Path":"<gkj@gregorykjohnson.com>",
    "X-Original-To":"gkj+person@localhost",
    "Delivered-To":"gkj+person@localhost",
    "Received":"""from localhost (localhost [127.0.0.1])
        by andy.gregorykjohnson.com (Postfix) with ESMTP id 356ED9DD17
        for <gkj+person@localhost>; Wed, 13 Jul 2005 17:23:16 -0400 (EDT)""",
    "Delivered-To":"gkj@sundance.gregorykjohnson.com",
    "Received":"""from localhost [127.0.0.1]
        by localhost with POP3 (fetchmail-6.2.5)
        for gkj+person@localhost (single-drop); Wed, 13 Jul 2005 17:23:16 -0400 (EDT)""",
    "Received":"""from andy.gregorykjohnson.com (andy.gregorykjohnson.com [64.32.235.228])
        by sundance.gregorykjohnson.com (Postfix) with ESMTP id 5B056316746
        for <gkj@gregorykjohnson.com>; Wed, 13 Jul 2005 17:23:11 -0400 (EDT)""",
    "Received":"""by andy.gregorykjohnson.com (Postfix, from userid 1000)
        id 490CD9DD17; Wed, 13 Jul 2005 17:23:11 -0400 (EDT)""",
    "Date":"Wed, 13 Jul 2005 17:23:11 -0400",
    "From":""""Gregory K. Johnson" <gkj@gregorykjohnson.com>""",
    "To":"gkj@gregorykjohnson.com",
    "Subject":"Sample message",
    "Mime-Version":"1.0",
    "Content-Type":"""multipart/mixed; boundary="NMuMz9nt05w80d4+\"""",
    "Content-Disposition":"inline",
    "User-Agent": "Mutt/1.5.9i" }
```

`./Lib/encodings/mac_arabic.py` is even harder, as the source and error report refer to two different numbers.

```python

encoding_map = {  # line 441
    ...
    0x0020: 0x0020,     #  SPACE, left-right
    0x0020: 0x00a0,     #  SPACE, right-left
    0x0021: 0x0021,     #  EXCLAMATION MARK, left-right
    0x0021: 0x00a1,     #  EXCLAMATION MARK, right-left
    0x0022: 0x0022,     #  QUOTATION MARK, left-right
    0x0022: 0x00a2,     #  QUOTATION MARK, right-left
    0x0023: 0x0023,     #  NUMBER SIGN, left-right
    0x0023: 0x00a3,     #  NUMBER SIGN, right-left
    0x0024: 0x0024,     #  DOLLAR SIGN, left-right
    0x0024: 0x00a4,     #  DOLLAR SIGN, right-left
    0x0025: 0x0025,     #  PERCENT SIGN, left-right
    0x0026: 0x0026,     #  AMPERSAND, left-right
    0x0026: 0x00a6,     #  AMPERSAND, right-left
    0x0027: 0x0027,     #  APOSTROPHE, left-right
    0x0027: 0x00a7,     #  APOSTROPHE, right-left
    0x0028: 0x0028,     #  LEFT PARENTHESIS, left-right
    0x0028: 0x00a8,     #  LEFT PARENTHESIS, right-left
    0x0029: 0x0029,     #  RIGHT PARENTHESIS, left-right
    0x0029: 0x00a9,     #  RIGHT PARENTHESIS, right-left
    0x002a: 0x002a,     #  ASTERISK, left-right
    0x002a: 0x00aa,     #  ASTERISK, right-left
    0x002b: 0x002b,     #  PLUS SIGN, left-right
    0x002b: 0x00ab,     #  PLUS SIGN, right-left
    0x002c: 0x002c,     #  COMMA, left-right; in Arabic-script context, displayed as 0x066C ARABIC THOUSANDS SEPARATOR
    0x002d: 0x002d,     #  HYPHEN-MINUS, left-right
    0x002d: 0x00ad,     #  HYPHEN-MINUS, right-left
    0x002e: 0x002e,     #  FULL STOP, left-right; in Arabic-script context, displayed as 0x066B ARABIC DECIMAL SEPARATOR
    0x002e: 0x00ae,     #  FULL STOP, right-left
    0x002f: 0x002f,     #  SOLIDUS, left-right
    0x002f: 0x00af,     #  SOLIDUS, right-left
    ...
}
```

It should report the line number of the offending keys, string keys should be in quotes, and (less important) other keys types should report the same value as in the source .
sigmavirus24: So that's definitely plausible, I inserted a break in the code and did a little exploring:

```py
# t.py
a = {
    'a': 'b',
    'a': 'c',
}
```

```
(Pdb) l
935  	        keys = [
936  	            convert_to_value(key) for key in node.keys
937  	        ]
938  	        import pdb
939  	        pdb.set_trace()
940  ->	        for key in set(keys):
941  	            if keys.count(key) > 1:
942  	                candidate_values = []
943  	                for candidate in range(len(keys)):
944  	                    if key == keys[candidate]:
945  	                        candidate_values.append(
(Pdb) p node
<_ast.Dict object at 0x10ad3d510>
(Pdb) p dir(node)
['__class__', '__delattr__', '__dict__', '__doc__', '__format__', '__getattribute__', '__hash__', '__init__', '__module__', '__new__', '__reduce__', '__reduce_ex__', '__repr__', '__setattr__', '__sizeof__', '__str__', '__subclasshook__', '__weakref__', '_attributes', '_fields', 'col_offset', 'depth', 'keys', 'lineno', 'parent', 'values']
(Pdb) p node.lineno
1
(Pdb) p node.values
[<_ast.Str object at 0x10ad3d5d0>, <_ast.Str object at 0x10ad3d610>]
(Pdb) p node.values[0].lineno
2
(Pdb) p node.values[1].lineno
3
```

So if we don't convert everything to its value, we can probably grab the line number rather easily.

Alternatively, we can create a class that all values can be shoved into as a generic and that can keep this information. @geokala are you up for that or do you want @jayvdb or myself to take over?
geokala: Modification made to grab it (hopefully that answers your question *smiles*).

It now gives the line number as the /first/ occurrence of the key definition. It would probably be possible to make it give a later one- perhaps the last one- if I play with reversing the lists.

I think that's what you intended?

I wouldn't expect it to have a big impact on performance- it does an extra index check and then lookup on a list by key only in the case that there is a duplicate. Where no duplicate is found there should be zero impact.
sigmavirus24: So I don't think the first one is the one we want to flag/report unless we want to change the message to. I think we'd want to flag *each one* after the first.

If reporting on the first line is easiest, then I'd like to suggest we say "dictionary key <key> repeated with different values on lines ..."

Also, I just noticed that the dictionary key is using `%s` instead of `%r` for formatting. For string keys, it could be confusing if you don't see `''`s around the string. Ints will be represented the same way, and tuples should work the same way as well.
jayvdb: Ideally each reported error has the line number where the value is. Other tools use this line number to show the source line for that specific error.

I agree some continues would help reduce the indents.
jayvdb: Another strange case (on 2b86b4e):

```bash
distlib$ pyflakes . 2>&1 | grep 'dictionary key'
./tests/test_metadata.py:146: dictionary key author repeated with different values
./tests/test_metadata.py:169: dictionary key author repeated with different values
```

This error stood out because there should never be a case where only one error appears - to be repeated there must be two errors (and currently they are reported with the same line number)

Sure enough, they are two different dicts that look like this:

```python
        metadata = LegacyMetadata(mapping={'author': 'Mister Café',  # line 146
                                     'name': 'my.project',
                                     'author': 'Café Junior',
                                     'summary': 'Café torréfié',
                                     'description': 'Héhéhé',
                                     'keywords': ['café', 'coffee']
                                  })
```

Somehow it is only reporting line 146, and not reporting the dup on 148.
geokala: @jayvdb: Sorry- I must've misunderstood. Previous behaviour (up until the change I just pushed) was to raise only one complaint per set of duplicated keys with differing values (e.g. { 1: 1, 1: 2 } would raise one complaint). I've modified it now so it should behave as you expect.

@lamby: I've also put in the change you and @sigmavirus24 suggested above (well, you suggested, with addendum, etc, etc). It could actually be changed to == 1 (it can't ever be zero for the comparison it's doing), but I'm not sure if that gives any real improvement.
lamby: > but i'm not sure if that gives any real improvement.

Really? Reducing indentation makes it far, far easier to follow IMHO
jayvdb: @geokala , thanks for including reporting of all the lines with duplicates, and adding the correct line numbers.

Could you please use `%r` for the literals, which will add quotes around the strings, which is quite necessary if the string contains a space, or worse if the string contains newline characters.

I'd also really like the reported literal to be same as appears in the source, rather than key `0x0020` appearing in the report output as 32.  If I see a literal in a error report, I expect to be able to grep the source file for that literal.  The perfect UI IMO would emit `... "0x0020" (32) ...`, as then it is grepable, and two different ways of writing the same number are shown as the same number.
To do this, it looks like we need to use `dict_node.keys[i].col_offset`, and parse for the end of the key (and give up if we somehow reach the corresponding `dict_node.values[i].col_offset` ?).  Or is there another way I'm not seeing?  Maybe this should be a followup patch.

re complexity/indents/etc:

```
$ python -m mccabe pyflakes/checker.py | grep DICT
("925:1: 'Checker.DICT'", 12)
```

That makes it the fourth most complex method, and the second most complex node handler method.  Which is not representative of the actual complexity of the task.  I've noted above one way to reduce that to 10, and I suspect one or two more generators/comprehensions could be added to make the complex parts can be easy to load into the brain as a single chunk.
geokala: @lamby: Apologies, I was unclear in my meaning- I meant that changing it to == 1, rather than <=1, while filling the same function here (and functioning the same way currently), probably doesn't gain us anything (and may open up future bug possibilities if the logic is slightly changed). The original change, yes, no complaints. -smiles-

@jayvdb: Sorry, I must've missed that one- I recall intending to do it. Either way, it should be good now.
I think the reported literal issue for hex (and similar) is going to be worthy of a followup patch, yes. As you suggest, it is somewhat non-trivial (unless someone knows a way).
I'll do another pass now to try to get the complexity below 10.
geokala: @jayvdb: Change made per your suggestion of any(x) for differing_values.
Complexity now 10.

I'm not sure if it can be reasonably reduced in complexity now (in a way that doesn't sacrifice readability), but that may be down to my skill level and/or perceptions.
jayvdb: Take a look at https://github.com/jayvdb/pyflakes/commit/3b29b50638d9b77372727160b5d13bcfd6420228 for some ideas on how to cut complexity to 5.
sigmavirus24: @jayvdb I don't know of a specific guideline for what the complexity of the code should be. If there is one, we should add it to Travis CI and document it for future contributors. At this point, playing complexity golf with @geokala seems rude. If you have the complexity that low in your own fork, feel free to submit it as a pull request after this one. I think @geokala has been more than patient with us here.
sigmavirus24: Since we're just golfing over complexity, I'm going to run this over some more code-bases today and if all seems good, I'm going to merge this.

If there are reviews other than complexity golfing, please do leave them. I think this pull request has lived for quite long enough. Other style nits should be submitted by people after merge.
geokala: @sigmavirus24 Sounds fair. I think @jayvdb's changes were quite elegant (I should've guessed collections would have something to simplify what I was trying to do there), but if you're happy running over this and then determining complexity intent later that's cool.

Also, yes, I do think determining a complexity guideline and testing for it (though maybe with an easy and obvious way to add exceptions) might be a good idea. I'm fairly sure flake8 supports this natively (as I think I actually turned it on at the start of a project in a previous workplace).

Either way, if there are any other issues, I'm happy to address them!
jayvdb: My review comments are not just style nits.  They are also performance improvements.
sigmavirus24: @jayvdb there is one review comment here unrelated to complexity that @geokala has kindly addressed. Unless there are more comments that are not being displayed by GitHub, all your existing comments relate to complexity.
sigmavirus24: Also @geokala if you want to adopt @jayvdb's changes, go ahead. IMO, you don't have to keep playing complexity golf though. You were asked to make it no higher than 10 and did so.
geokala: I'm happy to adopt them, though it'll probably be late tonight before I get to them.
jayvdb: Ian, I think you have misread my comment earlier about '10' and if you look briefly at my diff you will see it is performance improvements implementing the additional generators/comprehensions I had indicated earlier would help get the complexity below 10, down to a reasonable level. The current version is twice reimplementing a Counter, using list scans. It is best practise when doing code reviews to not worry about those kind of review comments until the patch is functionally ok. I put a diff up because that was easier than trying to explain how to fix it, which would have been a longer and more aggravating process for everyone involved, as it involved rearranging the code significantly. But the code isnt mine, and I wouldntvwant 
jayvdb: While looking at pyflakes output, it occurred to me that this feature is basically identical to the existing `RedefinedWhileUnused` which emits `"filename.py:875: redefinition of unused 'xxx' from line 863` , except it is a prior dict key value that is unused rather than a variable value.

If there are any complaints after this is release that this is another case of `pyflakes` violating its contract with users and producing an error on valid code, `RedefinedWhileUnused` is a simple way to demonstrate that there is precedent for it.

Also, if we wanted to only emit only one error line for a pair of repeated keys, using the same suffix of `RedefinedWhileUnused` being `... from line x` referring at the prior & different use of the key would be the way to do it.  Note: I'm not asking for, or advocating for, it to be modified; im just saying that would be reasonable IMO.
The current approach of emitting two messages for a pair of repeated keys is a bit verbose, but it is my preference as it works better with the `flake8 --show-source` as both lines with the different values are shown.
geokala: Hmm... collections has no Counter in 2.6... @jayvdb, do we have a good alternative other than using a backport?
jayvdb: Sure.  We only need a dict with the value incrementing, as we dont need any of the fancy Counter features.
Best to build it as a fallback class for when the real Counter doesnt exist, but it could also be a normal (default)dict incrementing values using a for loop.
geokala: @jayvdb: Okay, I've put in a counter method and just used it, rather than putting extra complexity in to determine whether to use it.

@sigmavirus24 (copying you on this if you want to give it another look and/or if you're both happy to merge?)

Patch:
diff --git a/pyflakes/checker.py b/pyflakes/checker.py
index 9f354fd..68b4e8f 100644
--- a/pyflakes/checker.py
+++ b/pyflakes/checker.py
@@ -81,6 +81,17 @@ class _FieldsOrder(dict):
         return fields
 
 
+def counter(items):
+    """
+    Simplest required implementation of collections.Counter. Required as 2.6
+    does not have Counter in collections.
+    """
+    results = {}
+    for item in items:
+        results[item] = results.get(item, 0) + 1
+    return results
+
+
 def iter_child_nodes(node, omit=None, _fields_order=_FieldsOrder()):
     """
     Yield all direct child nodes of *node*, that is, all fields that
@@ -97,6 +108,33 @@ def iter_child_nodes(node, omit=None, _fields_order=_FieldsOrder()):
                 yield item
 
 
+def convert_to_value(item):
+    if isinstance(item, ast.Str):
+        return item.s
+    elif hasattr(ast, 'Bytes') and isinstance(item, ast.Bytes):
+        return item.s
+    elif isinstance(item, ast.Tuple):
+        return tuple(convert_to_value(i) for i in item.elts)
+    elif isinstance(item, ast.Num):
+        return item.n
+    elif isinstance(item, ast.Name):
+        result = VariableKey(item=item)
+        constants_lookup = {
+            'True': True,
+            'False': False,
+            'None': None,
+        }
+        return constants_lookup.get(
+            result.name,
+            result,
+        )
+    elif (not PY33) and isinstance(item, ast.NameConstant):
+        # None, True, False are nameconstants in python3, but names in 2
+        return item.value
+    else:
+        return UnhandledKeyType()
+
+
 class Binding(object):
     """
     Represents the binding of a value to a name.
@@ -133,6 +171,31 @@ class Definition(Binding):
     """
 
 
+class UnhandledKeyType(object):
+    """
+    A dictionary key of a type that we cannot or do not check for duplicates.
+    """
+
+
+class VariableKey(object):
+    """
+    A dictionary key which is a variable.
+
+    @ivar item: The variable AST object.
+    """
+    def __init__(self, item):
+        self.name = item.id
+
+    def __eq__(self, compare):
+        return (
+            compare.__class__ == self.__class__
+            and compare.name == self.name
+        )
+
+    def __hash__(self):
+        return hash(self.name)
+
+
 class Importation(Definition):
     """
     A binding created by an import statement.
@@ -855,7 +918,7 @@ class Checker(object):
     PASS = ignore
 
     # "expr" type nodes
-    BOOLOP = BINOP = UNARYOP = IFEXP = DICT = SET = \
+    BOOLOP = BINOP = UNARYOP = IFEXP = SET = \
         COMPARE = CALL = REPR = ATTRIBUTE = SUBSCRIPT = \
         STARRED = NAMECONSTANT = handleChildren
 
@@ -876,6 +939,42 @@ class Checker(object):
     # additional node types
     COMPREHENSION = KEYWORD = FORMATTEDVALUE = handleChildren
 
+    def DICT(self, node):
+        # Complain if there are duplicate keys with different values
+        # If they have the same value it's not going to cause potentially
+        # unexpected behaviour so we'll not complain.
+        keys = [
+            convert_to_value(key) for key in node.keys
+        ]
+
+        key_counts = counter(keys)
+        duplicate_keys = [
+            key for key, count in key_counts.items()
+            if count > 1
+        ]
+
+        for key in duplicate_keys:
+            key_indices = [i for i, i_key in enumerate(keys) if i_key == key]
+
+            values = counter(
+                convert_to_value(node.values[index])
+                for index in key_indices
+            )
+            if any(count == 1 for value, count in values.items()):
+                for key_index in key_indices:
+                    key_node = node.keys[key_index]
+                    if isinstance(key, VariableKey):
+                        self.report(messages.MultiValueRepeatedKeyVariable,
+                                    key_node,
+                                    key.name)
+                    else:
+                        self.report(
+                            messages.MultiValueRepeatedKeyLiteral,
+                            key_node,
+                            key,
+                        )
+        self.handleChildren(node)
+
     def ASSERT(self, node):
         if isinstance(node.test, ast.Tuple) and node.test.elts != []:
             self.report(messages.AssertTuple, node)
diff --git a/pyflakes/messages.py b/pyflakes/messages.py
index 05db5bf..58bb6a2 100644
--- a/pyflakes/messages.py
+++ b/pyflakes/messages.py
@@ -116,6 +116,22 @@ class DuplicateArgument(Message):
         self.message_args = (name,)
 
 
+class MultiValueRepeatedKeyLiteral(Message):
+    message = 'dictionary key %r repeated with different values'
+
+    def __init__(self, filename, loc, key):
+        Message.__init__(self, filename, loc)
+        self.message_args = (key,)
+
+
+class MultiValueRepeatedKeyVariable(Message):
+    message = 'dictionary key variable %s repeated with different values'
+
+    def __init__(self, filename, loc, key):
+        Message.__init__(self, filename, loc)
+        self.message_args = (key,)
+
+
 class LateFutureImport(Message):
     message = 'from __future__ imports must occur at the beginning of the file'
 


Test patch:
diff --git a/pyflakes/test/test_dict.py b/pyflakes/test/test_dict.py
new file mode 100644
index 0000000..628ec0c
--- /dev/null
+++ b/pyflakes/test/test_dict.py
@@ -0,0 +1,217 @@
+"""
+Tests for dict duplicate keys Pyflakes behavior.
+"""
+
+from sys import version_info
+
+from pyflakes import messages as m
+from pyflakes.test.harness import TestCase, skipIf
+
+
+class Test(TestCase):
+
+    def test_duplicate_keys(self):
+        self.flakes(
+            "{'yes': 1, 'yes': 2}",
+            m.MultiValueRepeatedKeyLiteral,
+            m.MultiValueRepeatedKeyLiteral,
+        )
+
+    @skipIf(version_info < (3,),
+            "bytes and strings with same 'value' are not equal in python3")
+    @skipIf(version_info[0:2] == (3, 2),
+            "python3.2 does not allow u"" literal string definition")
+    def test_duplicate_keys_bytes_vs_unicode_py3(self):
+        self.flakes("{b'a': 1, u'a': 2}")
+
+    @skipIf(version_info < (3,),
+            "bytes and strings with same 'value' are not equal in python3")
+    @skipIf(version_info[0:2] == (3, 2),
+            "python3.2 does not allow u"" literal string definition")
+    def test_duplicate_values_bytes_vs_unicode_py3(self):
+        self.flakes(
+            "{1: b'a', 1: u'a'}",
+            m.MultiValueRepeatedKeyLiteral,
+            m.MultiValueRepeatedKeyLiteral,
+        )
+
+    @skipIf(version_info >= (3,),
+            "bytes and strings with same 'value' are equal in python2")
+    def test_duplicate_keys_bytes_vs_unicode_py2(self):
+        self.flakes(
+            "{b'a': 1, u'a': 2}",
+            m.MultiValueRepeatedKeyLiteral,
+            m.MultiValueRepeatedKeyLiteral,
+        )
+
+    @skipIf(version_info >= (3,),
+            "bytes and strings with same 'value' are equal in python2")
+    def test_duplicate_values_bytes_vs_unicode_py2(self):
+        self.flakes("{1: b'a', 1: u'a'}")
+
+    def test_multiple_duplicate_keys(self):
+        self.flakes(
+            "{'yes': 1, 'yes': 2, 'no': 2, 'no': 3}",
+            m.MultiValueRepeatedKeyLiteral,
+            m.MultiValueRepeatedKeyLiteral,
+            m.MultiValueRepeatedKeyLiteral,
+            m.MultiValueRepeatedKeyLiteral,
+        )
+
+    def test_duplicate_keys_in_function(self):
+        self.flakes(
+            '''
+            def f(thing):
+                pass
+            f({'yes': 1, 'yes': 2})
+            ''',
+            m.MultiValueRepeatedKeyLiteral,
+            m.MultiValueRepeatedKeyLiteral,
+        )
+
+    def test_duplicate_keys_in_lambda(self):
+        self.flakes(
+            "lambda x: {(0,1): 1, (0,1): 2}",
+            m.MultiValueRepeatedKeyLiteral,
+            m.MultiValueRepeatedKeyLiteral,
+        )
+
+    def test_duplicate_keys_tuples(self):
+        self.flakes(
+            "{(0,1): 1, (0,1): 2}",
+            m.MultiValueRepeatedKeyLiteral,
+            m.MultiValueRepeatedKeyLiteral,
+        )
+
+    def test_duplicate_keys_tuples_int_and_float(self):
+        self.flakes(
+            "{(0,1): 1, (0,1.0): 2}",
+            m.MultiValueRepeatedKeyLiteral,
+            m.MultiValueRepeatedKeyLiteral,
+        )
+
+    def test_duplicate_keys_ints(self):
+        self.flakes(
+            "{1: 1, 1: 2}",
+            m.MultiValueRepeatedKeyLiteral,
+            m.MultiValueRepeatedKeyLiteral,
+        )
+
+    def test_duplicate_keys_bools(self):
+        self.flakes(
+            "{True: 1, True: 2}",
+            m.MultiValueRepeatedKeyLiteral,
+            m.MultiValueRepeatedKeyLiteral,
+        )
+
+    def test_duplicate_keys_bools_false(self):
+        # Needed to ensure 2.x correctly coerces these from variables
+        self.flakes(
+            "{False: 1, False: 2}",
+            m.MultiValueRepeatedKeyLiteral,
+            m.MultiValueRepeatedKeyLiteral,
+        )
+
+    def test_duplicate_keys_none(self):
+        self.flakes(
+            "{None: 1, None: 2}",
+            m.MultiValueRepeatedKeyLiteral,
+            m.MultiValueRepeatedKeyLiteral,
+        )
+
+    def test_duplicate_variable_keys(self):
+        self.flakes(
+            '''
+            a = 1
+            {a: 1, a: 2}
+            ''',
+            m.MultiValueRepeatedKeyVariable,
+            m.MultiValueRepeatedKeyVariable,
+        )
+
+    def test_duplicate_variable_values(self):
+        self.flakes(
+            '''
+            a = 1
+            b = 2
+            {1: a, 1: b}
+            ''',
+            m.MultiValueRepeatedKeyLiteral,
+            m.MultiValueRepeatedKeyLiteral,
+        )
+
+    def test_duplicate_variable_values_same_value(self):
+        # Current behaviour is not to look up variable values. This is to
+        # confirm that.
+        self.flakes(
+            '''
+            a = 1
+            b = 1
+            {1: a, 1: b}
+            ''',
+            m.MultiValueRepeatedKeyLiteral,
+            m.MultiValueRepeatedKeyLiteral,
+        )
+
+    def test_duplicate_key_float_and_int(self):
+        """
+        These do look like different values, but when it comes to their use as
+        keys, they compare as equal and so are actually duplicates.
+        The literal dict {1: 1, 1.0: 1} actually becomes {1.0: 1}.
+        """
+        self.flakes(
+            '''
+            {1: 1, 1.0: 2}
+            ''',
+            m.MultiValueRepeatedKeyLiteral,
+            m.MultiValueRepeatedKeyLiteral,
+        )
+
+    def test_no_duplicate_key_error_same_value(self):
+        self.flakes('''
+        {'yes': 1, 'yes': 1}
+        ''')
+
+    def test_no_duplicate_key_errors(self):
+        self.flakes('''
+        {'yes': 1, 'no': 2}
+        ''')
+
+    def test_no_duplicate_keys_tuples_same_first_element(self):
+        self.flakes("{(0,1): 1, (0,2): 1}")
+
+    def test_no_duplicate_key_errors_func_call(self):
+        self.flakes('''
+        def test(thing):
+            pass
+        test({True: 1, None: 2, False: 1})
+        ''')
+
+    def test_no_duplicate_key_errors_bool_or_none(self):
+        self.flakes("{True: 1, None: 2, False: 1}")
+
+    def test_no_duplicate_key_errors_ints(self):
+        self.flakes('''
+        {1: 1, 2: 1}
+        ''')
+
+    def test_no_duplicate_key_errors_vars(self):
+        self.flakes('''
+        test = 'yes'
+        rest = 'yes'
+        {test: 1, rest: 2}
+        ''')
+
+    def test_no_duplicate_key_errors_tuples(self):
+        self.flakes('''
+        {(0,1): 1, (0,2): 1}
+        ''')
+
+    def test_no_duplicate_key_errors_instance_attributes(self):
+        self.flakes('''
+        class Test():
+            pass
+        f = Test()
+        f.a = 1
+        {f.a: 1, f.a: 1}
+        ''')