From 393717dbb42943d1238188e07d80610f9a7b69c3 Mon Sep 17 00:00:00 2001
From: JT <547158+jntrnr@users.noreply.github.com>
Date: Wed, 29 Mar 2023 10:23:10 +1300
Subject: [PATCH] Improve inferred record types and type compat (#8649)

# Description

This allows for type inference to infer record types in more cases. The
only time we will now fall back to `Any` is when one of the fields has a
computed value.

I also updated the type mismatch error and highlighting to be in-line
with other errors.

# User-Facing Changes

This may result in stricter type checking. Previously `{}` had the
inferred type `Any` but will now have the correct inferred type of
`Record<>`.

# Tests + Formatting

Don't forget to add tests that cover your changes.

Make sure you've run and fixed any issues with these commands:

- `cargo fmt --all -- --check` to check standard code formatting (`cargo
fmt --all` applies these changes)
- `cargo clippy --workspace -- -D warnings -D clippy::unwrap_used -A
clippy::needless_collect` to check that you're using the standard code
style
- `cargo test --workspace` to check that all tests pass

> **Note**
> from `nushell` you can also use the `toolkit` as follows
> ```bash
> use toolkit.nu # or use an `env_change` hook to activate it
automatically
> toolkit check pr
> ```

# After Submitting

If your PR had any user-facing changes, update [the
documentation](https://github.com/nushell/nushell.github.io) after the
PR is merged, if necessary. This will help us keep the docs up to date.
---
 crates/nu-parser/src/errors.rs     |  2 +-
 crates/nu-parser/src/parser.rs     | 20 ++++++++++++++++----
 crates/nu-parser/src/type_check.rs | 16 ++++++++++++++++
 src/tests/test_type_check.rs       |  5 +++++
 4 files changed, 38 insertions(+), 5 deletions(-)

diff --git a/crates/nu-parser/src/errors.rs b/crates/nu-parser/src/errors.rs
index ea67015570..37991cf614 100644
--- a/crates/nu-parser/src/errors.rs
+++ b/crates/nu-parser/src/errors.rs
@@ -307,7 +307,7 @@ pub enum ParseError {
 
     #[error("Type mismatch.")]
     #[diagnostic(code(nu::parser::type_mismatch))]
-    TypeMismatch(Type, Type, #[label("expected {0:?}, found {1:?}")] Span), // expected, found, span
+    TypeMismatch(Type, Type, #[label("expected {0}, found {1}")] Span), // expected, found, span
 
     #[error("Missing required flag.")]
     #[diagnostic(code(nu::parser::missing_required_flag))]
diff --git a/crates/nu-parser/src/parser.rs b/crates/nu-parser/src/parser.rs
index 1c8cfe8c85..1246ff9867 100644
--- a/crates/nu-parser/src/parser.rs
+++ b/crates/nu-parser/src/parser.rs
@@ -1029,7 +1029,6 @@ pub fn parse_internal_call(
                 continue;
             }
 
-            let orig_idx = spans_idx;
             let (arg, err) = parse_multispan_value(
                 working_set,
                 &spans[..end],
@@ -1040,7 +1039,6 @@ pub fn parse_internal_call(
             error = error.or(err);
 
             let arg = if !type_compatible(&positional.shape.to_type(), &arg.ty) {
-                let span = span(&spans[orig_idx..spans_idx]);
                 error = error.or_else(|| {
                     Some(ParseError::TypeMismatch(
                         positional.shape.to_type(),
@@ -1048,7 +1046,7 @@ pub fn parse_internal_call(
                         arg.span,
                     ))
                 });
-                Expression::garbage(span)
+                Expression::garbage(arg.span)
             } else {
                 arg
             };
@@ -5853,6 +5851,7 @@ pub fn parse_record(
     let mut output = vec![];
     let mut idx = 0;
 
+    let mut field_types = Some(vec![]);
     while idx < tokens.len() {
         let (field, err) = parse_value(
             working_set,
@@ -5887,6 +5886,15 @@ pub fn parse_record(
         error = error.or(err);
         idx += 1;
 
+        if let Some(field) = field.as_string() {
+            if let Some(fields) = &mut field_types {
+                fields.push((field, value.ty.clone()));
+            }
+        } else {
+            // We can't properly see all the field types
+            // so fall back to the Any type later
+            field_types = None;
+        }
         output.push((field, value));
     }
 
@@ -5894,7 +5902,11 @@ pub fn parse_record(
         Expression {
             expr: Expr::Record(output),
             span,
-            ty: Type::Any, //FIXME: but we don't know the contents of the fields, do we?
+            ty: (if let Some(fields) = field_types {
+                Type::Record(fields)
+            } else {
+                Type::Any
+            }),
             custom_completion: None,
         },
         error,
diff --git a/crates/nu-parser/src/type_check.rs b/crates/nu-parser/src/type_check.rs
index 5b51f8d636..68d47410a8 100644
--- a/crates/nu-parser/src/type_check.rs
+++ b/crates/nu-parser/src/type_check.rs
@@ -13,6 +13,22 @@ pub fn type_compatible(lhs: &Type, rhs: &Type) -> bool {
         (Type::Closure, Type::Block) => true,
         (Type::Any, _) => true,
         (_, Type::Any) => true,
+        (Type::Record(fields_lhs), Type::Record(fields_rhs)) => {
+            // Structural subtyping
+            'outer: for field_lhs in fields_lhs {
+                for field_rhs in fields_rhs {
+                    if field_lhs.0 == field_rhs.0 {
+                        if type_compatible(&field_lhs.1, &field_rhs.1) {
+                            continue 'outer;
+                        } else {
+                            return false;
+                        }
+                    }
+                }
+                return false;
+            }
+            true
+        }
         (lhs, rhs) => lhs == rhs,
     }
 }
diff --git a/src/tests/test_type_check.rs b/src/tests/test_type_check.rs
index dfac8a3e2a..73a85d3811 100644
--- a/src/tests/test_type_check.rs
+++ b/src/tests/test_type_check.rs
@@ -20,6 +20,11 @@ fn number_int() -> TestResult {
     run_test(r#"def foo [x:number] { $x }; foo 1"#, "1")
 }
 
+#[test]
+fn int_record_mismatch() -> TestResult {
+    fail_test(r#"def foo [x:int] { $x }; foo {}"#, "expected int")
+}
+
 #[test]
 fn number_float() -> TestResult {
     run_test(r#"def foo [x:number] { $x }; foo 1.4"#, "1.4")