rustc_typeck: enforce argument type is sized

[venkatagiri]

closes #42312

r? @nikomatsakis

[nikomatsakis]

Looks good! just one minor nit.

[nikomatsakis]

Nit: can you modify this to say "the check for a non-trivial pattern is a hack to avoid..."? Otherwise, it seems like this whole test is a hack, which isn't quite right.

[venkatagiri]

On it.

[venkatagiri]

@nikomatsakis I have fixed the comment.

[nikomatsakis]

@bors r+

Nice!

[bors]

📌 Commit 589a377 has been approved by nikomatsakis

[venkatagiri]

@nikomatsakis A run-pass test failed in CI.
The Target type in Deref trait is optionally Sized.
This feels like a genuine error which should be compiling.

[00:54:08] error[E0277]: the trait bound `<Self as std::ops::Deref>::Target: std::marker::Sized` is not satisfied
[00:54:08]   --> /checkout/src/test/run-pass/associated-types-sugar-path.rs:19:29
[00:54:08]    |
[00:54:08] 19 |     fn baz(_: Self::Target) where Self: Deref {}
[00:54:08]    |                             ^ `<Self as std::ops::Deref>::Target` does not have a constant size known at compile-time
[00:54:08]    |
[00:54:08]    = help: the trait `std::marker::Sized` is not implemented for `<Self as std::ops::Deref>::Target`
[00:54:08]    = help: consider adding a `where <Self as std::ops::Deref>::Target: std::marker::Sized` bound
[00:54:08] 
[00:54:08] error: aborting due to previous error(s)

[Mark-Simulacrum]

@bors r-

[nikomatsakis]

This feels like a genuine error which should be compiling.

I do not think this code should type-check. But I do think it's probably worth doing a crater run -- we may need a warning cycle here.

[nikomatsakis]

Commenced crater run:

• Building toolchain
• Building toolchain
• Start testing crates
• Start testing crates
• Await quiescence
• Generate report: https://gist.github.com/nikomatsakis/076ac4ddedfa570cf9abc805de844692

[nikomatsakis]

Crater report:

• From: 03abb1b
• To: 589a377116d33dca515a8347842c808f96055bb9

Coverage

• 9856 crates tested: 7595 working / 2148 broken / 17 regressed / 14 fixed / 82 unknown.

Root regressions, sorted by rank:

• phf-0.7.21 (before) (after) -- empty logs?
• maplit-0.1.4 (before) (after) -- network error
• r2d2_redis-0.6.0 (before) (after) -- network error
• screenruster-saver-0.1.8 (before) (after) -- network error
• mitochondria-1.1.2 (before) (after) -- network error
• k2hash-sys-0.0.1 (before) (after) -- network error
• kbgpg-0.1.1 (before) (after) -- network error
• accel-mma84-0.2.0 (before) (after) -- network error
• ntree-0.1.0 (before) (after) -- network error
• primapalooza-0.3.4 (before) (after) -- network error
• rmp-rpc-0.0.5 (before) (after) -- network error
• bmp085-0.1.1 (before) (after) -- network error
• bondage-0.0.0 (before) (after) -- network error
• sqlpop-0.3.0 (before) (after) -- mysterious timeout
• tak-0.6.1 (before) (after) -- network error
• tokio-reserve-0.0.0 (before) (after) -- network error

[nikomatsakis]

Everything appears to be false positives. @venkatagiri I think we're good to go, if you just want to adjust that failing test.

[venkatagiri]

@nikomatsakis An UI test is failing now. src/test/ui/type-check/unknown_type_for_closure.rs. Error difference is below. Should my PR be affecting this type error?

diff of stderr:

 error[E0282]: type annotations needed
-  --> $DIR/unknown_type_for_closure.rs:12:14
+  --> $DIR/unknown_type_for_closure.rs:12:17
    |
 12 |     let x = |_| {    };
-   |              ^ consider giving this closure parameter a type
+   |                 ^ cannot infer type for `_`

 error: aborting due to previous error(s)

[nikomatsakis]

@venkatagiri hmm. So I'm a bit surprised that this code would be affected, but it's certainly possible. What's going wrong in this particular case is that the type of this argument is an uninferred type variable (i.e., unconstrained). There is some logic here that tries to special-case ?T: Sized obligations, where ?T is an unresolved type variable, and print them in a nice way. The first question to figure out is whether this logic is still triggering -- basically, is need_type_info() getting called? If so, then we would want to figure out why need_type_info() is not detect and printing the nicer error anymore.

[venkatagiri]

@nikomatsakis The need_type_info is indeed getting called. But, while it is doing visit_expr, it is supposed to reach visit_body, where found_arg_pattern should get updated but it is not reaching that. visit_body isn't even getting called(when earlier it used to). I couldn't quite understand all the walk_s happening.
Please let me know how I could move forward in debugging this one.

[nikomatsakis]

Ah, very interesting. Can you maybe add a debug! to extract the result of self.tcx.hir.find(body_id.node_id)?

[venkatagiri]

@nikomatsakis

...
DEBUG:rustc::ty::maps: ty::queries::extern_crate::try_get_with(key=DefId { krate: CrateNum(1), node: DefIndex(0) => std/8dd0374 }, span=src/test/ui/type-check/unknown_type_for_closure.rs:1:1: 1:1)
DEBUG:rustc::traits::error_reporting: maybe_report_ambiguity(predicate=Binder(TraitPredicate(<_ as std::marker::Sized>)), obligation=Obligation(predicate=Binder(TraitPredicate(<_ as std::marker::Sized>)),depth=0))
DEBUG:rustc::infer: is_tainted_by_errors(err_count=0, err_count_on_creation=0, tainted_by_errors_flag=false)
DEBUG:rustc::ty::fold: HasTypeFlagsVisitor: t=_ t.flags=c04 self.flags=80
DEBUG:rustc::ty::fold: HasTypeFlagsVisitor: t=_ t.flags=c04 self.flags=c
DEBUG:rustc::ty::fold: HasTypeFlagsVisitor: t=_ t.flags=c04 self.flags=4
DEBUG:rustc::infer::error_reporting::need_type_info: self.tcx.hir.find >> Some(NodeExpr(expr(13: { })))
error[E0282]: type annotations needed
  --> src/test/ui/type-check/unknown_type_for_closure.rs:12:17
   |
12 |     let x = |_| {    };
   |                 ^ cannot infer type for `_`

DEBUG:rustc_typeck::check::regionck: regionck_fn(id=4)
...

[arielb1]

So the problem is that need_type_info is using the body-id of the closure instead of that of the outer function, so it only looks for locals within the closure. Currently it gets the body-id from the obligation, which is a bit wonky anyway. How easy would it be to plumb the body of the function to it, through report_fulfillment_errors etc?

[venkatagiri]

@arielb1 Could you please provide some mentoring instructions on how I can go about plumbing the body of function through report_fulfillment_errors? I am new to working on compiler.

[arielb1]

@venkatagiri

The "body of a function" - the "code" within a function's block - is represented by a BodyId. need_type_info uses the BodyId to get the HIR of the function and run over it to get the correct field. The problem here is that need_type_info gets the body of the closure instead of the body of the containing fn, so in your case:

fn main()
{ // we want this body
    let x = |_|
    { // but currently we get this, which misses the _ arg
    };
}

We need to pass need_type_info the BodyId of the "type-checking item", so when it iterates it will find all the fields. An item-level function being type-checked is stored in the Inherited (the FnCtxt represents the closure being checked, which would bring us back to our current problem).

Inherited does not store the BodyId right now, but its constructor does have access to the BodyId of it, so you'll need to store that in the Inherited.

Then you'll only need to pass that BodyId all the way to need_type_info. The call stack goes through these functions:

• infer::error_reporting::need_type_info::need_type_info
• traits::error_reporting::maybe_report_ambiguity (3 separate calls to need_type_info)
• traits::error_reporting::report_fulfillment_error
• traits::error_reporting::report_fulfillment_errors
• check::select_all_obligations_or_error (and also check::select_obligations_where_possible) - here you can access the Inherited.

You can pass the BodyId through these functions as a parameter. There are actually other call sites to report_fulfillment_errors, but these don't seem to have anything to do with function bodies, so you'll actually want to pass an Option<BodyId>, pass None from these callsites, and don't look for anything in these cases (currently, we pass BodyIds that actually refer to non-bodies such as the fn item, so we have this hack to not visit anything in that case. That hack could be removed now).

[nikomatsakis]

@arielb1

So the problem is that need_type_info is using the body-id of the closure instead of that of the outer function

yeah that's sort of what I expected

How easy would it be to plumb the body of the function to it, through report_fulfillment_errors etc?

Not very hard, I suppose. However, looking at the closure body-id should also be ok, I would think, since the body actually contains the patterns for the parameters; it might just be a matter of tweaking the visitor?

[nikomatsakis]

Still, I do eventually plan to remove the body-id from its current position, so threading the body-id down is probably a good idea.

[arielb1]

Not very hard, I suppose. However, looking at the closure body-id should also be ok, I would think, since the body actually contains the patterns for the parameters; it might just be a matter of tweaking the visitor?

The body is just the expression and does not include the signature. In any case, there could be Sized requirements for locals in the base functions etc. that were added from the closure, so it's still probably the best to not rely on which obligation gets picked for error reporting.

[nikomatsakis]

@arielb1 yes, ok

[nikomatsakis]

@venkatagiri I see that @arielb1 left some pretty good instructions here -- GH wasn't showing me those at first, or I failed to refresh anyway -- but I agree with the plan he laid out for how to fix it.

[venkatagiri]

@arielb1 Thanks for the detailed instructions. I have updated the PR with the changes. But, now, there is another compile-fail test failing. The type annotations needed error has moved from the inner closure(with y as arg) to outer closure(with x as arg).

fn main() {
    let ex = |x| { // Error is thrown for x and below error is missing.
        let_(add(x,x), |y| { //~ ERROR type annotations needed
            let_(add(x, x), |x|x)})};
}

[nikomatsakis]

@venkatagiri that seems fine; I think you can just update the test and move the //~ ERROR annotation around.

[nikomatsakis]

this looks nice :)

[nikomatsakis]

@bors r+

[bors]

📌 Commit 5ed21f5 has been approved by nikomatsakis

[bors]

⌛ Testing commit 5ed21f5 with merge b3cf125...

[bors]

💔 Test failed - status-travis

[Mark-Simulacrum]

@bors retry

• Spurious segfault in tests for musl #38618

[bors]

⌛ Testing commit 5ed21f5 with merge 0816b94...

[bors]

☀️ Test successful - status-appveyor, status-travis
Approved by: nikomatsakis
Pushing 0816b94 to master...