diff --git a/src/destructuring.rs b/src/destructuring.rs index a5cf280a06..61aef29d1c 100644 --- a/src/destructuring.rs +++ b/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(); diff --git a/src/error.rs b/src/error.rs index 7372329a9a..e2f32e4637 100644 --- a/src/error.rs +++ b/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 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] diff --git a/src/parser/error.rs b/src/parser/error.rs index 09ffb011aa..051f5a6556 100644 --- a/src/parser/error.rs +++ b/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: diff --git a/src/parser/grammar.lalrpop b/src/parser/grammar.lalrpop index 0c34a0dbdd..c41ccac791 100644 --- a/src/parser/grammar.lalrpop +++ b/src/parser/grammar.lalrpop @@ -476,7 +476,7 @@ Pattern: FieldPattern = { // A full pattern at the left-hand side of a destructuring let. RecordPattern: RecordPattern = { - "{" ",")*> "}" => { + "{" ",")*> "}" =>? { 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) }, }; diff --git a/tests/integration/destructuring/pass/nested_duplicated_ident.ncl b/tests/integration/destructuring/pass/nested_duplicated_ident.ncl new file mode 100644 index 0000000000..34816f77f3 --- /dev/null +++ b/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 \ No newline at end of file diff --git a/tests/integration/destructuring/repeated_ident.ncl b/tests/integration/destructuring/repeated_ident.ncl index 17eca95f37..95555ac46a 100644 --- a/tests/integration/destructuring/repeated_ident.ncl +++ b/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" } \ No newline at end of file diff --git a/tests/integration/destructuring/repeated_ident_typed.ncl b/tests/integration/destructuring/repeated_ident_typed.ncl new file mode 100644 index 0000000000..31d441d7c5 --- /dev/null +++ b/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 +): _ diff --git a/tests/integration/main.rs b/tests/integration/main.rs index cec03a8b0e..41f088e735 100644 --- a/tests/integration/main.rs +++ b/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 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(), diff --git a/tests/snapshot/inputs/errors/record_destructuring_duplicate_ident.ncl b/tests/snapshot/inputs/errors/record_destructuring_duplicate_ident.ncl new file mode 100644 index 0000000000..28a98337f0 --- /dev/null +++ b/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" } \ No newline at end of file diff --git a/tests/snapshot/snapshots/snapshot__error_stderr_record_destructuring_duplicate_ident.ncl.snap b/tests/snapshot/snapshots/snapshot__error_stderr_record_destructuring_duplicate_ident.ncl.snap new file mode 100644 index 0000000000..c9da903384 --- /dev/null +++ b/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 + +