Fix panic on duplicated top-level idents in record destructuring (#1324)

Previously Nickel would panic when encountering code like this:

```
let f = fun { x, x } = x in f { x = 1 }
```

This commit fixes that by checking each destructured record pattern for
duplciated identifiers at parsing time, and returning an error if any
are encountered.

Note, however, that in order to preserve backwards compatibility with
Nickel 1.0, the following code is still valid (and returns `1`):

```
let f = fun { x = { y }, z = { y } } => y
in f { x = { y = 1 }, z = {  y = 2 } }
```

src/destructuring.rs

@@ -1,9 +1,12 @@
 //! In this module, you have the main structures used in the destructuring feature of nickel.
 //! Also, there are implementation managing the generation of a contract from a pattern.
 
+use std::collections::{hash_map::Entry, HashMap};
+
 use crate::{
     identifier::Ident,
     label::Label,
+    parser::error::ParseError,
     position::{RawSpan, TermPos},
     term::{
         record::{Field, RecordAttrs, RecordData},
@@ -37,6 +40,14 @@ pub enum Match {
     Simple(Ident, Field),
 }
 
+impl Match {
+    fn ident(&self) -> Ident {
+        match self {
+            Match::Assign(ident, ..) | Match::Simple(ident, ..) => *ident,
+        }
+    }
+}
+
 /// Last match field of a `Destruct`.
 #[derive(Debug, PartialEq, Clone)]
 pub enum LastMatch {
@@ -58,6 +69,48 @@ pub struct RecordPattern {
 }
 
 impl RecordPattern {
+    /// Check the matches for duplication, and raise an error if any occur.
+    ///
+    /// Note that for backwards-compatibility reasons this function _only_
+    /// checks top-level matches. In Nickel 1.0, this code panicked:
+    ///
+    /// ```text
+    /// let f = fun { x, x, .. } => x in f { x = 1 }
+    /// ```
+    ///
+    /// However this "works", even though the binding to `y` is duplicated.
+    ///
+    /// ```text
+    /// let f =
+    ///   fun { x = { y }, z = { y }, .. } => y
+    /// in f { x = { y = 1 }, z = { y = 2 } }
+    /// # evaluates to 1
+    /// ```
+    ///
+    /// This function aims to raise errors in the first case, but maintain the
+    /// behaviour in the second case.
+    pub fn check_matches(&self) -> Result<(), ParseError> {
+        let mut matches = HashMap::new();
+
+        for m in self.matches.iter() {
+            let binding = m.ident();
+            let label = binding.label().to_owned();
+            match matches.entry(label) {
+                Entry::Occupied(occupied_entry) => {
+                    return Err(ParseError::DuplicateIdentInRecordPattern {
+                        ident: binding,
+                        prev_ident: occupied_entry.remove_entry().1,
+                    })
+                }
+                Entry::Vacant(vacant_entry) => {
+                    vacant_entry.insert(binding);
+                }
+            }
+        }
+
+        Ok(())
+    }
+
     /// Generate the contract elaborated from this pattern.
     pub fn into_contract(self) -> LabeledType {
         let label = self.label();

src/error.rs

@@ -429,6 +429,13 @@ pub enum ParseError {
         input: String,
         pos_path_elem: TermPos,
     },
+    /// A duplicate binding was encountered in a record destructuring pattern.
+    DuplicateIdentInRecordPattern {
+        /// The duplicate identifier.
+        ident: Ident,
+        /// The previous instance of the duplicated identifier.
+        prev_ident: Ident,
+    },
 }
 
 /// An error occurring during the resolution of an import.
@@ -600,6 +607,9 @@ impl ParseError {
                     field_span,
                     annot_span,
                 },
+                InternalParseError::DuplicateIdentInRecordPattern { ident, prev_ident } => {
+                    ParseError::DuplicateIdentInRecordPattern { ident, prev_ident }
+                }
             },
         }
     }
@@ -1677,6 +1687,13 @@ impl IntoDiagnostics<FileId> for ParseError {
                         "Only identifiers and simple string literals are allowed.".into(),
                     ])
             }
+            ParseError::DuplicateIdentInRecordPattern { ident, prev_ident } =>
+                Diagnostic::error()
+                    .with_message(format!("duplicated binding `{}` in record pattern", ident.label()))
+                    .with_labels(vec![
+                        secondary(&prev_ident.pos.unwrap()).with_message("previous binding here"),
+                        primary(&ident.pos.unwrap()).with_message("duplicated binding here"),
+                    ]),
         };
 
         vec![diagnostic]

src/parser/error.rs

@@ -36,6 +36,13 @@ pub enum ParseError {
     /// A recursive let pattern was encountered. They are not currently supported because we
     /// decided it was too involved to implement them.
     RecursiveLetPattern(RawSpan),
+    /// A duplicate binding was encountered in a record destructuring pattern.
+    DuplicateIdentInRecordPattern {
+        /// The duplicate identifier.
+        ident: Ident,
+        /// The previous instance of the duplicated identifier.
+        prev_ident: Ident,
+    },
     /// A type variable is used in ways that imply it has muiltiple different kinds.
     ///
     /// This can happen in several situations, for example:

src/parser/grammar.lalrpop

@@ -476,7 +476,7 @@ Pattern: FieldPattern = {
 
 // A full pattern at the left-hand side of a destructuring let.
 RecordPattern: RecordPattern = {
-    <start: @L> "{" <mut matches: (<Match> ",")*> <last:LastMatch?> "}" <end: @R> => {
+    <start: @L> "{" <mut matches: (<Match> ",")*> <last:LastMatch?> "}" <end: @R> =>? {
         let (open, rest) = match last {
             Some(LastMatch::Match(m)) => {
                 matches.push(*m);
@@ -487,7 +487,9 @@ RecordPattern: RecordPattern = {
         };
 
         let span = mk_span(src_id, start, end);
-        RecordPattern{ matches, open, rest, span }
+        let pattern = RecordPattern{ matches, open, rest, span };
+        pattern.check_matches()?;
+        Ok(pattern)
     },
 };
 

tests/integration/destructuring/pass/nested_duplicated_ident.ncl

@@ -0,0 +1,4 @@
+# test.type = 'pass'
+let f = fun { x = { y }, z = { y } } => y in
+let result = f { x = { y = 1 }, z = { y = 2 } } in
+result == 1

tests/integration/destructuring/repeated_ident.ncl

@@ -1,14 +1,9 @@
 # test.type = 'error'
 #
 # [test.metadata]
-# error = 'TypecheckError::MissingRow'
+# error = 'ParseError::DuplicateIdentInRecordPattern'
 #
 # [test.metadata.expectation]
-# ident = 'a'
-
-# Note: currently repeated identifiers in patterns aren't caught unless
-#       we're in typechecking mode. there's an open issue for this (#1098)
-(
-  let { a, a, .. } = { a = 1, b = 2 } in
-  a : Number
-): _
+# ident = 'duped'
+let f = fun { duped, duped, .. } => duped
+in f { duped = 1, other = "x" }

tests/integration/destructuring/repeated_ident_typed.ncl

@@ -0,0 +1,11 @@
+# test.type = 'error'
+#
+# [test.metadata]
+# error = 'ParseError::DuplicateIdentInRecordPattern'
+#
+# [test.metadata.expectation]
+# ident = 'a'
+(
+  let { a, a, .. } = { a = 1, b = 2 } in
+  a : Number
+): _

tests/integration/main.rs

@@ -1,7 +1,7 @@
 use std::{io::Cursor, thread};
 
 use nickel_lang::{
-    error::{Error, EvalError, ImportError, TypecheckError},
+    error::{Error, EvalError, ImportError, ParseError, TypecheckError},
     term::Term,
 };
 use nickel_lang_utilities::{
@@ -159,7 +159,9 @@ enum ErrorExpectation {
     #[serde(rename = "TypecheckError::MissingDynTail")]
     TypecheckMissingDynTail,
     #[serde(rename = "ParseError")]
-    ParseError,
+    AnyParseError,
+    #[serde(rename = "ParseError::DuplicateIdentInRecordPattern")]
+    ParseDuplicateIdentInRecordPattern { ident: String },
     #[serde(rename = "ImportError::ParseError")]
     ImportParseError,
 }
@@ -189,7 +191,17 @@ impl PartialEq<Error> for ErrorExpectation {
             )
             | (TypecheckExtraDynTail, Error::TypecheckError(TypecheckError::ExtraDynTail(..)))
             | (ImportParseError, Error::ImportError(ImportError::ParseErrors(..)))
-            | (ParseError, Error::ParseErrors(..)) => true,
+            | (AnyParseError, Error::ParseErrors(..)) => true,
+            (ParseDuplicateIdentInRecordPattern { ident }, Error::ParseErrors(e)) => {
+                let first_error = e
+                    .errors
+                    .first()
+                    .expect("Got ParserErrors without any errors");
+                matches!(
+                    first_error,
+                    ParseError::DuplicateIdentInRecordPattern { ident: id1, .. } if ident.as_str() == id1.label()
+                )
+            }
             (EvalFieldMissing { field }, Error::EvalError(EvalError::FieldMissing(ident, ..))) => {
                 field == ident
             }
@@ -253,7 +265,10 @@ impl std::fmt::Display for ErrorExpectation {
     fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
         use ErrorExpectation::*;
         let name = match self {
-            ParseError => "ParseError".to_owned(),
+            AnyParseError => "ParseError".to_owned(),
+            ParseDuplicateIdentInRecordPattern { ident } => {
+                format!("ParseError::DuplicateIdentInRecordPattern({ident})")
+            }
             ImportParseError => "ImportError::ParseError".to_owned(),
             EvalBlameError => "EvalError::BlameError".to_owned(),
             EvalTypeError => "EvalError::TypeError".to_owned(),

tests/snapshot/inputs/errors/record_destructuring_duplicate_ident.ncl

@@ -0,0 +1,4 @@
+# capture = 'stderr'
+# command = []
+let f = fun { one, two, one } => { one, two }
+in f { one = 1, two = "2" }

tests/snapshot/snapshots/snapshot__error_stderr_record_destructuring_duplicate_ident.ncl.snap

@@ -0,0 +1,13 @@
+---
+source: tests/snapshot/main.rs
+expression: err
+---
+error: duplicated binding `one` in record pattern
+  ┌─ [INPUTS_PATH]/errors/record_destructuring_duplicate_ident.ncl:3:25
+  │
+3 │ let f = fun { one, two, one } => { one, two }
+  │               ---       ^^^ duplicated binding here
+  │               │          
+  │               previous binding here
+
+