From 195c356f918d9d617ec8705cd459732ff1418650 Mon Sep 17 00:00:00 2001
From: Mathieu Duponchelle <mathieu@centricular.com>
Date: Sun, 20 May 2018 22:02:08 +0200
Subject: [PATCH] dict: address review comments

---
 mesonbuild/interpreterbase.py          |  3 +--
 mesonbuild/mparser.py                  | 17 +++++++++--------
 run_unittests.py                       | 11 +++++++++++
 test cases/common/199 dict/meson.build |  6 ++++++
 4 files changed, 27 insertions(+), 10 deletions(-)

diff --git a/mesonbuild/interpreterbase.py b/mesonbuild/interpreterbase.py
index 1e14829ea..6a979d180 100644
--- a/mesonbuild/interpreterbase.py
+++ b/mesonbuild/interpreterbase.py
@@ -300,8 +300,7 @@ class InterpreterBase:
 
     def evaluate_dictstatement(self, cur):
         (arguments, kwargs) = self.reduce_arguments(cur.args)
-        if len(arguments) > 0:
-            raise InvalidCode('Only key:value pairs are valid in dict construction.')
+        assert (not arguments, 'parser bug, arguments should be empty')
         return kwargs
 
     def evaluate_notstatement(self, cur):
diff --git a/mesonbuild/mparser.py b/mesonbuild/mparser.py
index fb1058c0a..18ee701e4 100644
--- a/mesonbuild/mparser.py
+++ b/mesonbuild/mparser.py
@@ -642,21 +642,22 @@ class Parser:
 
         while not isinstance(s, EmptyNode):
             potential = self.current
-            if self.accept('comma'):
-                a.commas.append(potential)
-                a.append(s)
-            elif self.accept('colon'):
+            if self.accept('colon'):
                 if not isinstance(s, StringNode):
                     raise ParseException('Key must be a string.',
                                          self.getline(), s.lineno, s.colno)
+                if s.value in a.kwargs:
+                    # + 1 to colno to point to the actual string, not the opening quote
+                    raise ParseException('Duplicate dictionary key: {}'.format(s.value),
+                                         self.getline(), s.lineno, s.colno + 1)
                 a.set_kwarg(s.value, self.statement())
                 potential = self.current
                 if not self.accept('comma'):
                     return a
                 a.commas.append(potential)
             else:
-                a.append(s)
-                return a
+                raise ParseException('Only key:value pairs are valid in dict construction.',
+                    self.getline(), s.lineno, s.colno)
             s = self.statement()
         return a
 
@@ -671,7 +672,7 @@ class Parser:
                 a.append(s)
             elif self.accept('colon'):
                 if not isinstance(s, IdNode):
-                    raise ParseException('Keyword argument must be a plain identifier.',
+                    raise ParseException('Dictionary key must be a plain identifier.',
                                          self.getline(), s.lineno, s.colno)
                 a.set_kwarg(s.value, self.statement())
                 potential = self.current
@@ -708,7 +709,7 @@ class Parser:
         varname = t
         varnames = [t]
 
-        if (self.accept('comma')):
+        if self.accept('comma'):
             t = self.current
             self.expect('id')
             varnames.append(t)
diff --git a/run_unittests.py b/run_unittests.py
index 3608d3e58..162bfcf9d 100755
--- a/run_unittests.py
+++ b/run_unittests.py
@@ -582,6 +582,7 @@ class BasePlatformTests(unittest.TestCase):
         if inprocess:
             try:
                 (returncode, out, err) = run_configure(self.meson_mainfile, self.meson_args + args + extra_args)
+                print (out)
                 if 'MESON_SKIP_TEST' in out:
                     raise unittest.SkipTest('Project requested skipping.')
                 if returncode != 0:
@@ -2328,6 +2329,16 @@ class FailureTests(BasePlatformTests):
         self.assertEqual(cm.exception.returncode, 2)
         self.wipe()
 
+    def test_dict_requires_key_value_pairs(self):
+        self.assertMesonRaises("dict = {3, 'foo': 'bar'}",
+            'Only key:value pairs are valid in dict construction.')
+        self.assertMesonRaises("{'foo': 'bar', 3}",
+            'Only key:value pairs are valid in dict construction.')
+
+    def test_dict_forbids_duplicate_keys(self):
+        self.assertMesonRaises("dict = {'a': 41, 'a': 42}",
+            'Duplicate dictionary key: a.*')
+
 
 class WindowsTests(BasePlatformTests):
     '''
diff --git a/test cases/common/199 dict/meson.build b/test cases/common/199 dict/meson.build
index a1debf1e3..18bb2e9b9 100644
--- a/test cases/common/199 dict/meson.build	
+++ b/test cases/common/199 dict/meson.build	
@@ -10,3 +10,9 @@ foreach key, value : dict
   test('dict test @0@'.format(key), exe,
     args : [dict[key], value])
 endforeach
+
+empty_dict = {}
+
+foreach key, value : empty_dict
+  assert(false, 'This dict should be empty')
+endforeach