Parse improvements rebase

[InnocentZero]

Supersedes #24 as it had too many merge conflicts with main.

[132ikl]

Should the parser parse unit types? In nushell's current parser, it seems like units are parsed specially, but not parsed as nothing values, and I can't seem to find the code where they are parsed.

ast -f "echo hi; null"
# => ╭───┬─────────┬────────────────────┬────────────────╮
# => │ # │ content │       shape        │      span      │
# => ├───┼─────────┼────────────────────┼────────────────┤
# => │ 0 │ echo    │ shape_internalcall │ ╭───────┬───╮  │
# => │   │         │                    │ │ start │ 0 │  │
# => │   │         │                    │ │ end   │ 4 │  │
# => │   │         │                    │ ╰───────┴───╯  │
# => │ 1 │ hi      │ shape_string       │ ╭───────┬───╮  │
# => │   │         │                    │ │ start │ 5 │  │
# => │   │         │                    │ │ end   │ 7 │  │
# => │   │         │                    │ ╰───────┴───╯  │
# => │ 2 │ null    │ shape_nothing      │ ╭───────┬────╮ │
# => │   │         │                    │ │ start │ 9  │ │
# => │   │         │                    │ │ end   │ 13 │ │
# => │   │         │                    │ ╰───────┴────╯ │
# => ╰───┴─────────┴────────────────────┴────────────────╯
ast -f "echo hi; ()"
# => ╭───┬─────────┬────────────────────┬───────────────╮
# => │ # │ content │       shape        │     span      │
# => ├───┼─────────┼────────────────────┼───────────────┤
# => │ 0 │ echo    │ shape_internalcall │ ╭───────┬───╮ │
# => │   │         │                    │ │ start │ 0 │ │
# => │   │         │                    │ │ end   │ 4 │ │
# => │   │         │                    │ ╰───────┴───╯ │
# => │ 1 │ hi      │ shape_string       │ ╭───────┬───╮ │
# => │   │         │                    │ │ start │ 5 │ │
# => │   │         │                    │ │ end   │ 7 │ │
# => │   │         │                    │ ╰───────┴───╯ │
# => ╰───┴─────────┴────────────────────┴───────────────╯

[InnocentZero]

Ah, I checked it via () | describe. That output nothing. Should it not be treated as one of nothing or none? Imo introducing a third type for just this doesn't make sense to me.

[132ikl]

I think it's actually just an empty sub-expression. For example, 1 | () | describe outputs int. Also, ast -f "{;}" (empty block, with semicolon to differentiate from record) doesn't output anything, similarly to ast -f "()", so I'm guessing both of these are either not parsed as a node at all, or are pruned from the AST before being output.

[sholderbach]

Yeah we don't have that "unit type" notion from Rust. Maybe we should iterate a bit on how the empty expression is handled to make checking for it easier but as of now, I don't think it makes sense to add it to the type system.

[InnocentZero]

Yeah we don't have that "unit type" notion from Rust. Maybe we should iterate a bit on how the empty expression is handled to make checking for it easier but as of now, I don't think it makes sense to add it to the type system.

Ah, but without that it simply throws a compile error as of now.

[kubouch]

I'll review it later, but IMO empty subexpression should evaluate to nothing, same as missing else branch. Then, 'a' | () and 'a' | null are inconsistent in the current Nushell. The question also is whether to allow 'a' | () at all, since 'a' | is not allowed...

[InnocentZero]

Just to clear everyone, I've only added a new AST Node, it still evaluates to nothing. This makes it easier than having an Option<NodeId> for an expression everywhere.

[132ikl]

This makes it easier than having an Option for an expression everywhere.

I would think () should just be a subexpression block containing no statements/expressions. It shouldn't require any special handling like requiring every NodeId to be optional or a special node, it should just be a Block node with a block that contains no statements.

[InnocentZero]

I think the above should work, but I'm not really sure of when to give the type as null and when to give the type as nothing 😅

[132ikl]

If you're asking about when a node should return Type::None vs Type::Nothing, then Type::None should be used when the node is a statement and does not return any value (like let foo = 123), and Type::Nothing should be used when a node is an expression which returns a null value (like (), since () is a sub-expression and all expressions must return a value)

[kubouch]

• The alias seems good.
• while I think doesn't need the flags. With the new parser, the keywords become real parser keywords instead of keyword/command hybrids as in current Nushell. We could special-case <keyword> -h/--help to give a help message and detect special cases like def --env/--wrapped, but otherwise I think we don't need to track flags on keywords.
• And I think we can just ban (), see the comment.

Sorry for the merge conflicts, you can add these to the updated parser without too much effort, just replace self.next() with self.tokens.advance().

[InnocentZero]

@kubouch have a look. Should be good to go ig.

[kubouch]

Some small details, looks good otherwise.

[InnocentZero]

@kubouch apologies for the delay. Should be good now.

[kubouch]

OK, thanks!