Implement array inference #524

h-michael · 2019-01-13T13:48:15Z

related #394

flodiebold

You're reimplementing tuples here -- arrays should be homogenous :)

flodiebold · 2019-01-13T13:52:39Z

crates/ra_hir/src/ty.rs

@@ -216,6 +216,9 @@ pub enum Ty {
    /// A tuple type.  For example, `(i32, bool)`.
    Tuple(Arc<[Ty]>),

+    /// A array type.  For example, `[i32]`.


we already have Slice above. The difference between Array and Slice would be that Array has a known length (which we can leave out for now since we don't care about it), but then your example is wrong ;) Also, an array should only have one inner type -- all elements of the array need to have the same type.

Also can you move this up to be next to Slice?

flodiebold · 2019-01-13T13:54:50Z

crates/ra_hir/src/ty.rs

@@ -1048,6 +1061,14 @@ impl<'a, D: HirDatabase> InferenceContext<'a, D> {

                Ty::Tuple(Arc::from(ty_vec))
            }
+            Expr::Array { exprs } => {
+                let mut ty_vec = Vec::with_capacity(exprs.len());
+                for arg in exprs.iter() {


we need to unify the types of all the elements here.

h-michael · 2019-01-13T15:20:56Z

@flodiebold Thanks. How is this?
But I also think that there may be better way of unifying.

h-michael · 2019-01-13T15:22:35Z

crates/ra_hir/src/ty.rs

    /// The pointee of an array slice.  Written as `[T]`.
    Slice(Arc<Ty>),

+    // An array with the given length. Written as `[T; n]`.
+    Array(Arc<Ty>),


Should we define Array(Ty, ty::Const)?

we don't currently have a ty::Const (the commented-out lines are copied from rustc) :) And for our current use cases, we don't really need the array size -- if there was a mismatch in sizes somewhere, I think we would probably want to continue type inference anyway to still be able to provide completions etc.

flodiebold · 2019-01-13T15:26:57Z

crates/ra_hir/src/ty.rs

    /// The pointee of an array slice.  Written as `[T]`.
    Slice(Arc<Ty>),

+    // An array with the given length. Written as `[T; n]`.
+    Array(Arc<Ty>),


we don't currently have a ty::Const (the commented-out lines are copied from rustc) :) And for our current use cases, we don't really need the array size -- if there was a mismatch in sizes somewhere, I think we would probably want to continue type inference anyway to still be able to provide completions etc.

flodiebold · 2019-01-13T15:29:58Z

crates/ra_hir/src/ty.rs

                }

                Ty::Tuple(Arc::from(ty_vec))
            }
+            Expr::Array { exprs } => {
+                let mut ty = Ty::Unknown;


I think there's a nicer way to do this: create a new type variable here (you could just write self.insert_type_vars(Ty::Unknown)), and then pass that as the expected type to each of the sub expressions.

As a bonus, you can actually use the expected type you currently have. So instead of starting with Ty::Unknown, you can look into expected.ty and see if it's a slice or array type. If it is, you can use the element type from that.

flodiebold · 2019-01-13T15:30:53Z

crates/ra_hir/src/ty.rs

+                    match infered {
+                        Ty::Unknown => (),
+                        _ => ty = infered,
+                    }


you're not actually making sure all the elements have the same type anywhere here. The best way to do that would be to pass the element type as expected type (so Expectation::has_type(ty)).

flodiebold · 2019-01-13T15:32:08Z

crates/ra_hir/src/ty/tests.rs

+    let d = [y];
+    let e = [d, d];
+    let f = [e, e];
+


here's an expression that checks whether the element types are actually unified:

[y, 1, 2, 3]

the 1, 2 and 3 should be inferred to isize, and I think with your current code they won't.

you could also add the case where the array is empty:

let x: [u8; 0] = [];

we should still infer the element type on the right side. I think this should already work. This would also test array type references, which you implemented but didn't use in this test (the [u8; 0] part).

h-michael

It increases the inferring accuracy than before.

h-michael · 2019-01-13T16:53:16Z

crates/ra_hir/src/ty/tests/data/array.txt

+[226; 227) 'a': [&str]
+[229; 234) '["b"]': [[&str]]
+[230; 233) '"b"': [&str]
+[245; 246) 'x': [[u8]]


I think this should be 'x': [u8].
But I don't know why is results above.

It's because you're passing the expected type for the slice (which is [u8]) as the expected type for the elements. You need to extract the u8 from the expected type and pass that down.

h-michael · 2019-01-13T16:59:25Z

crates/ra_hir/src/ty.rs

+                let mut ty = self.insert_type_vars(expected.ty.clone());
+
+                for (i, expr) in exprs.iter().enumerate() {
+                    if i == 0 {


oops, this is not good implementation.
If getting [1, y, 2, 3] (let y: u32 = 1;), it must inferes [_].
I'll fix it.

You don't need to use the inferred type from the subexpressions. If you just pass Expectation::has_type(ty), all the element expressions will be unified to the same type. No need for special casing the first element either :)

flodiebold · 2019-01-13T17:01:42Z

crates/ra_hir/src/ty.rs

+                let mut ty = self.insert_type_vars(expected.ty.clone());
+
+                for (i, expr) in exprs.iter().enumerate() {
+                    if i == 0 {


You don't need to use the inferred type from the subexpressions. If you just pass Expectation::has_type(ty), all the element expressions will be unified to the same type. No need for special casing the first element either :)

flodiebold · 2019-01-13T17:03:37Z

crates/ra_hir/src/ty.rs

                }

                Ty::Tuple(Arc::from(ty_vec))
            }
+            Expr::Array { exprs } => {
+                let mut ty = self.insert_type_vars(expected.ty.clone());


maybe let's call this elem_ty, since it should be the element type. So you can't just use expected.ty, that's the expected type for the whole slice ;) You need to check whether the expected type is Slice or Array; if so, you can take the element type from that; otherwise, you can use a new type variable for the element type (self.new_type_var())

You need to check whether the expected type is Slice or Array; if so, you can take the element type from that

How to check whether the expected type is Slice or Array?
I think that there is no information where this processing context.

I think I defined wrong ArrayExpr.
ArrayExpr must contains SEMI and Number or Ref referring to usize.
My definitions is not for Array but for Slice, I think now.
Do I have the misconception something?

How to check whether the expected type is Slice or Array?

You have the expected type in expected.ty here. You can match on that, basically:

let elem_ty = match expected.ty { Ty::Slice(inner) | Ty::Array(inner) => inner.clone(), _ => self.new_type_var() }

I think I defined wrong ArrayExpr.
ArrayExpr must contains SEMI and Number or Ref referring to usize.
My definitions is not for Array but for Slice, I think now.
Do I have the misconception something?

Both [a, b, c] and [a; 99] are array expressions resulting in an array type, not a slice, and if I'm not mistaken, they're both represented by ArrayExpr. You could add support for the second form already here as well, but it'd be fine to ignore that for now and do in in a later PR.

(You get a slice by a coercion from an array, which we don't implement yet, but it will kind of work if we pass down the expected element type like I wrote: for example, in let x: &[u8] = [1, 2, 3], the expected type is a slice of u8; then when typing the array expression, we take the expected element type, so u8, and use that for the elements. Even though the resulting type of the expression, which is an array of u8, doesn't unify with the expected type in the end.)

flodiebold · 2019-01-13T17:04:18Z

crates/ra_hir/src/ty/tests/data/array.txt

+[226; 227) 'a': [&str]
+[229; 234) '["b"]': [[&str]]
+[230; 233) '"b"': [&str]
+[245; 246) 'x': [[u8]]


It's because you're passing the expected type for the slice (which is [u8]) as the expected type for the elements. You need to extract the u8 from the expected type and pass that down.

h-michael · 2019-01-14T13:54:22Z

@flodiebold Fixed.
Thank you for your very very detailed explanation.
I understand implementation of ra_hir crate.

flodiebold

Great, I just have two small comments, sorry :)

flodiebold · 2019-01-14T20:47:55Z

crates/ra_hir/src/ty/tests.rs

+    let b = [a, a];
+    let c = [b, b];
+
+    let d = [y, 1, 2, 3]


there should be semicolons here and in the line below... it's a testament to the parser's error recovery that this still works :)

flodiebold · 2019-01-14T20:49:27Z

crates/ra_hir/src/ty.rs

+                };
+
+                for expr in exprs.iter() {
+                    elem_ty = self.infer_expr(*expr, &Expectation::has_type(elem_ty.clone()))?;


I don't think you have to assign elem_ty here. You should be able to keep using the elem_ty above:

Suggested change

elem_ty = self.infer_expr(*expr, &Expectation::has_type(elem_ty.clone()))?;

self.infer_expr(*expr, &Expectation::has_type(elem_ty.clone()))?;

(and remove the mut).

h-michael · 2019-01-15T00:30:59Z

@flodiebold Thanks! Fixed it.

flodiebold

Sorry, the latest change revealed another small problem ;) Apart from that, can you rebase on current master to get rid of the merge commit and make it mergable again?

flodiebold · 2019-01-15T19:18:18Z

crates/ra_hir/src/ty/tests/data/array.txt

+[9; 10) 'x': &str
+[18; 19) 'y': isize
+[28; 293) '{     ... []; }': ()
+[38; 39) 'a': [_]


the _ means that we have a type variable here, which shouldn't ever happen -- all type variables should be replaced by their known type or Ty::Unknown after inference. The reason that this doesn't happen here is that Ty::Array is missing in Ty::walk_mut, which I missed so far. Can you add that there (should work exactly like the slice type)?

h-michael · 2019-01-16T15:09:50Z

@flodiebold I fixed and rebased.

flodiebold · 2019-01-16T15:11:02Z

Great, thanks!

bors r+

524: Implement array inference r=flodiebold a=h-michael related #394 Co-authored-by: Hirokazu Hata <[email protected]>

bors · 2019-01-16T15:16:52Z

Build succeeded

continuous-integration/travis-ci/push

flodiebold reviewed Jan 13, 2019

View reviewed changes

h-michael force-pushed the array branch from db793cb to 5a6c038 Compare January 13, 2019 14:43

h-michael commented Jan 13, 2019

View reviewed changes

flodiebold reviewed Jan 13, 2019

View reviewed changes

h-michael force-pushed the array branch from 4fed290 to 4f34b2c Compare January 13, 2019 16:51

h-michael commented Jan 13, 2019

View reviewed changes

flodiebold reviewed Jan 13, 2019

View reviewed changes

h-michael force-pushed the array branch from 4f34b2c to c14a4e7 Compare January 14, 2019 13:52

flodiebold reviewed Jan 14, 2019

View reviewed changes

flodiebold reviewed Jan 15, 2019

View reviewed changes

h-michael added 7 commits January 16, 2019 23:22

Update ARRAY_EXPR grammar

4729a1b

Implement array inference

d665acb

Add infer array test

2a3262c

Fix Ty::Array

5d3884d

Fix infer array test

478ce1c

Remove unneeded code

5c570fa

Fix test case invalid syntax

f66ca1b

h-michael force-pushed the array branch from d8c88f3 to c9722fa Compare January 16, 2019 14:41

Add Ty::Array to walk_mut

0aedd4f

h-michael force-pushed the array branch from c9722fa to 0aedd4f Compare January 16, 2019 15:09

bors bot added a commit that referenced this pull request Jan 16, 2019

Merge #524

454cc31

524: Implement array inference r=flodiebold a=h-michael related #394 Co-authored-by: Hirokazu Hata <[email protected]>

bors bot merged commit 0aedd4f into rust-lang:master Jan 16, 2019

h-michael deleted the array branch January 16, 2019 15:17

	elem_ty = self.infer_expr(*expr, &Expectation::has_type(elem_ty.clone()))?;
	self.infer_expr(*expr, &Expectation::has_type(elem_ty.clone()))?;

Implement array inference #524

Implement array inference #524

Uh oh!

Conversation

h-michael commented Jan 13, 2019

Uh oh!

flodiebold 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!

h-michael commented Jan 13, 2019

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!

h-michael 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!

h-michael Jan 13, 2019 • 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!

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!

h-michael commented Jan 14, 2019

Uh oh!

flodiebold 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!

h-michael commented Jan 15, 2019

Uh oh!

flodiebold left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

h-michael commented Jan 16, 2019

Uh oh!

flodiebold commented Jan 16, 2019

Uh oh!

bors bot commented Jan 16, 2019

Build succeeded

Uh oh!

Uh oh!

h-michael Jan 13, 2019 •

edited

Loading