Skip to content

allow lists to have type annotations#8529

Merged
sholderbach merged 5 commits intonushell:mainfrom
1Kinoti:annotations
Mar 24, 2023
Merged

allow lists to have type annotations#8529
sholderbach merged 5 commits intonushell:mainfrom
1Kinoti:annotations

Conversation

@1Kinoti
Copy link
Copy Markdown
Contributor

@1Kinoti 1Kinoti commented Mar 19, 2023

this pr refines #8270 and closes #8109

description

examples:

the original syntax is okay

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

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 below)

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

def okay [items: list<list<int>>] {}

def okay [items: list<list>] {}

any unterminated annotation is caught

Error: nu::parser::unexpected_eof

  × Unexpected end of code.
   ╭─[source:1:1]
 1 │ def err [nums: list<int] {}
   ·                       ▲
   ·                       ╰── expected closing >
   ╰────

unknown types are flagged

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

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

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

@codecov
Copy link
Copy Markdown

codecov bot commented Mar 19, 2023

Codecov Report

Merging #8529 (3a26b69) into main (2c3aade) will increase coverage by 0.04%.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #8529      +/-   ##
==========================================
+ Coverage   68.44%   68.49%   +0.04%     
==========================================
  Files         628      628              
  Lines      101551   101631      +80     
==========================================
+ Hits        69508    69610     +102     
+ Misses      32043    32021      -22     
Impacted Files Coverage Δ
crates/nu-parser/src/lib.rs 100.00% <ø> (ø)
src/tests.rs 100.00% <ø> (ø)
crates/nu-parser/src/lex.rs 88.94% <100.00%> (+1.41%) ⬆️
crates/nu-parser/src/parser.rs 83.70% <100.00%> (+0.66%) ⬆️

... and 3 files with indirect coverage changes

Copy link
Copy Markdown
Member

@sholderbach sholderbach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! Looks like you figured a relatively straightforward patch. And great to see all the tests!

@amtoine
Copy link
Copy Markdown
Member

amtoine commented Mar 19, 2023

very nice :ok_hand

@Sygmei
Copy link
Copy Markdown
Contributor

Sygmei commented Mar 24, 2023

Will this feature be ported to record type as well ?

@1Kinoti
Copy link
Copy Markdown
Contributor Author

1Kinoti commented Mar 24, 2023

yes of course, after lists have been stabilized, then records, tables, and closures can be considered

Copy link
Copy Markdown
Member

@sholderbach sholderbach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I checked that the lexing changes don't conflict with the new match syntax. So let's merge this as the changes look good and well covered.
Thanks for tackling that @1Kinoti

@sholderbach sholderbach merged commit 8cf9bc9 into nushell:main Mar 24, 2023
@1Kinoti
Copy link
Copy Markdown
Contributor Author

1Kinoti commented Mar 24, 2023

thanks! is it okay if i now tackle records and tables?

@1Kinoti 1Kinoti deleted the annotations branch March 24, 2023 12:04
@fdncred
Copy link
Copy Markdown
Contributor

fdncred commented Mar 24, 2023

thanks! is it okay if i now tackle records and tables?

I'm anxious to see what that looks like and I'm excited to try this out more. Good work @1Kinoti!

@fdncred
Copy link
Copy Markdown
Contributor

fdncred commented Mar 24, 2023

@1Kinoti Here's something I stumbled upon while testing this.

Given this custom example:

> def foo [my_list: list<string> = ['val1' 'val2']] {
  $my_list
}
> foo
╭───┬──────╮
│ 0 │ val1 │
│ 1 │ val2 │
╰───┴──────╯
> foo [1 2 3]
╭───┬───╮
│ 0 │ 1 │
│ 1 │ 2 │
│ 2 │ 3 │
╰───┴───╯

I expected the last test to produce and error because I defined $my_list as a list<string> and I'm passing in a list<int>. Any ideas?

@amtoine
Copy link
Copy Markdown
Member

amtoine commented Mar 24, 2023

@fdncred
agree 👍

especially since

def foo [x: list<int> = [1 2 3]] { print $x }

works as expected.

@amtoine
Copy link
Copy Markdown
Member

amtoine commented Mar 24, 2023

but really love the feature 🤩
now it's the turn of records 😏

@1Kinoti
Copy link
Copy Markdown
Contributor Author

1Kinoti commented Mar 24, 2023

good catch @fdncred , let me look into it

@1Kinoti
Copy link
Copy Markdown
Contributor Author

1Kinoti commented Mar 24, 2023

i have opened #8600 that addresses a semi-related issue, but this

def foo [my_list: list<string> = ['val1' 'val2']] {
  $my_list
}
foo [1 2 3] # okay but shouldn't be

is still not flagged as an error because the parser now parsed the
[1 2 3] as a list

(foo [1 2 3) | describe # list<string>

good news is that it only happens with strings, however, bad news also
is that it only happens with strings (the type with most issues imo). i
will look into a fix for this

sophiajt pushed a commit that referenced this pull request Mar 24, 2023
# Description

@fdncred noticed an
[issue](#8529 (comment))
with list annotations that while i was trying to find a fix found
another issue.

innitially, this was accepted by the parser
```nu
def err [list: list<int> = ['a' 'b' 'c']] {}
```

but now an error is raised
```nu
Error: nu::parser::assignment_mismatch

  × Default value wrong type
   ╭─[entry #1:1:1]
 1 │ def err [list: list<int> = ['a' 'b' 'c']] {}
   ·                            ──────┬────
   ·                                   ╰── expected default value to be `list<int>`
   ╰────
```

# User-Facing Changes

none

# Tests + Formatting

done
@1Kinoti
Copy link
Copy Markdown
Contributor Author

1Kinoti commented Mar 26, 2023

this issue if still present with something like

def foo [bar: string] { $bar }
foo 69               # is okay, but shouldn't be
foo 69 | describe    # string

fdncred pushed a commit that referenced this pull request Apr 26, 2023
# Description
follow up to #8529
cleaned up version of #8892 

- the original syntax is okay
```nu
def okay [rec: record] {}
```
- you can now add type annotations for fields if you know
  them before hand
```nu
def okay [rec: record<name: string>] {}
```

- you can specify multiple fields
```nu
def okay [person: record<name: string age: int>] {}

# an optional comma is allowed
def okay [person: record<name: string, age: int>] {}
```

- if annotations are specified, any use of the command will be type
  checked against the specified type
```nu
def unwrap [result: record<ok: bool, value: any>] {}

unwrap {ok: 2, value: "value"}

# errors with

Error: nu::parser::type_mismatch

  × Type mismatch.
   ╭─[entry #4:1:1]
 1 │ unwrap {ok: 2, value: "value"}
   ·         ───────┬─────
   ·                    ╰── expected record<ok: bool, value: any>, found record<ok: int, value: string>
   ╰────
```
> here the error is in the `ok` field, since `any` is coerced into any
type
> as a result `unwrap {ok: true, value: "value"}` is okay

- the key must be a string, either quoted or unquoted
```nu
def err [rec: record<{}: list>] {}

# errors with
Error:
  × `record` type annotations key not string
   ╭─[entry #7:1:1]
 1 │ def unwrap [result: record<{}: bool, value: any>] {}
   ·                            ─┬
   ·                             ╰── must be a string
   ╰────
```

- a key doesn't have to have a type in which case it is assumed to be
`any`
```nu
def okay [person: record<name age>] {}

def okay [person: record<name: string age>] {}
```

- however, if you put a colon, you have to specify a type
```nu
def err [person: record<name: >] {}

# errors with
Error: nu::parser::parse_mismatch

  × Parse mismatch during operation.
   ╭─[entry #12:1:1]
 1 │ def unwrap [res: record<name: >] { $res }
   ·                             ┬
   ·                             ╰── expected type after colon
   ╰────
```

# User-Facing Changes
**[BREAKING CHANGES]**
- this change adds a field to `SyntaxShape::Record` so any plugins that
used it will have to update and include the field. though if you are
unsure of the type the record expects, `SyntaxShape::Record(vec![])`
will suffice
sholderbach pushed a commit that referenced this pull request Jul 7, 2023
# Description

follow up to #8529 and #8914

this works very similarly to record annotations, only difference being
that

```sh
table<name: string>
      ^^^^  ^^^^^^
      |     | 
      |     represents the type of the items in that column
      |
      represents the column name
```
more info on the syntax can be found
[here](#8914 (comment))

# User-Facing Changes

**[BREAKING CHANGE]**
this change adds a field to `SyntaxShape::Table` so any plugins that
used it will have to update and include the field. though if you are
unsure of the type the table expects, `SyntaxShape::Table(vec![])` will
suffice
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

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

5 participants