allow lists to have type annotations by 1Kinoti · Pull Request #8270 · nushell/nushell

1Kinoti · 2023-03-01T01:39:34Z

Description

this pr allows list type parameters to have type annotations,

example

def nums [nums: list<int>] { $nums }

unterminated annotations raise an error

Error: nu::parser::unterminated_list_type_annotation (link)

  × Unterminated list type annotation.
   ╭─[entry #42:1:1]
 1 │ def test [list: list<int] {}
   ·                 ───┬──
   ·                    ╰── use `>` to terminate it
   ╰────

type annotations can be nested

def test [list: list<list<int>>] {}

empty type annotations are allowed

def test [list: list<>] {} # this is the same as `list: list`

the original format is also allowed

def test [list: list] {}

'would be nice' polishes

the error message could be better. i am not sure if the error message i used are the best
we should be able to parse such a line, for better error reporting

def test [list: list< int>] {}
                ─────┬────
                      ╰── spaces are not allowed in type annotations

codecov · 2023-03-01T02:44:39Z

Codecov Report

Merging #8270 (5978c8b) into main (d614188) will increase coverage by 0.02%.
The diff coverage is 100.00%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #8270      +/-   ##
==========================================
+ Coverage   68.18%   68.21%   +0.02%     
==========================================
  Files         620      620              
  Lines       99306    99345      +39     
==========================================
+ Hits        67708    67764      +56     
+ Misses      31598    31581      -17

Impacted Files	Coverage Δ
crates/nu-parser/src/parser.rs	`82.76% <100.00%> (+0.31%)`	⬆️
src/test_bins.rs	`96.88% <0.00%> (+0.44%)`	⬆️
crates/nu-parser/src/lite_parser.rs	`69.34% <0.00%> (+2.16%)`	⬆️

baehyunsol · 2023-03-01T05:54:57Z

Does it throw an error when [1, 2, true] is given when list<int> is expected?

1Kinoti · 2023-03-01T09:11:21Z

yes it does. unfortunately,, it currently only underlines the first list element that does not match the specified type

def test [list: list<int>] {};
test [1 true 5]
        ─┬─
         ╰── expected int

sholderbach

Thanks for bringing the type system a good bit forward! We have been missing out on that for a while now.

sholderbach · 2023-03-05T10:03:39Z

crates/nu-parser/src/parser.rs

+    if bytes.starts_with(b"list<") {
+        return parse_list_shape(working_set, bytes, span);
+    }
+


I don't particularly like the separated special case. It might be confusing when you look at the larger match below.
But I can see how it makes sense with the error return in parse_list_shape().

thanks for the review!

i had thought about using a match arm with a guard but that looked even more confusing

let result = match bytes { // ... list if list.starts_with(b"list<") => { /* ... */ } }

sholderbach · 2023-03-05T10:05:56Z

crates/nu-parser/src/parser.rs

+        return (SyntaxShape::List(Box::new(SyntaxShape::Any)), None);
+    } else if src.is_empty() {
+        // list<
+        return (
+            SyntaxShape::List(Box::new(SyntaxShape::Any)),
+            Some(unterminated_error),
+        );
+    }
+
+    let (shape, err) = parse_shape_name(working_set, src, inner_span);
+    if err.is_some() {
+        return (SyntaxShape::List(Box::new(shape)), err);
+    }
+
+    let error = if !is_terminated {
+        Some(unterminated_error)
+    } else {
+        None
+    };
+
+    (SyntaxShape::List(Box::new(shape)), error)


All those errors still return a SyntaxShape::List(). Is this desirable or should we reject bogus type names in there outright? (Haven't checked how the parse/error pair is handled upstream in this case)

Looking at the final return type of parse_shape_name(), any unknown type makes the function return type Any but with an Some(error). from the tests, i found that if the error section of the tuple is some, then the error is raised. so it seems that if Some(error) then the returned shape name doesn't matter,

i returned a list<any> just for consistency

Not a parser expert, seems to work in the tests, which is the important bit.
If anyone of the parser experts wants to weigh in, would be welcome.

sholderbach · 2023-03-05T10:06:54Z

src/tests/test_parser.rs

+fn list_type_annotations() -> TestResult {
+    let input = "def test [list: list<string>] { $list | length }; test [ 'nushell', 'nunu' ]";
+    let expected = "2";
+    run_test(input, expected)
+}


Thanks for the thorough testing!

sholderbach · 2023-03-05T10:08:06Z

crates/nu-command/tests/commands/def.rs

+fn def_with_specified_list_type_nested() {
+    let actual = nu!(
+        cwd: ".", pipeline(
+        r#"
+        def test-command [ foo: list<list<any>> ] {}
+        "#
+    ));
+
+    assert!(actual.err.is_empty());
+}


Are those tests redundant to the tests in src/tests/test_parser.rs or does it catch another potential area of issues?

before this pr there was this test, this is what i replaced.

i will remove them

Yeah I saw that we already had one there. I checked the src/tests/test_parser.rs tests and they also check the whole nu binary so there is no issue that it is just a unit test in parser land.

sholderbach · 2023-03-05T10:09:46Z

crates/nu-command/tests/commands/def.rs

+fn def_with_specified_list_type_unterminated() {
+    let actual = nu!(
+        cwd: ".", pipeline(
+        r#"
+        def test-command [ foo: list<any ] {}
+        "#
+    ));


Might be worth adding a test with a bogus spec like list<foo> or invalid separators like list<foo; bar>,list<int, string>.

amtoine · 2023-03-05T10:15:05Z

amazing ❤️

i do even think this PR will address the second part of #8109 😮

the first part of list type annotations do not allow default values and specifying generic type #8109, a.k.a. default list values, should have been addressed in fixes a def parsing bug with a default list #8096
the second part of list type annotations do not allow default values and specifying generic type #8109 was about generic type annotations of lists 🎉

1Kinoti · 2023-03-05T14:55:40Z

one thing i not sure about is if the error messages are okay, i couldn't come up with anything better

sholderbach

Thanks for the quick turnaround!

sholderbach · 2023-03-05T18:01:24Z

src/tests/test_parser.rs

+#[test]
+fn list_type_annotations_nested_with_unknown_type() -> TestResult {
+    let input =
+        "def test [list: list<list<str>>] { $list | length }; test [ ['nushell'], ['nunu'] ]";


Took me a second to realize that str is not string. Damn you Rust and Python brain. :D

sholderbach · 2023-03-05T18:05:56Z

src/tests/test_parser.rs

+#[test]
+fn list_type_annotations_unknown_separators() -> TestResult {
+    let input = "def test [list: list<int, string>] { $list | length }; test [2 5 4]";
+    let expected = "use `>` to terminate it";


Would be great if we could use something like ParseError::ExtraPositional or ExtraToken here to indicate that it is too much. Don't know if we get to see that from the lexer?

Don't know if the whitespace variants below have to be errors or if we could be tolerant to them?

currently, the lexer lexes that signature as two parameters, separated by a comma, so we get,
list: list<int and string> as two separate params. to really handle this, we need to have a tiny lexer that works on the signature and replaces this,

Ah that sucks a bit. I think, if we want to repeat your work for records, we would need to be able to support that, right?

you mean something like

def test [person: record<name: string, age: int>] {}

yeah, this DEFINITELY needs the smaller lexer

amtoine · 2023-03-18T14:08:31Z

oooh no why did you close this? 😢

1Kinoti · 2023-03-18T14:27:01Z

currently, the lexer lexes that signature as two parameters, separated by a comma, so we get,
list: list as two separate params. to really handle this, we need to have a tiny lexer that works on the signature and replaces this,

realized that it needs more work than i thought. i will open a more refined one when i figure out if we can rewrite the lexer, or at least have another lexer specifically meant for signatures. if we implement the latter, then we can solve all signature problems and even future ones, like:

# the lexer can be used to lex assignment type annotations
let person: record<name: string, age: int> = {name: "Name", age: 25}

# can also be used to lex return values
def run [] -> record<status: string, value: any> {
   {status: Ok, value: "some value"}
}

# and of cause param type annotations
def complex_math [args: list<int>] {}

sholderbach · 2023-03-18T14:31:56Z

I think we can start off with the list signatures as a stepping stone to see if we run into particular issues in practice. I wouldn't mind having the partial solution while having the work in the lexer going on in the background. Sorry that the PR got under the wheels of all the other stuff going on. (Don't know if there were merge problems accumulating?)

this pr refines #8270 and closes #8109 # description examples: the original syntax is okay ```nu def okay [nums: list] {} # the type of list will be list<any> ``` empty annotations are allowed in any variation the last two may be caught by a future formatter, but do not affect `nu` code currently ```nu def okay [nums: list<>] {} # okay def okay [nums: list< >] {} # weird but also okay def okay [nums: list< >] {} # also weird but okay ``` types are allowed (See [notes](#notes) below) ```nu def okay [nums: list<int>] {} # `test [a b c]` will throw an error def okay [nums: list< int > {} # any amount of space within the angle brackets is okay def err [nums: list <int>] {} # this is not okay, `nums` and `<int>` will be parsed as # two separate params, ``` nested annotations are allowed in many variations ```nu def okay [items: list<list<int>>] {} def okay [items: list<list>] {} ``` any unterminated annotation is caught ```nu Error: nu::parser::unexpected_eof × Unexpected end of code. ╭─[source:1:1] 1 │ def err [nums: list<int] {} · ▲ · ╰── expected closing > ╰──── ``` unknown types are flagged ```nu Error: nu::parser::unknown_type × Unknown type. ╭─[source:1:1] 1 │ def err [nums: list<str>] {} · ─┬─ · ╰── unknown type ╰──── Error: nu::parser::unknown_type × Unknown type. ╭─[source:1:1] 1 │ def err [nums: list<int, string>] {} · ─────┬───── · ╰── unknown type ╰──── ``` # notes the error message for mismatched types in not as intuitive ```nu Error: nu::parser::parse_mismatch × Parse mismatch during operation. ╭─[source:1:1] 1 │ def err [nums: list<int>] {}; err [a b c] · ┬ · ╰── expected int ╰──── ``` it should be something like this ```nu Error: nu::parser::parse_mismatch × Parse mismatch during operation. ╭─[source:1:1] 1 │ def err [nums: list<int>] {}; err [a b c] · ──┬── · ╰── expected list<int> ╰──── ``` this is currently not implemented

feat: allow lists to have type annotations

70c12ef

1Kinoti marked this pull request as draft March 1, 2023 01:46

test: update def_* tests to reflect new changes

4bd6ae3

1Kinoti marked this pull request as ready for review March 1, 2023 02:23

fix: use get_span_contents() instead of slice indexing

f704741

1Kinoti and others added 3 commits March 3, 2023 04:15

Merge branch 'nushell:main' into main

a3b2aad

test: empty annotations

2c7523d

fix: subtle bug when nested annotation has error

df59bb1

sholderbach reviewed Mar 5, 2023

View reviewed changes

sholderbach added A:parser Issues related to parsing A:type-system Problems or features related to nushell's type system labels Mar 5, 2023

sholderbach linked an issue Mar 5, 2023 that may be closed by this pull request

list type annotations do not allow default values and specifying generic type #8109

Closed

Merge branch 'nushell:main' into main

9e87ec7

1Kinoti added 2 commits March 5, 2023 18:07

test: remove redundant tests

d91f6c1

test: add more tests

5978c8b

sholderbach reviewed Mar 5, 2023

View reviewed changes

1Kinoti closed this Mar 18, 2023

1Kinoti mentioned this pull request Mar 19, 2023

allow lists to have type annotations #8529

Merged

Conversation

1Kinoti commented Mar 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

'would be nice' polishes

Uh oh!

codecov bot commented Mar 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

baehyunsol commented Mar 1, 2023

Uh oh!

1Kinoti commented Mar 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sholderbach left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amtoine commented Mar 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

1Kinoti commented Mar 5, 2023

Uh oh!

sholderbach left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

1Kinoti Mar 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amtoine commented Mar 18, 2023

Uh oh!

1Kinoti commented Mar 18, 2023

Uh oh!

sholderbach commented Mar 18, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

1Kinoti commented Mar 1, 2023 •

edited

Loading

codecov bot commented Mar 1, 2023 •

edited

Loading

1Kinoti commented Mar 1, 2023 •

edited

Loading

amtoine commented Mar 5, 2023 •

edited

Loading

1Kinoti Mar 5, 2023 •

edited

Loading