Skip to content

Commit

Permalink
Disallow duplicated columns in table literals (nushell#10875)
Browse files Browse the repository at this point in the history
# Description
Pretty much all operations/commands in Nushell assume that the column
names/keys in a record and thus also in a table (which consists of a
list of records) are unique.
Access through a string-like cell path should refer to a single column
or key/value pair and our output through `table` will only show the last
mention of a repeated column name.

```nu
[[a a]; [1 2]]
╭─#─┬─a─╮
│ 0 │ 2 │
╰───┴───╯
```

While the record parsing already either errors with the
`ShellError::ColumnDefinedTwice` or silently overwrites the first
occurence with the second occurence, the table literal syntax `[[header
columns]; [val1 val2]]` currently still allowed the creation of tables
(and internally records with more than one entry with the same name.

This is not only confusing, but also breaks some assumptions around how
we can efficiently perform operations or in the past lead to outright
bugs (e.g. nushell#8431 fixed by nushell#8446).

This PR proposes to make this an error.
After this change another hole which allowed the construction of records
with non-unique column names will be plugged.

## Parts
- Fix `SE::ColumnDefinedTwice` error code
- Remove previous tests permitting duplicate columns
- Deny duplicate column in table literal eval
- Deny duplicate column in const eval
- Deny duplicate column in `from nuon`

# User-Facing Changes
`[[a a]; [1 2]]` will now return an error:

```
Error: nu::shell::column_defined_twice

  × Record field or table column used twice
   ╭─[entry nushell#2:1:1]
 1 │ [[a a]; [1 2]]
   ·   ┬ ┬
   ·   │ ╰── field redefined here
   ·   ╰── field first defined here
   ╰────
```

this may under rare circumstances block code from evaluating.

Furthermore this makes some NUON files invalid if they previously
contained tables with repeated column names.

# Tests + Formatting
Added tests for each of the different evaluation paths that materialize
tables.
  • Loading branch information
sholderbach authored and dmatos2012 committed Feb 20, 2024
1 parent 688c7e4 commit 514b9ed
Show file tree
Hide file tree
Showing 8 changed files with 63 additions and 34 deletions.
15 changes: 11 additions & 4 deletions crates/nu-command/src/formats/from/nuon.rs
Original file line number Diff line number Diff line change
Expand Up @@ -361,13 +361,13 @@ fn convert_to_value(
"subexpressions not supported in nuon".into(),
expr.span,
)),
Expr::Table(headers, cells) => {
Expr::Table(mut headers, cells) => {
let mut cols = vec![];

let mut output = vec![];

for key in headers {
let key_str = match key.expr {
for key in headers.iter_mut() {
let key_str = match &mut key.expr {
Expr::String(key_str) => key_str,
_ => {
return Err(ShellError::OutsideSpannedLabeledError(
Expand All @@ -379,7 +379,14 @@ fn convert_to_value(
}
};

cols.push(key_str);
if let Some(idx) = cols.iter().position(|existing| existing == key_str) {
return Err(ShellError::ColumnDefinedTwice {
second_use: key.span,
first_use: headers[idx].span,
});
} else {
cols.push(std::mem::take(key_str));
}
}

for row in cells {
Expand Down
23 changes: 0 additions & 23 deletions crates/nu-command/tests/commands/reject.rs
Original file line number Diff line number Diff line change
Expand Up @@ -106,29 +106,6 @@ fn reject_nested_field() {
assert_eq!(actual.out, "{a: {c: 5}}");
}

#[test]
fn reject_two_identical_elements() {
let actual = nu!("[[a, a]; [1, 2]] | reject a");

assert!(actual.out.contains("record 0 fields"));
}

#[test]
fn reject_large_vec_with_two_identical_elements() {
let actual = nu!("[[a, b, c, d, e, a]; [1323, 23, 45, 100, 2, 2423]] | reject a");

assert!(!actual.out.contains("1323"));
assert!(!actual.out.contains("2423"));
assert!(actual.out.contains('b'));
assert!(actual.out.contains('c'));
assert!(actual.out.contains('d'));
assert!(actual.out.contains('e'));
assert!(actual.out.contains("23"));
assert!(actual.out.contains("45"));
assert!(actual.out.contains("100"));
assert!(actual.out.contains('2'));
}

#[test]
fn reject_optional_column() {
let actual = nu!("{} | reject foo? | to nuon");
Expand Down
12 changes: 12 additions & 0 deletions crates/nu-command/tests/format_conversions/nuon.rs
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,18 @@ fn to_nuon_table() {
assert_eq!(actual.out, "true");
}

#[test]
fn from_nuon_illegal_table() {
let actual = nu!(pipeline(
r#"
"[[repeated repeated]; [abc, xyz], [def, ijk]]"
| from nuon
"#
));

assert!(actual.err.contains("column_defined_twice"));
}

#[test]
fn to_nuon_bool() {
let actual = nu!(pipeline(
Expand Down
13 changes: 12 additions & 1 deletion crates/nu-engine/src/eval.rs
Original file line number Diff line number Diff line change
Expand Up @@ -571,7 +571,18 @@ pub fn eval_expression(
Expr::Table(headers, vals) => {
let mut output_headers = vec![];
for expr in headers {
output_headers.push(eval_expression(engine_state, stack, expr)?.as_string()?);
let header = eval_expression(engine_state, stack, expr)?.as_string()?;
if let Some(idx) = output_headers
.iter()
.position(|existing| existing == &header)
{
return Err(ShellError::ColumnDefinedTwice {
second_use: expr.span,
first_use: headers[idx].span,
});
} else {
output_headers.push(header);
}
}

let mut output_rows = vec![];
Expand Down
16 changes: 12 additions & 4 deletions crates/nu-protocol/src/eval_const.rs
Original file line number Diff line number Diff line change
Expand Up @@ -303,10 +303,18 @@ pub fn eval_constant(
Expr::Table(headers, vals) => {
let mut output_headers = vec![];
for expr in headers {
output_headers.push(value_as_string(
eval_constant(working_set, expr)?,
expr.span,
)?);
let header = value_as_string(eval_constant(working_set, expr)?, expr.span)?;
if let Some(idx) = output_headers
.iter()
.position(|existing| existing == &header)
{
return Err(ShellError::ColumnDefinedTwice {
second_use: expr.span,
first_use: headers[idx].span,
});
} else {
output_headers.push(header);
}
}

let mut output_rows = vec![];
Expand Down
4 changes: 2 additions & 2 deletions crates/nu-protocol/src/shell_error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -597,8 +597,8 @@ pub enum ShellError {
/// ## Resolution
///
/// Check the record to ensure you aren't reusing the same field name
#[error("Record field used twice")]
#[diagnostic(code(nu::shell::not_a_list))]
#[error("Record field or table column used twice")]
#[diagnostic(code(nu::shell::column_defined_twice))]
ColumnDefinedTwice {
#[label = "field redefined here"]
second_use: Span,
Expand Down
5 changes: 5 additions & 0 deletions src/tests/test_table_operations.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,10 @@
use crate::tests::{fail_test, run_test, TestResult};

#[test]
fn illegal_column_duplication() -> TestResult {
fail_test("[[lang, lang]; [nu, 100]]", "column_defined_twice")
}

#[test]
fn cell_path_subexpr1() -> TestResult {
run_test("([[lang, gems]; [nu, 100]]).lang | get 0", "nu")
Expand Down
9 changes: 9 additions & 0 deletions tests/const_/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,15 @@ fn const_table() {
assert_eq!(actual.out, "table<a: int, b: int, c: int>");
}

#[test]
fn const_invalid_table() {
let inp = &["const x = [[a b a]; [10 20 30] [100 200 300]]"];

let actual = nu!(&inp.join("; "));

assert!(actual.err.contains("column_defined_twice"));
}

#[test]
fn const_string() {
let inp = &[r#"const x = "abc""#, "$x"];
Expand Down

0 comments on commit 514b9ed

Please sign in to comment.