From c4e868f6af9cc9bcba4a673186298cc2a2621772 Mon Sep 17 00:00:00 2001 From: Matthew Healy Date: Fri, 26 May 2023 10:21:04 +0200 Subject: [PATCH] Fix panic on duplicated top-level idents in record destructuring 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 | 53 +++++++++++++++++++ src/error.rs | 17 ++++++ src/parser/error.rs | 7 +++ src/parser/grammar.lalrpop | 6 ++- .../pass/nested_duplicated_ident.ncl | 4 ++ .../destructuring/repeated_ident.ncl | 13 ++--- .../destructuring/repeated_ident_typed.ncl | 11 ++++ tests/integration/main.rs | 23 ++++++-- .../record_destructuring_duplicate_ident.ncl | 4 ++ ...ord_destructuring_duplicate_ident.ncl.snap | 13 +++++ 10 files changed, 136 insertions(+), 15 deletions(-) create mode 100644 tests/integration/destructuring/pass/nested_duplicated_ident.ncl create mode 100644 tests/integration/destructuring/repeated_ident_typed.ncl create mode 100644 tests/snapshot/inputs/errors/record_destructuring_duplicate_ident.ncl create mode 100644 tests/snapshot/snapshots/snapshot__error_stderr_record_destructuring_duplicate_ident.ncl.snap 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 + +