Replace `structurally_resolved_type` in casts check. #48270

leoyvens · 2018-02-16T13:22:34Z

The behaviour of resolve_type_vars_if_possible is simpler and infallible. Other minor refactorings.

I'm not sure if this is backwards compatible, in theory resolving obligations between two cast checks could solve a dependency between them, but I don't know if that's actually possible and it doesn't sound like something we'd want to support.

rust-highfive · 2018-02-16T13:22:37Z

r? @estebank

(rust_highfive has picked a reviewer for you, use r? to override)

estebank · 2018-02-16T23:11:31Z

LGTM, I think we should run crater on this to verify that we're not breaking something in the wild.

@bors try

bors · 2018-02-16T23:11:42Z

⌛ Trying commit 1efd547 with merge 2d68ae8...

Replace `structurally_resolved_type` in casts check. The behaviour of `resolve_type_vars_if_possible` is simpler and infallible. Other minor refactorings. I'm not sure if this is backwards compatible, in theory resolving obligations between two cast checks could solve a dependency between them, but I don't know if that's actually possible and it doesn't sound like something we'd want to support.

bors · 2018-02-17T01:24:38Z

☀️ Test successful - status-travis
State: approved= try=True

leoyvens · 2018-02-18T14:34:07Z

I've tacked on a similar refactoring to unary expr check. In theory this could make more code compile because we don't error or could ommit an otherwise useful error. In practice I couldn't observe any effect.

estebank · 2018-02-18T23:17:39Z

cc @rust-lang/compiler

nikomatsakis · 2018-02-19T17:07:48Z

r? @nikomatsakis -- I'd like to read this over

nikomatsakis · 2018-02-19T20:10:22Z

src/librustc/ty/cast.rs

@@ -64,7 +63,7 @@ impl<'tcx> CastTy<'tcx> {
            ty::TyBool => Some(CastTy::Int(IntTy::Bool)),
            ty::TyChar => Some(CastTy::Int(IntTy::Char)),
            ty::TyInt(_) => Some(CastTy::Int(IntTy::I)),
-            ty::TyInfer(ty::InferTy::IntVar(_)) => Some(CastTy::Int(IntTy::Ivar)),
+            ty::TyInfer(ty::InferTy::IntVar(_)) => Some(CastTy::Int(IntTy::I)),


this Ivar is basically unused I guess?

nikomatsakis · 2018-02-19T20:11:37Z

src/librustc_typeck/check/cast.rs

@@ -488,11 +488,7 @@ impl<'a, 'gcx, 'tcx> CastCheck<'tcx> {
                    ty::TypeVariants::TyInfer(t) => {
                        match t {
                            ty::InferTy::IntVar(_) |
-                            ty::InferTy::FloatVar(_) |
-                            ty::InferTy::FreshIntTy(_) |
-                            ty::InferTy::FreshFloatTy(_) => {


Similarly, the "fresh" variants here are just dead code I suppose

nikomatsakis · 2018-02-20T21:54:55Z

OK, well, the change basically makes sense, not a big diff obviously. crater run seems like a fine thing.

cc @rust-lang/infra

nikomatsakis · 2018-02-20T21:57:48Z

I'm not sure if this is backwards compatible, in theory resolving obligations between two cast checks could solve a dependency between them, but I don't know if that's actually possible and it doesn't sound like something we'd want to support.

I'm not entirely happy with how casts fit into the pipeline, but I suppose we would probably prefer for them to apply "atomically" (maybe?). At least, I don't love the idea of the ordering being overly significant, although I guess it's not the end of the world if it is, since we already have some ordering dependencies from coercions.

nikomatsakis · 2018-03-09T23:55:34Z

cc @rust-lang/infra -- or should it be @rust-lang/release ? -- any word on crater run?

Mark-Simulacrum · 2018-03-10T00:40:53Z

Crater run started; results expected in ~5 days. Our crater queue is quite full right now, so PRs are waiting for a week or two generally to get their turn at crater.

aidanhs · 2018-03-15T00:28:33Z

Crater results: http://cargobomb-reports.s3.amazonaws.com/pr-48270/index.html. 'Blacklisted' crates (spurious failures etc) can be found here. If you see any spurious failures not on the list, please make a PR against that file.

(looks like this broke nix and so broke half the world 😄)

leoyvens · 2018-03-15T17:16:56Z

Thanks for the crater run! Interesting results. Curiously nix itself didn't regress because the newest version isn't affected. Anyways, the issue is that it's possible to depend on the order in which casts are checked, this compiles today:

fn main() {
    let x = &"hello";
    let mut y = x as *const _;
    y = 0 as *const _;
}

But this does not compile today:

fn main() {
    let x = &"hello";
    let mut y = 0 as *const _;
    y = x as *const _;
              ^^^^^^^^ cannot cast to a pointer of an unknown kind.
}

We should make a decision, either they should both compile, or none of them should compile. If they both should compile, then we should use a more robust fixed-point algorithm rather than check in arbitratry order. If none of them should compile, then we should introduce a future-compatibility warning.

nikomatsakis · 2018-03-19T21:19:48Z

We should make a decision, either they should both compile, or none of them should compile

Ideally yes, but in fact order does matter to rustc type checking quite a lot -- all coercions operate on a "greedy basis" and it's going to be a long road getting away from it, if we ever fully do.

nikomatsakis · 2018-03-20T18:02:59Z

So, like @leodasvacas, I am not thrilled with the order dependency on casts -- we had intended to avoid it. Though, iirc, I've noted it before, and decided that -- since it exists and code relies on it -- we ought to keep that code working as best we can. Perhaps the same rule of thumb applies here.

I don't think that breaking the older version of nix is really an option. It might be ok if we can instead publish a patch for that older version. That's imperfect, since apps would still need to update their lock files, but better than the current status. We might then want to do a warning period as well.

nikomatsakis · 2018-03-20T18:03:27Z

I'd like to nominate for discussion but I'm not sure where =) I guess we'll start with T-lang.

nikomatsakis · 2018-03-22T20:20:06Z

@leodasvacas

We should make a decision, either they should both compile, or none of them should compile. If they both should compile, then we should use a more robust fixed-point algorithm rather than check in arbitratry order.

So we were talking about this in the meeting. I find this idea appealing -- but I am a bit wary of building that on the current implementation. I wonder if you'd have interest in trying to extend Chalk to model coercions and casts? In such a context, I would feel much more confident that we aren't accidentally introducing ambiguity.

In general, I have this feeling like the right way to correct things here is to try and leave the current implementation alone -- so as not to disturb existing code -- and transition towards a new implementation with a cleaner foundation.

The behaviour of `resolve_type_vars_if_possible` is simpler and infallible.

`resolve_type_vars_with_obligations` is the same but doesn't error on unresolved type variables. In theory this could make more code compile because we don't error or could ommit an otherwise useful error. In practice I couldn't observe any effect.

leoyvens · 2018-03-22T23:24:53Z

I've updated this PR to revert the problematic change and added tests for the current behavior.

I wonder if you'd have interest in trying to extend Chalk to model coercions and casts?

Sounds interesting, I'll ping you on gitter.

shepmaster · 2018-04-07T22:12:06Z

What is the path forward with this PR? It sounds like "close" to me.

leoyvens · 2018-04-08T15:22:13Z

This PR basically just adds tests now. It's mistagged, should be waiting-for-review.

shepmaster · 2018-04-08T18:42:58Z

should be waiting-for-review.

That's on you @nikomatsakis !

nikomatsakis · 2018-04-11T18:28:00Z

@bors r+

bors · 2018-04-11T18:28:01Z

📌 Commit 0a5a5c3 has been approved by nikomatsakis

…atsakis Replace `structurally_resolved_type` in casts check. The behaviour of `resolve_type_vars_if_possible` is simpler and infallible. Other minor refactorings. I'm not sure if this is backwards compatible, in theory resolving obligations between two cast checks could solve a dependency between them, but I don't know if that's actually possible and it doesn't sound like something we'd want to support.

Rollup of 14 pull requests Successful merges: - #49525 (Use sort_by_cached_key where appropriate) - #49575 (Stabilize `Option::filter`.) - #49614 (in which the non-shorthand patterns lint keeps its own counsel in macros) - #49665 (Small nits to make couple of tests pass on mips targets.) - #49781 (add regression test for #16223 (NLL): use of collaterally moved value) - #49795 (Properly look for uninhabitedness of variants in niche-filling check) - #49809 (Stop emitting color codes on TERM=dumb) - #49856 (Do not uppercase-lint #[no_mangle] statics) - #49863 (fixed typo) - #49857 (Fix "fp" target feature for AArch64) - #49849 (Add --enable-debug flag to musl CI build script) - #49734 (proc_macro: Generalize `FromIterator` impl) - #49730 (Fix ICE with impl Trait) - #48270 (Replace `structurally_resolved_type` in casts check.) Failed merges:

rust-highfive assigned estebank Feb 16, 2018

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 16, 2018

rust-highfive assigned nikomatsakis and unassigned estebank Feb 19, 2018

nikomatsakis reviewed Feb 19, 2018

View reviewed changes

nikomatsakis added S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 20, 2018

shepmaster added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-crater Status: Waiting on a crater run to be completed. labels Mar 18, 2018

nikomatsakis added I-nominated T-lang Relevant to the language team, which will review and decide on the PR/issue. S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels Mar 20, 2018

nikomatsakis removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 20, 2018

leoyvens added 2 commits March 22, 2018 17:42

Replace structurally_resolved_type in casts check.

7719f53

The behaviour of `resolve_type_vars_if_possible` is simpler and infallible.

nikomatsakis removed the I-nominated label Mar 22, 2018

leoyvens force-pushed the refactor-casts branch from 05ceae5 to 04121c5 Compare March 22, 2018 22:51

revert making casts atomic, test order dependency

0a5a5c3

leoyvens force-pushed the refactor-casts branch from 04121c5 to 0a5a5c3 Compare March 27, 2018 01:59

shepmaster added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels Apr 8, 2018

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 11, 2018

kennytm mentioned this pull request Apr 11, 2018

Rollup of 14 pull requests #49875

Merged

bors merged commit 0a5a5c3 into rust-lang:master Apr 12, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace `structurally_resolved_type` in casts check. #48270

Replace `structurally_resolved_type` in casts check. #48270

leoyvens commented Feb 16, 2018

rust-highfive commented Feb 16, 2018

estebank commented Feb 16, 2018

bors commented Feb 16, 2018

bors commented Feb 17, 2018

leoyvens commented Feb 18, 2018

estebank commented Feb 18, 2018

nikomatsakis commented Feb 19, 2018

nikomatsakis Feb 19, 2018

nikomatsakis Feb 19, 2018

nikomatsakis commented Feb 20, 2018

nikomatsakis commented Feb 20, 2018

nikomatsakis commented Mar 9, 2018

Mark-Simulacrum commented Mar 10, 2018

aidanhs commented Mar 15, 2018

leoyvens commented Mar 15, 2018

nikomatsakis commented Mar 19, 2018

nikomatsakis commented Mar 20, 2018

nikomatsakis commented Mar 20, 2018

nikomatsakis commented Mar 22, 2018

leoyvens commented Mar 22, 2018

shepmaster commented Apr 7, 2018

leoyvens commented Apr 8, 2018

shepmaster commented Apr 8, 2018

nikomatsakis commented Apr 11, 2018

bors commented Apr 11, 2018

Replace structurally_resolved_type in casts check. #48270

Replace structurally_resolved_type in casts check. #48270

Conversation

leoyvens commented Feb 16, 2018

rust-highfive commented Feb 16, 2018

estebank commented Feb 16, 2018

bors commented Feb 16, 2018

bors commented Feb 17, 2018

leoyvens commented Feb 18, 2018

estebank commented Feb 18, 2018

nikomatsakis commented Feb 19, 2018

nikomatsakis Feb 19, 2018

Choose a reason for hiding this comment

nikomatsakis Feb 19, 2018

Choose a reason for hiding this comment

nikomatsakis commented Feb 20, 2018

nikomatsakis commented Feb 20, 2018

nikomatsakis commented Mar 9, 2018

Mark-Simulacrum commented Mar 10, 2018

aidanhs commented Mar 15, 2018

leoyvens commented Mar 15, 2018

nikomatsakis commented Mar 19, 2018

nikomatsakis commented Mar 20, 2018

nikomatsakis commented Mar 20, 2018

nikomatsakis commented Mar 22, 2018

leoyvens commented Mar 22, 2018

shepmaster commented Apr 7, 2018

leoyvens commented Apr 8, 2018

shepmaster commented Apr 8, 2018

nikomatsakis commented Apr 11, 2018

bors commented Apr 11, 2018

Replace `structurally_resolved_type` in casts check. #48270

Replace `structurally_resolved_type` in casts check. #48270