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

Could not compile lexical-core on latest nightly, possibly a type checker issue? #81654

Closed
HTGAzureX1212 opened this issue Feb 2, 2021 · 23 comments · Fixed by #81727
Closed
Labels
C-bug Category: This is a bug. regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Milestone

Comments

@HTGAzureX1212
Copy link
Contributor

I am using the latest nightly compiler build. When it got to compiling the lexical-core 0.7.4 crate:

I expected to see this happen: the compilation should be completed with no errors, as with the previous nightly builds

Instead, this happened: 27 errors from the crate spat on the screen.

Meta

rustc --version --verbose:

rustc 1.51.0-nightly (d4e3570db 2021-02-01)
binary: rustc
commit-hash: d4e3570db4c007089035b833cc20c7fc2f8cb32f
commit-date: 2021-02-01
host: x86_64-pc-windows-msvc
release: 1.51.0-nightly
LLVM version: 11.0.1

I am not sure if it had been a bug in the type-checker, as the compilation doesn't fail on previous nightly compilers , only ocurred in the newest nightly.

The related issue on lexical-core: Alexhuszagh/rust-lexical#55

@HTGAzureX1212 HTGAzureX1212 added the C-bug Category: This is a bug. label Feb 2, 2021
@HTGAzureX1212
Copy link
Contributor Author

Some errors:

error[E0308]: mismatched types
  --> C:\Users\<user>\.cargo\registry\src\git.luolix.top-1ecc6299db9ec823\lexical-core-0.7.4\src\atof\algorithm\bhcomp.rs:62:24
   |
62 |     let bytes = bits / Limb::BITS;
   |                        ^^^^^^^^^^ expected `usize`, found `u32`

error[E0277]: cannot divide `usize` by `u32`
  --> C:\Users\<user>\.cargo\registry\src\git.luolix.top-1ecc6299db9ec823\lexical-core-0.7.4\src\atof\algorithm\bhcomp.rs:62:22
   |
62 |     let bytes = bits / Limb::BITS;
   |                      ^ no implementation for `usize / u32`
   |
   = help: the trait `Div<u32>` is not implemented for `usize`

@maekawatoshiki
Copy link
Contributor

maekawatoshiki commented Feb 2, 2021

I have the exactly same issue.

@jonas-schievink jonas-schievink added the regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. label Feb 2, 2021
@rustbot rustbot added the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Feb 2, 2021
@sbnl
Copy link

sbnl commented Feb 2, 2021

Same issue.
rustc --version; cargo --version; rustup --version

rustc 1.51.0-nightly (d4e3570 2021-02-01)
cargo 1.51.0-nightly (c3abcfe8a 2021-01-25)
rustup 1.23.1 (3df2264a9 2020-11-30)

@m-ou-se
Copy link
Member

m-ou-se commented Feb 2, 2021

This is failing because <integer>::BITS was added to the standard library and stabilized yesterday: #76904

This constant is a u32 in the standard library, but Limb::BITS used to resolve to a usize constant associated to a trait. The code now fails because it gets an u32 where it expects a usize.

Unfortunately, breaking things like this is always a risk when stabilizing new methods or constants.

@HTGAzureX1212
Copy link
Contributor Author

So would it be more of an issue on lexical-core side?

@m-ou-se
Copy link
Member

m-ou-se commented Feb 2, 2021

It wasn't really a bug in lexical-core or any other crate. This is just what can happen when adding new names to the standard library. There is never a guarantee that Type::something will always resolve to the same trait, since types can always add their own associated constants and methods.

lexical-core could fix this issue by explicitly using <Limb as SomeTrait>::BITS instead.

Depending on how big the fallout is, we might want to (temporarily) roll the stabilization of <integer>::BITS back.

@HTGAzureX1212
Copy link
Contributor Author

Makes sense.

@m-ou-se
Copy link
Member

m-ou-se commented Feb 2, 2021

rustc does warn for potential breakage before things are stabilized:

warning: a method with this name may be added to the standard library in the future
    --> src/atof/algorithm/math.rs:1909:34
     |
1909 | const ALGORITHM_D_B: Wide = 1 << Limb::BITS;
     |                                  ^^^^^^^^^^
     |
     = note: `#[warn(unstable_name_collisions)]` on by default
     = warning: once this method is added to the standard library, the ambiguity may cause an error or change in behavior!
     = note: for more information, see issue #48919 <https://github.com/rust-lang/rust/issues/48919>
     = help: call with fully qualified syntax `util::num::Integer::BITS(...)` to keep using the current method

However, those warnings are only visible when comping the crate directly (or as a local dependency), not when compiling it as an remote dependency, to avoid flooding people with warnings for crates they are not working on.

@HTGAzureX1212
Copy link
Contributor Author

I wished I could just clone the crate and make the changes myself, but the only issue is it is a dependency used by sqlformat which is a dependency used by sqlx. Is there some way to force sqlformat to use my local version instead of the remote version on crates.io?

@m-ou-se
Copy link
Member

m-ou-se commented Feb 2, 2021

@HTG-YT Yes, that should be possible with a [patch] section in your Cargo.toml: https://doc.rust-lang.org/cargo/reference/overriding-dependencies.html#the-patch-section

@HTGAzureX1212
Copy link
Contributor Author

Thanks

@catenacyber
Copy link

@HTG-YT I have the same problem. Did you do a patch in your Cargo.toml ?

@HTGAzureX1212
Copy link
Contributor Author

Yes, I cloned the repository of lexical-core and made the fixes myself.

@catenacyber
Copy link

Do you have a pointer to this fix ?

@camelid camelid added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Feb 3, 2021
@trevyn
Copy link
Contributor

trevyn commented Feb 3, 2021

@catenacyber See Alexhuszagh/rust-lexical#56

Looks like the maintainer of lexical-core hasn't been active on GitHub since March, this is going to be fun.

@trevyn
Copy link
Contributor

trevyn commented Feb 3, 2021

In the meantime, you can pop this in your root Cargo.toml:

[patch.crates-io]
lexical-core = {git = 'https://github.com/Gelbpunkt/rust-lexical', branch = 'fix-warnings-and-update-deps'}

@m-ou-se
Copy link
Member

m-ou-se commented Feb 3, 2021

We are reverting stabilization of ::BITS for now, until this is solved upstream.

@Mark-Simulacrum Mark-Simulacrum added this to the 1.51.0 milestone Feb 3, 2021
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Feb 3, 2021
…mulacrum

Revert stabilizing integer::BITS.

We agreed in the libs meeting just now to revert stablization, since the [breakage](rust-lang#81654) is significant throughout the ecosystem, through `lexical-core`.

cc rust-lang#76904

Fixes rust-lang#81654
m-ou-se added a commit to m-ou-se/rust that referenced this issue Feb 4, 2021
…mulacrum

Revert stabilizing integer::BITS.

We agreed in the libs meeting just now to revert stablization, since the [breakage](rust-lang#81654) is significant throughout the ecosystem, through `lexical-core`.

cc rust-lang#76904

Fixes rust-lang#81654
m-ou-se added a commit to m-ou-se/rust that referenced this issue Feb 4, 2021
…mulacrum

Revert stabilizing integer::BITS.

We agreed in the libs meeting just now to revert stablization, since the [breakage](rust-lang#81654) is significant throughout the ecosystem, through `lexical-core`.

cc rust-lang#76904

Fixes rust-lang#81654
thbar added a commit to etalab/transpo-rt that referenced this issue Jul 8, 2021
I'm doing this to work around rust-lang/rust#81654, as suggested over there rust-lang/rust#81654 (comment).
thbar added a commit to etalab/transpo-rt that referenced this issue Jul 8, 2021
* Trigger CI

* Bump up lexical-core

I'm doing this to work around rust-lang/rust#81654, as suggested over there rust-lang/rust#81654 (comment).
KoichiYasuoka added a commit to KoichiYasuoka/tokenizers that referenced this issue Jul 16, 2021
KoichiYasuoka added a commit to KoichiYasuoka/tokenizers that referenced this issue Jul 16, 2021
bors added a commit to rust-lang/crater that referenced this issue Jul 16, 2021
Update lexical-core

This updates lexical-core because the old version no longer builds on nightly due to rust-lang/rust#81654.
Narsil added a commit to huggingface/spm_precompiled that referenced this issue Jul 19, 2021
@jhoobergs
Copy link

I am now a maintainer of the lexical family of crates. lexical-core 0.7.5 is published utilizing Gelbpunkt's fix in Alexhuszagh/rust-lexical#56, and lexical 5.2.1 is published depending on it. This should unblock this issue and resume stabilization, assuming all of lexical's dependents are able to receive patch updates automatically.

In combination with @trevyn 's patch solution I guess the best fix for now is to use?

[patch.crates-io]
lexical-core = {git = 'https://github.com/Alexhuszagh/rust-lexical' }

(I am using some dependencies that use old versions of nom (5.*) and this is why I needed it)

@memoryruins
Copy link
Contributor

memoryruins commented Jul 26, 2021

@jhoobergs nom 5.1.2 has this in its Cargo.toml, which has the fixed version in its range.

[dependencies.lexical-core]
optional = true
version = ">= 0.6, < 0.8"

Trying myself with the following Cargo.toml, cargo brings in lexical-core 0.7.6.

[dependencies]
nom = "5"

It's possible that cargo update -p lexical-core is all that is needed, without the patch, so that your Cargo.lock is updated.
https://doc.rust-lang.org/cargo/commands/cargo-update.html

@jhoobergs
Copy link

Thanks, that was indeed the case. I had tried cargo clean but had forgotten about to lock-file.

n1t0 pushed a commit to huggingface/tokenizers that referenced this issue Aug 12, 2021
* update lexical-core because 0.7.4 doesn't compile

Fix the issue as described in rust-lang/rust#81654

* update lexical-core because 0.7.4 doesn't compile

Fix the issue as described in rust-lang/rust#81654
bkochendorfer added a commit to mozilla-iam/dino-park-lookout that referenced this issue May 3, 2022
hilbert-yaa added a commit to hilbert-yaa/veridian that referenced this issue Feb 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: This is a bug. regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.