Improve type-checking #19

InnocentZero · 2024-11-26T23:11:56Z

Adds type-checking to the following cases:

block outputs (reflect the type of the last statement, none otherwise)
parameters in def (any if not specified, else the specified type)
if else (reflect the type of the blocks if the blocks are equal, if they are unequal then forbidden, and the if branch type otherwise)

Signed-off-by: innocentzero <[email protected]>

Both of them are redundant now given that blocks return the type of the last statement Signed-off-by: innocentzero <[email protected]>

InnocentZero · 2024-11-27T16:08:27Z

Updates:

Params now has a None type, since they are not expected to have types anyways.
Block and BLOCK_TYPE are now redundant, since blocks return the type of the last statement

Signed-off-by: innocentzero <[email protected]>

kubouch · 2024-11-27T19:47:34Z

About the if-else, if they have different types, it should be allowed and the type should be oneof<a, b>. For example,

if $cond { 4 } else { 'a' }

should have type oneof<int, string>. We can start just by casting it to any, but it definitely shouldn't throw an error.

Params having None type sounds good. Block having the last type also seems good.

InnocentZero · 2024-11-27T19:50:12Z

Yeah, I realised that much later while going through docs and messing around in the shell, but that'd require some deeper changes. Anyways, I'll revert those changes in the next commit and hopefully add some more checks.

Signed-off-by: innocentzero <[email protected]>

Implements OneOf for if-else and def nodes Signed-off-by: innocentzero <[email protected]>

Signed-off-by: innocentzero <[email protected]>

InnocentZero · 2024-11-29T00:55:11Z

Updates:

Removed the if-else checks since they were basically wrong. Now it only checks for the condition returning a boolean
Added a OneOf(...) type that stores an index. We use that index in Typechecker::oneof_types to get the corresponding BTreeSet of types.
if-else return type now supports OneOf if there are more that one types being returned. Else it will return the singleton type
for loop and while loop typechecking has been added
Added an alias test because I wanted to see the behaviour of the call node
Typechecking for match node is now supported. Tests, however, suggest that it's not being parsed properly atm.

InnocentZero · 2024-11-29T00:56:06Z

@kubouch should I attempt to fix match parsing or would you like to check and merge it rn? The PR looks decently sized and implements a couple of major changes, so I'd want to get a review if possible.

kubouch · 2024-12-08T10:53:11Z

Let's not add too much stuff to it, the match parsing can be left for another PR.

The summary seems good, I'll try to take a look whenever possible. Any specific reason to use BTreeSet over a plain Vec?

InnocentZero · 2024-12-08T10:59:00Z

BTreeSet makes it easier to keep unique elements. I did not want a oneof<int, int> kind of situation.

kubouch · 2024-12-08T11:42:12Z

I see, yeah, a set makes sense in this context. Any reason for the BTreeSet over a regular HashSet?

Signed-off-by: innocentzero <[email protected]>

InnocentZero · 2024-12-08T13:47:58Z

My bad, I was working on something else that required BTreeSet and forgot that HashSet exists when I switched back to working on this.

Signed-off-by: innocentzero <[email protected]>

InnocentZero · 2024-12-08T19:08:07Z

I'm not sure what to make of the CI tests :/

EDIT: Nvm, figured out. I'd sort the output before printing it.

Signed-off-by: innocentzero <[email protected]>

tests/alias.nu

kubouch · 2024-12-15T19:25:15Z

src/snapshots/new_nu_parser__test__node_output@for_break_continue.nu.snap

-16: unknown
-17: unknown
-18: unknown
+14: oneof<(), unknown>


The type of missing if branch is nothing. So, if $spam { 1 } should have a return type of oneof<int, nothing>.

That () is from the else branch. The if branch does not have a type because continue and break are not checked/parsed properly.

Yeah, I meant the else branch. It should be nothing, not () coming from the missing else branch. Try if false { 1 } | describe in Nushell, it'll print nothing, so we should match the behavior.

src/snapshots/new_nu_parser__test__node_output@binary_ops_mismatch.nu.snap

This is used to indicate the type status in case of an error to avoid ambiguities around the usage of other types Signed-off-by: innocentzero <[email protected]>

src/snapshots/new_nu_parser__test__node_output@invalid_types.nu.snap

kubouch · 2024-12-15T21:02:57Z

src/snapshots/new_nu_parser__test__node_output@invalid_types.nu.snap

-10: unknown
-11: unknown
-12: ()
+9: list<unknown>


I think Param should also be forbidden since it won't be directly evaluated?

I was thinking we can do some better typechecking by checking each param and if it is type compatible to the argument of the function once we have the facilities for that.

kubouch · 2024-12-15T21:17:20Z

src/typechecker.rs

+            } => {
+                self.typecheck_node(range);
+
+                // We don't need to typecheck variable after this


Why not? (The comment is not very descriptive.)

My bad. Basically the variable's type is resolved when we typecheck the list. I think the comment is positioned wrong.

Not to mention, the variable's type is being set by us after checking the type of the range. We're setting the type in the for node itself, not before.

kubouch · 2024-12-15T21:24:48Z

src/snapshots/[email protected]

+Error (NodeId 26): incomplete expression
+Error (NodeId 27): incomplete expression
+Error (NodeId 28): expected: right paren ')'
+Error (NodeId 30): incomplete expression


Seems like it's cropped?

I removed and ran cargo insta, seems like the output is the same according to git.

kubouch · 2024-12-15T21:24:55Z

src/typechecker.rs

+                    self.error("Blocks in looping constructs cannot return values", block);
+                }
+
+                self.set_node_type_id(node_id, NOTHING_TYPE);


To match the current behavior of Nushell (i.e., for i in 0..8 { print $i } | describe throwing an error because for is a statement), we should probably make it into a none type.

However, in the future, we could perhaps allow for to be an expression, and you'd be able to break a value from it, e.g., let x = for i in 1..8 { if $ > 3 { break ($i * $i) } }. In that case, nothing type is appropriate for a loop without value breaks. This applies to while and loop as well.

For the second part, would it not be a problem if the for loop never evaluates with a list at runtime? Or do you plan to keep it oneof<nothing, type>?

For now I've gone with none

src/typechecker.rs

kubouch · 2024-12-15T21:32:07Z

OK, some minor comments, but otherwise looks good.

Since params are not supposed to be evaluated, their type is changed to Forbidden Signed-off-by: innocentzero <[email protected]>

Match not returning anything should be a nothing type as it is an expression. The type for a for loop is now None, which matches the current nushell implementation. Signed-off-by: innocentzero <[email protected]>

kubouch · 2024-12-16T17:32:52Z

OK, let's try it, thanks!

I think the match parsing might be borked and you did some fixes on it in your other branch, so let's roll with it.

InnocentZero added 7 commits November 27, 2024 03:59

add block output typechecking

8e42795

Signed-off-by: innocentzero <[email protected]>

add parameter typechecking

2b6b68f

Signed-off-by: innocentzero <[email protected]>

add typechecking for if else block

ace961f

Signed-off-by: innocentzero <[email protected]>

fix typechecking in If Node

9acff9b

Signed-off-by: innocentzero <[email protected]>

Add better diagnostics to if else

0433b31

Signed-off-by: innocentzero <[email protected]>

add none type for params

54aebb7

Signed-off-by: innocentzero <[email protected]>

Remove Type::Block and BLOCK_TYPE

ef7fbcb

Both of them are redundant now given that blocks return the type of the last statement Signed-off-by: innocentzero <[email protected]>

Check if condition's type

5503a46

Signed-off-by: innocentzero <[email protected]>

InnocentZero added 6 commits November 28, 2024 01:43

remove if-else branch checks

6fd3232

Signed-off-by: innocentzero <[email protected]>

add OneOf type

1f4959f

Implements OneOf for if-else and def nodes Signed-off-by: innocentzero <[email protected]>

Typecheck For loop node

0e68280

Signed-off-by: innocentzero <[email protected]>

Add while node

cde0a9f

Signed-off-by: innocentzero <[email protected]>

add aliasing test

aeec48a

Signed-off-by: innocentzero <[email protected]>

Typecheck match node

77e46cd

Signed-off-by: innocentzero <[email protected]>

Replace BTreeSet with HashSet

09886e4

Signed-off-by: innocentzero <[email protected]>

InnocentZero mentioned this pull request Dec 8, 2024

Parser improvements #24

Closed

Fix while loop typechecking

b0820fb

Signed-off-by: innocentzero <[email protected]>

Fix test output

5b9ee66

Signed-off-by: innocentzero <[email protected]>

kubouch reviewed Dec 15, 2024

View reviewed changes

tests/alias.nu Outdated Show resolved Hide resolved

kubouch reviewed Dec 15, 2024

View reviewed changes

src/snapshots/new_nu_parser__test__node_output@binary_ops_mismatch.nu.snap Outdated Show resolved Hide resolved

Add Error type

76057a0

This is used to indicate the type status in case of an error to avoid ambiguities around the usage of other types Signed-off-by: innocentzero <[email protected]>

kubouch reviewed Dec 15, 2024

View reviewed changes

src/snapshots/new_nu_parser__test__node_output@invalid_types.nu.snap Outdated Show resolved Hide resolved

kubouch reviewed Dec 15, 2024

View reviewed changes

src/typechecker.rs Outdated Show resolved Hide resolved

InnocentZero added 2 commits December 16, 2024 11:09

Forbidden type for params

095f859

Since params are not supposed to be evaluated, their type is changed to Forbidden Signed-off-by: innocentzero <[email protected]>

change for and match types

cc6a0e7

Match not returning anything should be a nothing type as it is an expression. The type for a for loop is now None, which matches the current nushell implementation. Signed-off-by: innocentzero <[email protected]>

kubouch merged commit eba4f27 into nushell:main Dec 16, 2024
4 checks passed

InnocentZero deleted the typechecking branch December 17, 2024 07:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve type-checking #19

Improve type-checking #19

InnocentZero commented Nov 26, 2024

InnocentZero commented Nov 27, 2024

kubouch commented Nov 27, 2024

InnocentZero commented Nov 27, 2024

InnocentZero commented Nov 29, 2024

InnocentZero commented Nov 29, 2024 •

edited

Loading

kubouch commented Dec 8, 2024

InnocentZero commented Dec 8, 2024

kubouch commented Dec 8, 2024

InnocentZero commented Dec 8, 2024

InnocentZero commented Dec 8, 2024 •

edited

Loading

kubouch Dec 15, 2024

InnocentZero Dec 15, 2024

kubouch Dec 15, 2024

kubouch Dec 15, 2024

InnocentZero Dec 16, 2024

kubouch Dec 15, 2024

InnocentZero Dec 16, 2024 •

edited

Loading

InnocentZero Dec 16, 2024

kubouch Dec 15, 2024

InnocentZero Dec 16, 2024 •

edited

Loading

kubouch Dec 15, 2024

InnocentZero Dec 16, 2024

InnocentZero Dec 16, 2024

kubouch commented Dec 15, 2024

kubouch commented Dec 16, 2024

Improve type-checking #19

Improve type-checking #19

Conversation

InnocentZero commented Nov 26, 2024

InnocentZero commented Nov 27, 2024

kubouch commented Nov 27, 2024

InnocentZero commented Nov 27, 2024

InnocentZero commented Nov 29, 2024

InnocentZero commented Nov 29, 2024 • edited Loading

kubouch commented Dec 8, 2024

InnocentZero commented Dec 8, 2024

kubouch commented Dec 8, 2024

InnocentZero commented Dec 8, 2024

InnocentZero commented Dec 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

InnocentZero Dec 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

InnocentZero Dec 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kubouch commented Dec 15, 2024

kubouch commented Dec 16, 2024

InnocentZero commented Nov 29, 2024 •

edited

Loading

InnocentZero commented Dec 8, 2024 •

edited

Loading

InnocentZero Dec 16, 2024 •

edited

Loading

InnocentZero Dec 16, 2024 •

edited

Loading