Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Broken on latest nightlies (regression on 2020-06-04) #1

Open
mx00s opened this issue Dec 16, 2020 · 7 comments
Open

Broken on latest nightlies (regression on 2020-06-04) #1

mx00s opened this issue Dec 16, 2020 · 7 comments

Comments

@mx00s
Copy link
Collaborator

mx00s commented Dec 16, 2020

Bumping the rust version in rust-toolchain by one day causes the crate to fail with compile-time errors like this:

error: constant expression depends on a generic parameter
 --> src/ranged_i32/add/ranged_i32.rs:9:5
  |
9 |     type Output = RangedI32<{ START + START_RHS }, { END + END_RHS }>;
  |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  |
  = note: this may fail depending on what value the parameter takes

This reproduces with the recent nightly-2020-12-15 release as well.

What changed?

Here are the rustc git revisions associated with 2020-06-03 and 2020-06-04 nightly releases:

$ rustup run nightly-2020-06-03 rustc --version
rustc 1.45.0-nightly (fe10f1a49 2020-06-02)
$ rustup run nightly-2020-06-04 rustc --version
rustc 1.45.0-nightly (56daaf669 2020-06-03)

Here's what changed over that range of commits: rust-lang/rust@fe10f1a...56daaf6.

The following stick out to me, although I haven't yet traced any concrete connection between any particular change and the regression here. Some test files include similar error messages with strings like constant expression depends on a generic parameter and this may fail depending on what value the parameter takes, but the text must have already existed in the compiler's source:

Both come from the rust-lang/rust#70107 PR.

Potential Solutions

  1. Find a workaround
  2. Patch rustc and convince the community to adopt it

In either case it may be insightful to further isolate exactly what change(s) caused the regression and whether undoing them on top of a 56daaf669 build fixes it. To do that I'd need to figure out how to persuade cargo/rustup to use a custom-built rustc.

@mx00s
Copy link
Collaborator Author

mx00s commented Dec 16, 2020

I'd need to figure out how to persuade cargo/rustup to use a custom-built rustc.

Aha! Cargo reads the RUSTC environment variable and, if it's set, uses it as the path to rustc instead of the default behavior. It also supports overriding in Cargo.toml .cargo/config.toml using build.rustc.

I'll try devising a workflow for this.

@mx00s
Copy link
Collaborator Author

mx00s commented Dec 19, 2020

I pushed rustc build tooling to the rustc-workflow branch.

./cargo-2020-06-03.sh test does the following:

  1. create a docker image that builds rustc using rev fe10f1a49 (nightly-2020-06-03) and sets the RUSTC environment variable to the resulting rustc binary
  2. spawn a temporary docker container from that image which runs cargo test in a volume-mount of the host's current working directory

The first time you run the script the first step will take a long time (your threadripper should help).

When I run that command it successfully runs all 19 tests and then emits a compiler error during the doc-test phase:

test result: ok. 19 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out

   Doc-tests ranged_const
error[E0514]: found crate `arith_traits` compiled by an incompatible version of rustc
 --> /tmp/src/ranged_i32/checked.rs:2:5
  |
2 | use arith_traits::{Check, Overflow};
  |     ^^^^^^^^^^^^
  |
  = help: please recompile that crate using this compiler (rustc 1.48.0 (7eac88abb 2020-11-16))
  = note: the following crate versions were found:
          crate `arith_traits` compiled by rustc 1.45.0-dev: /tmp/target/debug/deps/libarith_traits-f7f8397b6890058d.rlib

error: aborting due to previous error

error: test failed, to rerun pass '--doc'

This suggests using the RUSTC environment variable to specify the compiler to use isn't totally reliable. I had also tried specifying the rustc path using build.rustc the .cargo/config[.toml] file in all three of your crates, but it doesn't change the behavior.

One potential explanation is the docker images are getting cargo using rustup's installer, and maybe using a more recent version of cargo with an old rustc build is a bad idea. If I were to pick this up again that's probably where I'd start, but I'm going to set this aside for now. I hope you find the tooling helpful.

EDIT: For completeness, I also included a cargo-2020-06-04.sh script, but I haven't tested it yet to verify it shows the same regression as the nightly-2020-06-04 build provided by rustup. (My machine is doing the first docker image build for it.)

EDIT 2: ./cargo-2020-06-04.sh test finished and shows the same sort of errors we're seeing with the corresponding nightly from rustup. Good sign.

@U007D
Copy link
Owner

U007D commented Dec 21, 2020

The rustc version used to build the project is specified by the rust-toolchain file in the crate root. It is pinned to 2020-06-03 because (as you correctly identified), changes to the const generics feature changed from "overflow-on-const-arithmetic-breaks-the-build" to "any-const-arithmetic-in-an-expression-is-automatically-breaks-the-build".

There is some discussion of the issue here. Tracing through the changes that were made and the rationale behind them (which I don't fully agree with), I'm not sure a PR which restores the prior behavior would be accepted, unfortunately.

@mx00s
Copy link
Collaborator Author

mx00s commented Dec 22, 2020

I agree, it seems like getting features to support this use case, let alone their stabilization, may not be attainable.

If we don't pursue compiler changes, this leaves us with:

  1. Find a workaround

What you're doing here reminds me a lot of the sort of code implemented in theorem provers. They'll use type-level encodings of natural numbers to help prove relations like your range invariants. A common way to do that is with a unary encoding where some type represents zero and another type represents the successor (+1) of another encoded number. Here's a small demo. It doesn't compile, but maybe the idea is enough to help you figure more out.

You're probably horrified about space and time overhead of this representation. It's brutal, but the hope is all of that overhead could be confined to compile-time, and there would be no overhead at runtime. My understanding is the standard Nat type in Idris, for instance, does all the compile-time computations in terms of more space+time efficient arbitrary-precision encodings. However, invariants defined in terms of Nats, like the length of two appended Vects is the sum of their lengths, is ensured at compile-time for all possible Vect-lengths, even for Vects of unknown length which are created dynamically at runtime. Runtime checks against the length of Vect's aren't necessary because their API and the type system make it impossible to access out of bounds.

It may work to port this to a language like Idris, expose a C-compatible interface for it, and then implement a library wrapper for it in Rust.

@mx00s
Copy link
Collaborator Author

mx00s commented Dec 27, 2020

Users of this crate would need to know at compile-time exactly what ranges they'll need. Do you have a list of ranges you'll need for your project? Maybe that would help us find some reasonable compromises.

@mx00s
Copy link
Collaborator Author

mx00s commented Jan 1, 2021

I found a workaround which may be enough to move toward a release. It's not as satisfying as what you were aiming for, but it works. See U007D/arranged@b196cf7 (proc-macro branch).

EDIT: the branch name is a misnomer. Originally I was going to use a proc-macro, but realizes macro_rules would suffice.

@mx00s
Copy link
Collaborator Author

mx00s commented Jan 31, 2021

Looks like you may have resolved this with your recent push. I haven't verified it myself yet and can't right now, but feel free to close this if it's all set.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants