Skip to content

Commit

Permalink
Prevent panic when parsing record type with field path (#1325)
Browse files Browse the repository at this point in the history
Nickel does not support record types with field paths. i.e. while this
is a valid type:

```
{ x : { y : String } }
```

this is not:

```
{ x.y : String }
```

However, at some point rather than raising the appropriate error here,
we started panicking. This commit fixes that by checking that a field's
path only contains a single element before deciding whether to allow
it in a record type.
  • Loading branch information
matthew-healy authored May 26, 2023
1 parent 9a2e2fc commit afdaff5
Show file tree
Hide file tree
Showing 3 changed files with 36 additions and 15 deletions.
17 changes: 10 additions & 7 deletions src/parser/uniterm.rs
Original file line number Diff line number Diff line change
Expand Up @@ -286,12 +286,15 @@ impl UniRecord {
}
}

/// Checks if this record qualifies as a record type. If this function returns true, then
/// `into_type_strict()` must succeed.
/// Checks if this record qualifies as a record type. If this function
/// returns true, then `into_type_strict()` must succeed.
pub fn is_record_type(&self) -> bool {
self.fields.iter().all(|field_def| {
// Warning: this pattern must stay in sync with the corresponding pattern in `into_type_strict`.
matches!(&field_def.field,
// Field paths with a depth > 1 are not supported in record types.
field_def.path.len() == 1
// Warning: this pattern must stay in sync with the
// corresponding pattern in `into_type_strict`.
&& matches!(&field_def.field,
Field {
value: None,
metadata:
Expand All @@ -313,9 +316,9 @@ impl UniRecord {
})
}

/// a plain record type, uniquely containing fields of the form `fields: Type`. Currently, this
/// doesn't support the field path syntax: `{foo.bar.baz : Type}.into_type_strict()` returns an
/// `Err`.
/// A plain record type, uniquely containing fields of the form `fields:
/// Type`. Currently, this doesn't support the field path syntax:
/// `{foo.bar.baz : Type}.into_type_strict()` returns an `Err`.
pub fn into_type_strict(self) -> Result<Types, InvalidRecordTypeError> {
fn term_to_record_rows(
id: Ident,
Expand Down
28 changes: 20 additions & 8 deletions tests/integration/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -162,6 +162,8 @@ enum ErrorExpectation {
AnyParseError,
#[serde(rename = "ParseError::DuplicateIdentInRecordPattern")]
ParseDuplicateIdentInRecordPattern { ident: String },
#[serde(rename = "ParseError::TypedFieldWithoutDefinition")]
ParseTypedFieldWithoutDefinition,
#[serde(rename = "ImportError::ParseError")]
ImportParseError,
}
Expand Down Expand Up @@ -190,17 +192,24 @@ impl PartialEq<Error> for ErrorExpectation {
Error::TypecheckError(TypecheckError::MissingDynTail(..)),
)
| (TypecheckExtraDynTail, Error::TypecheckError(TypecheckError::ExtraDynTail(..)))
| (ImportParseError, Error::ImportError(ImportError::ParseErrors(..)))
| (AnyParseError, Error::ParseErrors(..)) => true,
(ParseDuplicateIdentInRecordPattern { ident }, Error::ParseErrors(e)) => {
let first_error = e
| (ImportParseError, Error::ImportError(ImportError::ParseErrors(..))) => true,
(e, Error::ParseErrors(es)) => {
let first_error = es
.errors
.first()
.expect("Got ParserErrors without any errors");
matches!(
first_error,
ParseError::DuplicateIdentInRecordPattern { ident: id1, .. } if ident.as_str() == id1.label()
)
match (e, first_error) {
(AnyParseError, _) => true,
(
ParseDuplicateIdentInRecordPattern { ident },
ParseError::DuplicateIdentInRecordPattern { ident: ident1, .. },
) => ident.as_str() == ident1.label(),
(
ParseTypedFieldWithoutDefinition,
ParseError::TypedFieldWithoutDefinition { .. },
) => true,
_ => false,
}
}
(EvalFieldMissing { field }, Error::EvalError(EvalError::FieldMissing(ident, ..))) => {
field == ident
Expand Down Expand Up @@ -269,6 +278,9 @@ impl std::fmt::Display for ErrorExpectation {
ParseDuplicateIdentInRecordPattern { ident } => {
format!("ParseError::DuplicateIdentInRecordPattern({ident})")
}
ParseTypedFieldWithoutDefinition => {
"ParseError::TypedFieldWithoutDefinition".to_owned()
}
ImportParseError => "ImportError::ParseError".to_owned(),
EvalBlameError => "EvalError::BlameError".to_owned(),
EvalTypeError => "EvalError::TypeError".to_owned(),
Expand Down
6 changes: 6 additions & 0 deletions tests/integration/typecheck/fail/record-typed-field-path.ncl
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
# test.type = 'error'
# eval = 'typecheck'
#
# [test.metadata]
# error = 'ParseError::TypedFieldWithoutDefinition'
{ x.y : Number }

0 comments on commit afdaff5

Please sign in to comment.