Fix panic on duplicated top-level idents in record destructuring

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
+
+