fix/rid-build: opt-in to Rust nightly when nightly is not the default toolchain

[SecondFlight]

When running the setup script in examples/dart/clock...

cargo run +nightly rid_build

... the following error is generated:

   Compiling clock v0.1.0 (C:\Users\qbgee\Documents\Code\rid\examples\dart\clock)
    Finished dev [unoptimized + debuginfo] target(s) in 58.83s
     Running `target\debug\rid_build.exe +nightly rid_build`
2021-10-03 21:35:26,996 INFO [rid_build] Generating bindings
thread 'main' panicked at 'Build failed: 
'cargo expand' encountered error(s):

   Compiling clock v0.1.0 (C:\Users\qbgee\Documents\Code\rid\examples\dart\clock)
error: the option `Z` is only accepted on the nightly compiler

error: could not compile `clock`

', rid_build.rs:23:45
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

As far as I can tell, this is because I don't have nightly as my default toolchain. This PR allows rid-build to run if nightly is not set as the default toolchain. It does this by adding the +nightly flag to the cargo rustc args generated by cargo_expand_args() in build_target.rs.

---

I have some questions about contributing.

You say in the readme that:

Any contribution should be provided via a pull request.

You also say

Please do not fork this repository

and

I expect you to only read or use the code, but not push any updates to the repository.

The way I read this, contributing is impossible; in order to contribute, I must either privately fork the repository or push to a branch within this repository.

I've done what I think is safest:

1. I made a small change and tested it locally.
2. I used the Github editor to mirror the change for this PR, thus ensuring I didn't accidentally push updates to master or otherwise break the repo.
3. I opened this PR from the resulting branch.
4. I followed all other contribution guidelines to the best of my ability.

I apologize if any part of this wasn't correct.

Would it be okay for me to privately fork this repository, or is there another way you'd prefer to see contributions made?

[thlorenz]

Thanks for the PR, what you did is fine. It is also ok to fork as long as that fork stays private for now (actually providing PRs from your fork is preferred .. I need to update this in the Readme).

WRT your fix the cargo expand code needs nightly due to the -Z flag that is correct. Your idea to code that into the command is good until we solve this via a proper command line tool which I'll build eventually.

Unfortunately this PR broke one test (I'm not entirely sure why). Could you please run make test from the root of the project on your branch to see if you can reproduce that problem locally so we can figure out how to fix it?

[SecondFlight]

Thanks for the reply! I've forked the repo and I'll make future contributions from there.

---

I've been doing some testing, and I'm more confused than when I started.

I'm unable to reproduce this in my fork:

Locally, both on master and on fix/nightly-opt-in, this issue occurs. No matter what I do, I see this specific test failure. I also tried adding +nightly to the makefile, but nothing changed.

I'm doing this on Windows, so that could have something to do with it?

[SecondFlight]

Well I don't how I feel about this, but a re-run fixed the issue here 😅

Since this is still happening for me locally, I'll keep digging and see if I can figure out why.

[SecondFlight]

A couple interesting discoveries.

First, I have had the test succeed locally twice after probably 50 runs. I haven't been able to reliably reproduce this.

Second, I can "fix" the issue by safely unwrapping where the error occurs (rid-macro-impl/src/common/state.rs):

     pub fn handled_impl_method_export(&self, ident: &Ident) -> bool {
-        self.handled_impl_method_exports
-            .as_ref()
-            .unwrap()
-            .contains(&ident.to_string())
+        match self.handled_impl_method_exports.as_ref() {
+            Some(exports) => {
+                exports.contains(&ident.to_string())
+            }
+            None => {
+                false
+            }
+        }
     }

I really don't like this, if only because I still don't understand what's going on. I'll keep digging.

[SecondFlight]

Here's a fix I feel a little better about. In rid-macro-impl/src/common/state.rs:

 impl ExpandState {
     fn init(&mut self) {
         if !self.initialized {
-            self.initialized = true;
             self.emitted_implementations = Some(HashSet::new());
             self.emitted_idents = Some(HashMap::new());
             self.handled_impl_method_exports = Some(HashSet::new());
+            self.initialized = true;
         }
     }

It seems like this should only break if there's threading happening that I don't know about, as the comment at the bottom of the file suggests. I don't feel confident enough to formally propose this change so I'll leave this for now.

[thlorenz]

Thanks for investigating this, it looks like a race condition issue. state is global (by necessity) and it could be that the order that tests run affects the outcome.

You could try to run cargo test --all -- --test-threads=1 from the root (to prevent them running in parallel and affecting each other) and see if that fixes the flaky tests.

This change shouldn't really change anything as that code runs sync from top to bottom, but in a race condition situation it could, I'm not sure ATM. I cannot reproduce the problem on my machine either way and CI has been pretty stable previously as well.

This change probably changes how things get compiled and the match version may be more efficient and thus making the race condition less likely.

I'd prefer to fix this properly somewhere down the line, but if running tests single threaded (one after the other) works we can leave it there for now.

[SecondFlight]

I bet that's it. I can only reliably reproduce it on my work PC which has 6 cores. My personal PC doesn't have the issue. I'll try cargo test --all -- --test-threads=1 when I get in this morning.

[SecondFlight]

Yep, that fixes it. ~I'm working on a PR to support Windows in the Makefiles so I'll include the fix there if that's alright.~ Just saw your comment above.

[SecondFlight]

The action was queued for a couple hours so I cancelled it. Looks like Github Actions is having some issues right now: https://www.githubstatus.com/incidents/bdbzpz7qxmbx

I tested the most recent change locally and it works. As I commented above, I won't be pushing anything else to this branch. Feel free to revert that commit before merging if you'd like.

[SecondFlight]

This is super broken locally, not sure how I missed it 😅 The full command, expanded as cargo rustc +nightly --example strings -- -Zunpretty=expanded, breaks with the following error:

   Compiling field_access v0.1.0 (C:\Users\qbgee\Documents\Code\rid\tests\dart\field_access)
error: Unrecognized option: 'example'