Object safe for dispatch #57545

bovinebuddha · 2019-01-12T14:16:52Z

rust-highfive · 2019-01-12T14:17:02Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @nikomatsakis (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

bovinebuddha · 2019-01-12T14:26:05Z

@withoutboats PR is up :)

nikomatsakis · 2019-01-24T22:34:05Z

@bovinebuddha I'm sorry for not getting back to you in all this time! It's been a very busy couple of weeks. I've added it to my calendar to review on Monday Jan 28, so hopefully that will help.

bovinebuddha · 2019-01-27T12:48:25Z

@nikomatsakis It's alright, I get gather you've got quite a full schedule. Just excited that its finally getting reviewed.

nikomatsakis

OK, I did a first pass. The code seems very nice.

I left myself some notes for next time. I want to come back and re-read the RFC a bit. I also wanted to reconsider some potential interactions with other parts of the trait system.

(I've scheduled a slot in my calendar for this on this coming Friday.)

src/librustc_typeck/check/coercion.rs

src/librustc/ty/wf.rs

mikeyhew · 2019-03-11T01:35:51Z

src/librustc/traits/select.rs

+                            } else {
+                                return;
+                            }
+                        }


Is the check for the feature flag necessary here? It seems like you could check if the trait is object safe anyway, even if the feature is not enabled. Or would that result in duplicate errors?

I believe this is the code which adds the automatic 'impl Trait for dyn Trait' predicate to trait objects. Maybe the branches should be clarified and a comment added. Like:

// If we should add the automatic 'impl Trait for dyn Trait' or not let object_impl_trait = !self.infcx.tcx.features().object_safe_for_dispatch || self.tcx().is_object_safe(principal.def_id()); if (object_impl_trait) { principal.with_self_ty(self.tcx(), self_ty) } else { return; }

At minimum, a comment would be good here -- I think I vaguely prefer the if object_impl_trait { .. } version as well.

But a comment like this would be great:

// Prior to RFC 2027, dyn Foo: Foo was always true. But RFC 2027 makes that only true if Foo is object safe.

mikeyhew · 2019-03-11T01:44:41Z

About the feature name: I know it comes from the RFC, but to me the feature is more general than just allowing static dispatch on dyn Trait types. The feature is that in order for dyn Trait to be a well-formed type, Trait no longer needs to be object-safe. Maybe a name like dyn_trait_always_well_formed would get that across.

bovinebuddha · 2019-03-11T07:02:16Z

I also didn't really like using the RFC name for the feature. I suggested something like 'symmetric_object_safety' but I think your name might be more to the point?

Dylan-DPC-zz · 2019-03-13T21:56:05Z

@nikomatsakis you can review this now (before new conflicts arise 😄 )

rust-highfive · 2019-10-22T19:34:00Z

Your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

2019-10-22T19:26:14.0338334Z ##[command]git remote add origin https://github.com/rust-lang/rust
2019-10-22T19:26:14.0587815Z ##[command]git config gc.auto 0
2019-10-22T19:26:14.0679457Z ##[command]git config --get-all http.https://github.com/rust-lang/rust.extraheader
2019-10-22T19:26:14.0732932Z ##[command]git config --get-all http.proxy
2019-10-22T19:26:14.0872275Z ##[command]git -c http.extraheader="AUTHORIZATION: basic ***" fetch --force --tags --prune --progress --no-recurse-submodules --depth=2 origin +refs/heads/*:refs/remotes/origin/* +refs/pull/57545/merge:refs/remotes/pull/57545/merge
---
2019-10-22T19:32:35.1034354Z   0     0    0     0    0     0      0      0 --:--:--  0:00:06 --:--:--     0
2019-10-22T19:32:36.1046599Z   0     0    0     0    0     0      0      0 --:--:--  0:00:07 --:--:--     0
2019-10-22T19:32:37.1062717Z   0     0    0     0    0     0      0      0 --:--:--  0:00:08 --:--:--     0
2019-10-22T19:32:38.1082119Z   0     0    0     0    0     0      0      0 --:--:--  0:00:09 --:--:--     0curl: (6) Could not resolve host: rust-lang-ci-mirrors.s3-us-west-1.amazonaws.com
2019-10-22T19:32:38.4801986Z The command '/bin/sh -c sh /scripts/sccache.sh' returned a non-zero code: 6
2019-10-22T19:32:39.6400806Z Sending build context to Docker daemon  523.8kB
2019-10-22T19:32:39.6402101Z 
2019-10-22T19:32:39.6656000Z Step 1/8 : FROM ubuntu:16.04
2019-10-22T19:32:39.6660454Z  ---> b9409899fe86
---
2019-10-22T19:32:47.7962564Z   0     0    0     0    0     0      0      0 --:--:--  0:00:06 --:--:--     0
2019-10-22T19:32:48.7974091Z   0     0    0     0    0     0      0      0 --:--:--  0:00:07 --:--:--     0
2019-10-22T19:32:49.7986287Z   0     0    0     0    0     0      0      0 --:--:--  0:00:08 --:--:--     0
2019-10-22T19:32:50.7999303Z   0     0    0     0    0     0      0      0 --:--:--  0:00:09 --:--:--     0curl: (6) Could not resolve host: rust-lang-ci-mirrors.s3-us-west-1.amazonaws.com
2019-10-22T19:32:51.1668143Z The command '/bin/sh -c sh /scripts/sccache.sh' returned a non-zero code: 6
2019-10-22T19:32:53.2805081Z Sending build context to Docker daemon  523.8kB
2019-10-22T19:32:53.2806002Z 
2019-10-22T19:32:53.3056240Z Step 1/8 : FROM ubuntu:16.04
2019-10-22T19:32:53.3080247Z  ---> b9409899fe86
---
2019-10-22T19:33:01.5142410Z   0     0    0     0    0     0      0      0 --:--:--  0:00:06 --:--:--     0
2019-10-22T19:33:02.5154212Z   0     0    0     0    0     0      0      0 --:--:--  0:00:07 --:--:--     0
2019-10-22T19:33:03.5166191Z   0     0    0     0    0     0      0      0 --:--:--  0:00:08 --:--:--     0
2019-10-22T19:33:04.5185925Z   0     0    0     0    0     0      0      0 --:--:--  0:00:09 --:--:--     0curl: (6) Could not resolve host: rust-lang-ci-mirrors.s3-us-west-1.amazonaws.com
2019-10-22T19:33:04.9172830Z The command '/bin/sh -c sh /scripts/sccache.sh' returned a non-zero code: 6
2019-10-22T19:33:08.0460030Z Sending build context to Docker daemon  523.8kB
2019-10-22T19:33:08.0460141Z 
2019-10-22T19:33:08.0734125Z Step 1/8 : FROM ubuntu:16.04
2019-10-22T19:33:08.0739291Z  ---> b9409899fe86
---
2019-10-22T19:33:16.1793869Z   0     0    0     0    0     0      0      0 --:--:--  0:00:06 --:--:--     0
2019-10-22T19:33:17.1805744Z   0     0    0     0    0     0      0      0 --:--:--  0:00:07 --:--:--     0
2019-10-22T19:33:18.1817804Z   0     0    0     0    0     0      0      0 --:--:--  0:00:08 --:--:--     0
2019-10-22T19:33:19.1838434Z   0     0    0     0    0     0      0      0 --:--:--  0:00:09 --:--:--     0curl: (6) Could not resolve host: rust-lang-ci-mirrors.s3-us-west-1.amazonaws.com
2019-10-22T19:33:19.5845978Z The command '/bin/sh -c sh /scripts/sccache.sh' returned a non-zero code: 6
2019-10-22T19:33:23.6902473Z Sending build context to Docker daemon  523.8kB
2019-10-22T19:33:23.6904318Z 
2019-10-22T19:33:23.7097258Z Step 1/8 : FROM ubuntu:16.04
2019-10-22T19:33:23.7104928Z  ---> b9409899fe86
---
2019-10-22T19:33:31.9063018Z   0     0    0     0    0     0      0      0 --:--:--  0:00:06 --:--:--     0
2019-10-22T19:33:32.9075212Z   0     0    0     0    0     0      0      0 --:--:--  0:00:07 --:--:--     0
2019-10-22T19:33:33.9090560Z   0     0    0     0    0     0      0      0 --:--:--  0:00:08 --:--:--     0
2019-10-22T19:33:34.9106620Z   0     0    0     0    0     0      0      0 --:--:--  0:00:09 --:--:--     0curl: (6) Could not resolve host: rust-lang-ci-mirrors.s3-us-west-1.amazonaws.com
2019-10-22T19:33:35.3249257Z The command '/bin/sh -c sh /scripts/sccache.sh' returned a non-zero code: 6
2019-10-22T19:33:35.3250498Z 
2019-10-22T19:33:35.3250498Z 
2019-10-22T19:33:35.3385236Z ##[error]Bash exited with code '1'.
2019-10-22T19:33:35.3420366Z ##[section]Starting: Checkout
2019-10-22T19:33:35.3422315Z ==============================================================================
2019-10-22T19:33:35.3422376Z Task         : Get sources
2019-10-22T19:33:35.3422429Z Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

nikomatsakis · 2019-10-22T20:23:14Z

I'm not sure what the problem is here-- I see no problem locally

nikomatsakis · 2019-10-22T21:36:43Z

@bors r+

I'm going to guess that this error is spurious somehow.

bors · 2019-10-22T21:36:45Z

📌 Commit ef5acde has been approved by nikomatsakis

bors · 2019-10-22T21:36:46Z

🌲 The tree is currently closed for pull requests below priority 1000, this pull request will be tested once the tree is reopened

Centril · 2019-10-22T22:07:53Z

Using this as a canary to check infra status; @bors p=1001

bors · 2019-10-22T22:08:05Z

⌛ Testing commit ef5acde with merge 48420ab...

…atsakis Object safe for dispatch cc #43561

rust-highfive · 2019-10-22T22:13:23Z

Your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

2019-10-22T22:11:12.5017013Z Script contents:
2019-10-22T22:11:12.5017432Z aws s3 cp --acl public-read cpu-usage.csv s3://$DEPLOY_BUCKET/rustc-builds/$BUILD_SOURCEVERSION/cpu-$CI_JOB_NAME.csv
2019-10-22T22:11:12.5017850Z ========================== Starting Command Output ===========================
2019-10-22T22:11:12.5020555Z [command]/bin/bash --noprofile --norc /home/vsts/work/_temp/1f47906c-a724-4e01-a164-3b742d9d085a.sh
2019-10-22T22:12:04.7238699Z upload failed: ./cpu-usage.csv to s3://rust-lang-ci2/rustc-builds/48420ab4175df894b1277e32ec9b19cf4d3f8bc9/cpu-.csv Could not connect to the endpoint URL: "https://rust-lang-ci2.s3.us-west-1.amazonaws.com/rustc-builds/48420ab4175df894b1277e32ec9b19cf4d3f8bc9/cpu-.csv"
2019-10-22T22:12:04.7761902Z 
2019-10-22T22:12:04.7796292Z ##[error]Bash exited with code '1'.
2019-10-22T22:12:04.7833371Z ##[section]Starting: Checkout
2019-10-22T22:12:04.7835561Z ==============================================================================
2019-10-22T22:12:04.7835668Z Task         : Get sources
2019-10-22T22:12:04.7835766Z Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

bors · 2019-10-22T22:13:33Z

💔 Test failed - checks-azure

Centril · 2019-10-22T22:15:18Z

@bors retry p=1

bors · 2019-10-23T13:34:35Z

⌛ Testing commit ef5acde with merge 4a8c5b2...

…atsakis Object safe for dispatch cc #43561

bors · 2019-10-23T17:16:25Z

☀️ Test successful - checks-azure
Approved by: nikomatsakis
Pushing 4a8c5b2 to master...

mikeyhew · 2019-10-26T15:43:35Z

@nikomatsakis once this is stabilized, we remove this hacky code from object_safety.rs

rust/src/librustc/traits/object_safety.rs

Line 558 in 8318ef2

    
           // use a bogus type parameter to mimick a forall(U) query using u32::MAX for now.

In that code, we substitute in U for Self, and add Self: Unsize<U> to the param env. Instead, we'll be able to substitute dyn Trait in for Self. We can't do that right now, because using dyn Trait in the predicate obligation triggers another check that Trait is object safe, and we get a query cycle.

Centril · 2019-10-26T15:51:32Z

@mikeyhew Can you extend the FIXME?

mikeyhew · 2019-10-26T20:45:29Z

@Centril good call. I've opened #65850 to address that

rust-highfive assigned nikomatsakis Jan 12, 2019

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 12, 2019

bovinebuddha force-pushed the object_safe_for_dispatch branch from 5c2fd5a to 18a3376 Compare January 12, 2019 14:19