Skip to content

Fix gaps in type inference #394

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
7 tasks done
flodiebold opened this issue Dec 31, 2018 · 19 comments
Closed
7 tasks done

Fix gaps in type inference #394

flodiebold opened this issue Dec 31, 2018 · 19 comments
Labels
E-easy E-has-instructions Issue has some instructions and pointers to code to get started good first issue

Comments

@flodiebold
Copy link
Member

flodiebold commented Dec 31, 2018

This is a 'kitchen sink' issue for various small things we don't currently handle in type inference 😄

All of these involve changing infer_expr in ty.rs; some also need improvements to the AST defined in ast.rs / grammar.ron, and it may make sense to wait for hir::Expr (#386) for these.

  • handle tuple expressions (TupleExpr), e.g. (1, 2, 3)
  • handle array expressions like [1, 2, 3]
  • handle array repeat expressions like [1; 99] (same; note that we currently ignore the length of an array in type inference.)
  • handle unary - and ! at least for primitive types (see also Implement type inference for binary operators #390)
  • handle string and byte string literals
  • handle char and byte literals
  • handle int/float literals -- these are a bit more complicated, since we need to use a type variable to infer the exact type, and fall back to the default (i32 / f32?) if we can't infer another type
@flodiebold flodiebold added good first issue E-easy E-has-instructions Issue has some instructions and pointers to code to get started labels Dec 31, 2018
@gfreezy
Copy link
Contributor

gfreezy commented Jan 2, 2019

i'm interested. where should i start?

@flodiebold
Copy link
Member Author

Hi @gfreezy, I'd suggest starting with tuples. Basically you need to do the following things:

  • the AST for tuples currently doesn't provide access to the tuple elements, so you need to add that. The AST is generated from grammar.ron; there's a line there saying
        "TupleExpr": (),

you need to add something like collections: [["exprs", "Expr"]] and then run cargo gen-syntax.

  • in infer_expr there's a match arm for TupleExpr that currently always returns Ty::Unknown; you need to go through the tuple element expressions here, call infer_expr on each of them, collect the resulting inferred types into a Ty::Tuple, and return that as result.
  • in ty/tests.rs, add a test for this which has some tuple expressions. These tests compare the current inference results against files in ty/tests/data/ which are written the first time each test is run. So before the change, the file will say that the tuple expressions have inferred type [unknown]; afterwards, they should have the correct tuple types.

Once #386 is done, the big match expression in infer_expr will match on a hir::Expr instead of a ast::Expr, but apart from that, this should work the same.

@h-michael
Copy link
Contributor

Can I try this?

@flodiebold
Copy link
Member Author

@h-michael I have no idea if @gfreezy already started with this, but since they didn't write anything I'd consider it free ;) You could of course also start with array expressions or string literals.

@gfreezy
Copy link
Contributor

gfreezy commented Jan 8, 2019

i haven't started with it. @h-michael you may start with it.

@marcusklaas
Copy link
Contributor

marcusklaas commented Jan 10, 2019

I'm working on inference for primitive literals (integers, floats, bool, char, &'static str etc.)

@marcusklaas
Copy link
Contributor

Are any of you working on other types, @gfreezy @h-michael? Otherwise I can do these too.

@h-michael
Copy link
Contributor

I'm trying tuple and array now.
But if you have no to do, you can go ahead !

@marcusklaas
Copy link
Contributor

I'll leave those to you then ;-)

bors bot added a commit that referenced this issue Jan 13, 2019
520: Imprement tuple inference r=flodiebold a=h-michael

related #394

I'm sorry I'm late.

I try implementing array inference next.

Co-authored-by: Hirokazu Hata <h.hata.ai.t@gmail.com>
bors bot added a commit that referenced this issue Jan 16, 2019
524:  Implement array inference r=flodiebold a=h-michael

related #394

Co-authored-by: Hirokazu Hata <h.hata.ai.t@gmail.com>
bors bot added a commit that referenced this issue Jan 28, 2019
698: Added support for primitive types type inference with std::ops::Not r=flodiebold a=WizardOfMenlo

On the guideline of #544 , this allows for type inference for all primitive types implementing [std::ops::Not](https://doc.rust-lang.org/beta/std/ops/trait.Not.html). I think this should be relevant #394 as well?

Co-authored-by: WizardOfMenlo <giacomofenzi@outlook.com>
@simonvandel
Copy link
Contributor

It seems that array repeat expressions like [1; 99] are already implemented. Can this issue be closed? @flodiebold

@flodiebold
Copy link
Member Author

Hm. Actually, I think they're kind of working accidentally, because we just ignore the repeat number and treat them as an array expression with one element (so we handle [1; 99 as [1]). We should still add proper handling for them (i.e. give them their own Expr variant and handle that in inference).

@vipentti
Copy link
Contributor

I wonder what would be the best way to handle the array repeat expressions when the expression is a more complex expression rather than a simple usize, e.g.

const fn foo() -> usize { 6 }
let y = [0u32; 6];    
let w = [0u32; { 3 + 3 }];
let x = [0u32; foo()];

All should be inferred to be

const fn foo() -> usize { 6 }
let y: [u32; 6] = [0u32; 6];    
let w: [u32; 6] = [0u32; { 3 + 3 }];
let x: [u32; 6] = [0u32; foo()];

This seems like it would require some evaluation during inference.
I guess to start with at least inferring a simple usize expression should be relatively simple ?

Additionally, don't all arrays have a fixed length e.g. ?

let x = [1, 2, 3];
// is the same as 
let x: [i32; 3] = [1, 2, 3]

Meaning we could have a new Expr variant but a single Ty variant with a length/expression that would evaluate to a length

@flodiebold
Copy link
Member Author

@vipentti We currently completely ignore array lengths, because const evaluation would be a whole other can of worms, and they're not that interesting for our current use cases (it's unlikely that [u32; 14] will have different methods to complete than [u32; 15]). Our array type doesn't even have a stored length. So to handle array repeat expressions for now, we'd run type inference on the repeat count, but we wouldn't try to evaluate it in any way.

@vipentti
Copy link
Contributor

vipentti commented Mar 1, 2019

What kind of inference result would we like to get for

let y = [0u32; 6];    
let w = [0u32; { 3 + 3 }];

Something like

y = [u32; <inferred>?];
w = [u32; <inferred>?];

@flodiebold
Copy link
Member Author

flodiebold commented Mar 1, 2019

See here -- our array types don't contain any length. So the inferred type currently just looks like [u32] (but it's not a slice).

(We could consider changing the display pattern for array types to be something like [u32; ?] to be able to actually distinguish them from slices...)

@kjeremy
Copy link
Contributor

kjeremy commented Mar 5, 2019

See here

Broken link.

@flodiebold
Copy link
Member Author

Try now ;)

@Lapz Lapz mentioned this issue Apr 3, 2019
bors bot added a commit that referenced this issue Apr 7, 2019
1103: Array inference r=flodiebold a=Lapz

Fixes the final item in #394. The only problem is that infering the repeat cause some types to be infered twices.
i.e 
```rust
fn test() {
    let y = unknown;
    [y, &y];
}
```

results in the following diff:

```diff
[11; 48) '{     ...&y]; }': ()
[21; 22) 'y': &{unknown}
[25; 32) 'unknown': &{unknown}
-[38; 45) '[y, &y]': [&&{unknown}]
+[38; 45) '[y, &y]': [&&{unknown};usize]
[39; 40) 'y': &{unknown}
+[39; 40) 'y': &{unknown}
[42; 44) '&y': &&{unknown}
[43; 44) 'y': &{unknown}
```

Should the code produce two inference results for 'y' and if not could any tell me what needs to change.

Co-authored-by: Lenard Pratt <l3np27@gmail.com>
@Lapz
Copy link
Contributor

Lapz commented Apr 7, 2019

I think this issue can be closed

@flodiebold
Copy link
Member Author

Indeed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
E-easy E-has-instructions Issue has some instructions and pointers to code to get started good first issue
Projects
None yet
Development

No branches or pull requests

8 participants