cargotest/support: remove internal mutability in favor of switching types #4314

nodakai · 2017-07-22T04:24:11Z

Cell<bool> was removed from ProjectBuilder. Instead PhantomData<T> was added to it (and RepoBuilder) to manage state transition in the type level (T is either Configuring or Done.)
ProjectBuilder::cargo_process() was removed as its design heavily depended on the internal mutability.

Also added #[must_use] to ProjectBuilder and RepoBuilder to check for call sites of their build() method.

rust-highfive · 2017-07-22T04:24:22Z

r? @alexcrichton

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

matklad · 2017-07-22T09:32:15Z

Wow, this looks cool @nodakai ! I think we need to consult @alexcrichton about general approach here.

I personally like this idea, because I've been bitten myself by not calling build, but I am afraid a little that this can affect compile times badly (we have a ton of integration tests, and each of them will have to do monomorphization).

Looks like almost all methods are implemented on ProjectBuilder<Done> and ProjectBulilder<Configuring>, and almost none on Project<Builder>.

So perhaps we can make two non-generic types, ProjectBuilder and Project?

nodakai · 2017-07-23T13:18:40Z

Good point, but actually my benchmark with time cargo test --no-run didn't show noticeable differences in the "user" time before and after my commit. P-values >> 0.05 mean there were no differences in the variances and the means of the two data sets, respectively. (YMMV)

So we can focus on which design is easier to use/maintain without worrying about compile time.

> bef<-c(297.70, 290.04, 290.38, 299.61, 301.13)
> aft<-c(294.68, 297.19, 296.62, 290.33, 296.24)
> var.test(bef, aft)

        F test to compare two variances

data:  bef and aft
F = 3.5338, num df = 4, denom df = 4, p-value = 0.249
alternative hypothesis: true ratio of variances is not equal to 1
95 percent confidence interval:
  0.3679351 33.9409042
sample estimates:
ratio of variances 
          3.533843 
> t.test(bef, aft, var.equal=T)

        Two Sample t-test

data:  bef and aft
t = 0.2873, df = 8, p-value = 0.7812
alternative hypothesis: true difference in means is not equal to 0
95 percent confidence interval:
 -5.340106  6.860106
sample estimates:
mean of x mean of y 
  295.772   295.012

https://gist.github.com/nodakai/3600c808725f44f2cd24939168358726

alexcrichton · 2017-07-23T15:24:49Z

I've personally never really been a huge fan of typestate and how the ergonomics work out in Rust, could we perhaps just maintain internal state and "do the right thing"? I don't think "you can only call build once" causes problems in practice. It seems great though to collapse cargo and cargo_process methods!

matklad · 2017-07-24T10:31:18Z

Yeah, phantom types do seem like an overkill here. However, splitting this into two completely different types, Project and ProjectBuilder, seems natural? Looks like this was the original intention of code here: https://github.com/rust-lang/cargo/pull/4314/files#diff-78598c8b4dd75654cb0ade93d2bc0059L200 :)

alexcrichton · 2017-07-24T14:26:10Z

Wow that is a dated comment. But sure yeah, having two types seems fine.

bors · 2017-08-27T08:51:35Z

☔ The latest upstream changes (presumably #4439) made this pull request unmergeable. Please resolve the merge conflicts.

…ypes Remove is_build: Cell<bool> from ProjectBuilder and introduce a new type Project. is_build==false <-> ProjectBuilder is_build==true <-> Project Also add #[must_use] to ProjectBuilder to confirm its instances are surely consumed by its build() method to produce Project. The same goes for RepoBuilder. ProjectBuilder::cargo_process() was removed as its design heavily depended on the internal mutability. Signed-off-by: NODA, Kai <nodakai@gmail.com>

nodakai · 2017-10-10T09:46:51Z

Removed phantom types according to the above discussion, and rebased against master

alexcrichton · 2017-10-10T21:51:36Z

@matklad mind taking over in reviewing this?

matklad · 2017-10-10T22:06:35Z

@alexcrichton unfortunately I don't have enough time now :(

alexcrichton · 2017-10-11T15:24:30Z

@nodakai unfortunately I'm also a little short on time right now, so it may be awhile before I can get to this. It's a pretty huge diff so if you can do anything to shrink it that'd definitely be appreciated.

nodakai · 2017-10-12T02:40:07Z

@alexcrichton No need to rush, take your time.

As you know, the cargo test suite consists of an internal library tests/cargotest/ and actual tests tests/*.rs (61 files!) which depend on it. This patch is large mainly because changes in the library part affected many test cases.

matklad · 2017-10-12T08:42:55Z

tests/cargotest/support/git.rs


+#[must_use]


matklad · 2017-10-12T08:52:08Z

LGTM: splitting Project into ProjectBuilder & Project is definitely the right thing to do!

Let's

@bors r+

it until it hits a merge conflict :)

Thanks for fixing up all those tests @nodakai ! i imagine this was a chore!

bors · 2017-10-12T08:52:10Z

📌 Commit d43ee1d has been approved by matklad

bors · 2017-10-12T08:52:17Z

⌛ Testing commit d43ee1d with merge e237a19259561f36d75507bb4b2a8a47bbc5f3b2...

bors · 2017-10-12T08:52:42Z

💡 This pull request was already approved, no need to approve it again.

This pull request is currently being tested. If there's no response from the continuous integration service, you may use retry to trigger a build again.

bors · 2017-10-12T08:52:42Z

📌 Commit d43ee1d has been approved by matklad

bors · 2017-10-12T08:52:49Z

⌛ Testing commit d43ee1d with merge 5763451...

cargotest/support: remove internal mutability in favor of switching types `Cell<bool>` was removed from `ProjectBuilder`. Instead `PhantomData<T>` was added to it (and `RepoBuilder`) to manage state transition in the type level (`T` is either `Configuring` or `Done`.) `ProjectBuilder::cargo_process()` was removed as its design heavily depended on the internal mutability. Also added `#[must_use]` to `ProjectBuilder` and `RepoBuilder` to check for call sites of their `build()` method.

bors · 2017-10-12T09:17:50Z

☀️ Test successful - status-appveyor, status-travis
Approved by: matklad
Pushing 5763451 to master...

rust-highfive assigned alexcrichton Jul 22, 2017

nodakai force-pushed the phantom-type-in-projectbuilder branch from 6a32139 to b94d333 Compare October 7, 2017 15:20

nodakai force-pushed the phantom-type-in-projectbuilder branch from b94d333 to d43ee1d Compare October 10, 2017 09:03

nodakai changed the title ~~cargotest/support: remove internal mutability in favor of phantom type~~ cargotest/support: remove internal mutability in favor of switching types Oct 10, 2017

matklad reviewed Oct 12, 2017

View reviewed changes

tests/cargotest/support/git.rs

#[must_use]

Copy link

Member

matklad Oct 12, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 💯

bors merged commit d43ee1d into rust-lang:master Oct 12, 2017

bors mentioned this pull request Oct 12, 2017

when cross-compiling, look for RUSTFLAGS_HOST for rustc flags of build scripts #4428

Closed

nodakai deleted the phantom-type-in-projectbuilder branch October 18, 2017 12:09

ehuss added this to the 1.22.0 milestone Feb 6, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cargotest/support: remove internal mutability in favor of switching types #4314

cargotest/support: remove internal mutability in favor of switching types #4314

nodakai commented Jul 22, 2017

rust-highfive commented Jul 22, 2017

matklad commented Jul 22, 2017 •

edited

Loading

nodakai commented Jul 23, 2017

alexcrichton commented Jul 23, 2017

matklad commented Jul 24, 2017

alexcrichton commented Jul 24, 2017

bors commented Aug 27, 2017

nodakai commented Oct 10, 2017

alexcrichton commented Oct 10, 2017

matklad commented Oct 10, 2017

alexcrichton commented Oct 11, 2017

nodakai commented Oct 12, 2017

matklad Oct 12, 2017

matklad commented Oct 12, 2017 •

edited

Loading

bors commented Oct 12, 2017

bors commented Oct 12, 2017

bors commented Oct 12, 2017

bors commented Oct 12, 2017

bors commented Oct 12, 2017

bors commented Oct 12, 2017

cargotest/support: remove internal mutability in favor of switching types #4314

cargotest/support: remove internal mutability in favor of switching types #4314

Conversation

nodakai commented Jul 22, 2017

rust-highfive commented Jul 22, 2017

matklad commented Jul 22, 2017 • edited Loading

nodakai commented Jul 23, 2017

alexcrichton commented Jul 23, 2017

matklad commented Jul 24, 2017

alexcrichton commented Jul 24, 2017

bors commented Aug 27, 2017

nodakai commented Oct 10, 2017

alexcrichton commented Oct 10, 2017

matklad commented Oct 10, 2017

alexcrichton commented Oct 11, 2017

nodakai commented Oct 12, 2017

matklad Oct 12, 2017

Choose a reason for hiding this comment

matklad commented Oct 12, 2017 • edited Loading

bors commented Oct 12, 2017

bors commented Oct 12, 2017

bors commented Oct 12, 2017

bors commented Oct 12, 2017

bors commented Oct 12, 2017

bors commented Oct 12, 2017

matklad commented Jul 22, 2017 •

edited

Loading

matklad commented Oct 12, 2017 •

edited

Loading