Ogeon / palette

A Rust library for linear color calculations and conversion
Apache License 2.0
751 stars 60 forks source link

Compilation error when trying to use palette #283

Closed NeverGivinUp closed 6 months ago

NeverGivinUp commented 2 years ago

When adding any use statement from my local palette crate to my library -- I have not tried using the official version -- I get the following error:

error[E0275]: overflow evaluating the requirement `f64: std::ops::Div<palette::blend::PreAlpha<_>>`
  --> src\text.rs:35:50
   |
35 |         let centering_height_shift = self.height / 2.0;
   |                                                  ^
   |
   = help: consider increasing the recursion limit by adding a `#![recursion_limit = "256"]` attribute to your crate (`web_utils`)
   = note: required because of the requirements on the impl of `std::ops::Div<palette::blend::PreAlpha<palette::blend::PreAlpha<_>>>` for `f64`
   = note: 127 redundant requirements hidden
   = note: required because of the requirements on the impl of `std::ops::Div<palette::blend::PreAlpha<palette::blend::PreAlpha<palette::blend::PreAlpha<palette::blend::PreAlpha<palette::blend::PreAlpha<palette::blend::PreAlpha<palette::blend::PreAlpha<palette::blend::PreAlpha<palette::blend::PreAlpha<palette::blend::PreAlpha<palette::blend::PreAlpha<palette::blend::PreAlpha<palette::blend::PreAlpha<palette::blend::PreAlpha<palette::blend::PreAlpha<palette::blend::PreAlpha<palette::blend::PreAlpha<palette::blend::PreAlpha<palette::blend::PreAlpha<palette::blend::PreAlpha<palette::blend::PreAlpha<palette::blend::PreAlpha<palette::blend::PreAlpha<palette::blend::PreAlpha<palette::blend::PreAlpha<palette::blend::PreAlpha<palette::blend::PreAlpha<palette::blend::PreAlpha<palette::blend::PreAlpha<palette::blend::PreAlpha<palette::blend::PreAlpha<palette::blend::PreAlpha<palette::blend::PreAlpha<palette::blend::PreAlpha<palette::blend::PreAlpha<palette::blend::PreAlpha<palette::blend::PreAlpha<palette::blend::PreAlpha<palette::blend::PreAlpha<palette::blend::PreAlpha<palette::blend::PreAlpha<palette::blend::PreAlpha<palette::blend::PreAlpha<palette::blend::PreAlpha<palette::blend::PreAlpha<palette::blend::PreAlpha<palette::blend::PreAlpha<palette::blend::PreAlpha<palette::blend::PreAlpha<palette::blend::PreAlpha<palette::blend::PreAlpha<palette::blend::PreAlpha<palette::blend::PreAlpha<palette::blend::PreAlpha<palette::blend::PreAlpha<palette::blend::PreAlpha<palette::blend::PreAlpha<palette::blend::PreAlpha<palette::blend::PreAlpha<palette::blend::PreAlpha<palette::blend::PreAlpha<palette::blend::PreAlpha<palette::blend::PreAlpha<palette::blend::PreAlpha<palette::blend::PreAlpha<palette::blend::PreAlpha<palette::blend::PreAlpha<palette::blend::PreAlpha<palette::blend::PreAlpha<palette::blend::PreAlpha<palette::blend::PreAlpha<palette::blend::PreAlpha<palette::blend::PreAlpha<palette::blend::PreAlpha<palette::blend::PreAlpha<palette::blend::PreAlpha<palette::blend::PreAlpha<palette::blend::PreAlpha<palette::blend::PreAlpha<palette::blend::PreAlpha<palette::blend::PreAlpha<palette::blend::PreAlpha<palette::blend::PreAlpha<palette::blend::PreAlpha<palette::blend::PreAlpha<palette::blend::PreAlpha<palette::blend::PreAlpha<palette::blend::PreAlpha<palette::blend::PreAlpha<palette::blend::PreAlpha<palette::blend::PreAlpha<palette::blend::PreAlpha<palette::blend::PreAlpha<palette::blend::PreAlpha<palette::blend::PreAlpha<palette::blend::PreAlpha<palette::blend::PreAlpha<palette::blend::PreAlpha<palette::blend::PreAlpha<palette::blend::PreAlpha<palette::blend::PreAlpha<palette::blend::PreAlpha<palette::blend::PreAlpha<palette::blend::PreAlpha<palette::blend::PreAlpha<palette::blend::PreAlpha<palette::blend::PreAlpha<palette::blend::PreAlpha<palette::blend::PreAlpha<palette::blend::PreAlpha<palette::blend::PreAlpha<palette::blend::PreAlpha<palette::blend::PreAlpha<palette::blend::PreAlpha<palette::blend::PreAlpha<palette::blend::PreAlpha<palette::blend::PreAlpha<palette::blend::PreAlpha<palette::blend::PreAlpha<palette::blend::PreAlpha<palette::blend::PreAlpha<palette::blend::PreAlpha<palette::blend::PreAlpha<palette::blend::PreAlpha<palette::blend::PreAlpha<palette::blend::PreAlpha<palette::blend::PreAlpha<palette::blend::PreAlpha<palette::blend::PreAlpha<_>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>` for `f64`

The error goes away, when I comment out the line impl_scalar_binop!(Div::div, DivAssign::div_assign, [f32, f64]);

I believe is unrelated to my local changes and it certainly is unrelated to the code, where the error occurs. It may be related to a bug in the rust compiler.

Ogeon commented 2 years ago

I had similar errors that came and went away but I don't remember how I fixed it last time.

Which compiler version are you using? Does it change if you switch to another version?

NeverGivinUp commented 2 years ago

I updated to the current nightly during our conversation on the weekend. My code unfortunately requires associated type bounds and does not compile with the stable compiler. How do I use an older nightly compiler?

Ogeon commented 2 years ago

I see, then it's trickier to test with something stable unless it can be minimized into an example without them.

I'm not sure off the top of my head but you may be able to pick the nightly version with rustup.

NeverGivinUp commented 2 years ago

Using an old nightly (in this case the one from 2022-01-01) didn't help. Neither did replacing the division by 2 by a multiplication by 0.5. This was the only line, the compiler didn't like, though. And extreme disambiguation did help

let centering_height_shift = <f64 as Div>::div(self.height, 2.0);
Ogeon commented 2 years ago

What if you write self.height / 2.0f64? Does it still complain?

Ogeon commented 2 years ago

I'm reopening this because there may be something that can mitigate the issue.

NeverGivinUp commented 2 years ago

What if you write self.height / 2.0f64? Does it still complain?

Yes. Also if I create a separate variable with explicit type

let h: f64 = self.height;
h/2.0_f64

Still does not work

NeverGivinUp commented 2 years ago

I'm reopening this because there may be something that can mitigate the issue.

There is an answer in the StackOverflow post, which I originally linked, which seems to contain 2 workarounds. But I didn't read them properly.

Ogeon commented 2 years ago

Ah, so something about how it transforms the division into a function call is getting stuck. It would be nice to have a small test case to keep here and also find out which implementation is the problematic one.

Would you mind sharing the full code or try to reduce it into a minimal test case?

Ogeon commented 2 years ago

I'll have a look at those workarounds later and see if they could help too. Thanks for pointing them out!

NeverGivinUp commented 2 years ago

https://github.com/NeverGivinUp/palette_compilation_rest.git

(was meant to be a compilation test, not a rest)

Ogeon commented 2 years ago

Perfect, thanks! 🙏

TiemenSch commented 1 year ago

This just started hitting my code today. Any multiplication with two f64's trips up one by one. I can't go change all these lines to the ::mul(self, rhs) format, since that would be silly. What would you recommend to do?

Ogeon commented 1 year ago

Ouch, sorry about that. You can try to use variables with explicit types to help the compiler, but it's also not ideal. 😕 You could also see if it depends on the compiler version. I didn't get around to figuring out exactly what triggers it before releasing, so I'm sorry for not having better answers right now, but thanks for highlighting that it's still an issue.

TiemenSch commented 1 year ago

For me, it happens in a projects after running a routinely cargo update which made the following results to my Cargo.lock:

    Updating crates.io index
      Adding fast-srgb8 v1.0.0
    Removing find-crate v0.6.3
    Updating getrandom v0.2.8 -> v0.2.9
    Updating palette v0.6.1 -> v0.7.0
    Updating palette_derive v0.6.1 -> v0.7.0
    Updating ratio-color v0.2.0 -> v0.3.0
    Updating serde v1.0.159 -> v1.0.160
    Updating serde_derive v1.0.159 -> v1.0.160
    Updating simba v0.8.0 -> v0.8.1
    Updating syn v2.0.13 -> v2.0.14
    Removing toml v0.5.11
    Updating uuid v1.3.0 -> v1.3.1

Where ratio-coloris my quickly made palette library depending on palette. The error is exactly the one given regarding the recursion limit suggestion and PreAlpha.

The first offending line in my code is a const multiplication of:

pub const DOUBLE_PI: f64 = 2.0_f64 * std::f64::consts::PI; 

But as soon as I patch that, all f64 multiplications inside function bodies get marked, probably because of the order in which things are compiled.

TiemenSch commented 1 year ago

It's rather uncommon for a library to touch the functionalities of others (or std in that matter), which makes it a rather strange error that should have been prohibited by the orphan rule usually?

Ogeon commented 1 year ago

Indeed, there's no obvious reason why it would be bothered by PreAlpha, unless it tries to somehow evaluate it as a possible candidate in the multiplication. That's what makes your case especially strange.

Do you think it would be possible to make a minimal example of when it fails? It could be cross referenced with the one above to see if there's any pattern.

Also, which compiler version are you using?

Azorlogh commented 1 year ago

I am able to reproduce this the following way:

[package]
name = "bug_palette"
version = "0.1.0"
edition = "2021"

[dependencies]
scad = "1.2.2"
palette = "0.7.0"
// main.rs
use palette::Oklch;
use scad::OffsetType;

fn main() {
    println!("{}", 42.0 * 1.0); // bug also happens when specifying f32 or f64
}
error[E0275]: overflow evaluating the requirement `f32: Mul<PreAlpha<_>>`
 --> src/main.rs:5:25
  |
5 |     println!("{}", 42.0 * 1.0);
  |                         ^
  |
  = help: consider increasing the recursion limit by adding a `#![recursion_limit = "256"]` attribute to your crate (`repro`)
  = note: required for `f32` to implement `Mul<PreAlpha<PreAlpha<_>>>`
  = note: 127 redundant requirements hidden
  = note: required for `f32` to implement `Mul<PreAlpha<PreAlpha<PreAlpha<PreAlpha<PreAlpha<PreAlpha<PreAlpha<PreAlpha<PreAlpha<PreAlpha<PreAlpha<PreAlpha<PreAlpha<PreAlpha<PreAlpha<PreAlpha<PreAlpha<PreAlpha<PreAlpha<PreAlpha<PreAlpha<PreAlpha<PreAlpha<PreAlpha<PreAlpha<PreAlpha<PreAlpha<PreAlpha<PreAlpha<PreAlpha<PreAlpha<PreAlpha<PreAlpha<PreAlpha<PreAlpha<PreAlpha<PreAlpha<PreAlpha<PreAlpha<PreAlpha<PreAlpha<PreAlpha<PreAlpha<PreAlpha<PreAlpha<PreAlpha<PreAlpha<PreAlpha<PreAlpha<PreAlpha<PreAlpha<PreAlpha<PreAlpha<PreAlpha<PreAlpha<PreAlpha<PreAlpha<PreAlpha<PreAlpha<PreAlpha<PreAlpha<PreAlpha<PreAlpha<PreAlpha<PreAlpha<PreAlpha<PreAlpha<PreAlpha<PreAlpha<PreAlpha<PreAlpha<PreAlpha<PreAlpha<PreAlpha<PreAlpha<PreAlpha<PreAlpha<PreAlpha<PreAlpha<PreAlpha<PreAlpha<PreAlpha<PreAlpha<PreAlpha<PreAlpha<PreAlpha<PreAlpha<PreAlpha<PreAlpha<PreAlpha<PreAlpha<PreAlpha<PreAlpha<PreAlpha<PreAlpha<PreAlpha<PreAlpha<PreAlpha<PreAlpha<PreAlpha<PreAlpha<PreAlpha<PreAlpha<PreAlpha<PreAlpha<PreAlpha<PreAlpha<PreAlpha<PreAlpha<PreAlpha<PreAlpha<PreAlpha<PreAlpha<PreAlpha<PreAlpha<PreAlpha<PreAlpha<PreAlpha<PreAlpha<PreAlpha<PreAlpha<PreAlpha<PreAlpha<PreAlpha<PreAlpha<PreAlpha<PreAlpha<PreAlpha<PreAlpha<_>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>`

I'm on latest stable:

stable-x86_64-unknown-linux-gnu (default)
rustc 1.68.2 (9eb3afe9e 2023-03-27)
Ogeon commented 1 year ago

Great, thanks! From this it pretty much looks like it's getting stuck while considering PreAlpha as a possible right-hand-side type. I'm not sure how to stop it, other than deleting the implementation of Mul<PreAlpha<T>> for f32 and so on, but that's the difference I can see from for example Alpha, which hasn't been showing this behavior. I see that there's a lot of bug reports for rustc with similar cases (but not exactly the same in the ones I have checked), so that may be the way to go in the end.

I will give it a proper try this weekend (or tomorrow if I have time)

Ogeon commented 1 year ago

By the way @Azorlogh, does it still fail when removing the scad dependency? It shouldn't be related but I'm curious to see how spooky this issue is.

Azorlogh commented 1 year ago

The issue is quite spooky indeed. Removing any of the use statements makes my example compile fine.
scad::OffsetType itself is just a pretty trivial enum.

Ogeon commented 1 year ago

Definitely spooky but the spookiness also makes it harder to set up a reliable test case for regression. 🤔

Ogeon commented 1 year ago

I found this, that seem to be pretty much the same case: https://github.com/rust-lang/rust/issues/80542

Ogeon commented 1 year ago

I created PR #312 to try to work around this. It works for the example from @Azorlogh, but let me know if it still fails in any other cases. You should be able to change your palette dependency to this to give it a try before it's merged:

[dependencies]
palette = { git = "https://github.com/Ogeon/palette.git", branch = "work_around_issue_283" }
Ogeon commented 1 year ago

The fix has been released in 0.7.1. Feel free to reopen this issue if it appears again.

Strosel commented 7 months ago

This seems to be an issue once again, but now for IntoIterator. My code does not depend on palette directly but yields the following error with no source location:

error[E0275]: overflow evaluating the requirement `&_: IntoIterator`
  |
  = help: consider increasing the recursion limit by adding a `#![recursion_limit = "256"]` attribute to your crate (`...`)
  = note: required for `&palette::lch::Lch<_, _>` to implement `IntoIterator`
  = note: 126 redundant requirements hidden
  = note: required for `&Lch<_, Lch<_, Lch<_, Lch<_, Lch<_, Lch<_, Lch<_, ...>>>>>>>` to implement `IntoIterator`

The specific type varies between compilations, seemingly by random, but so far I've seen OkLabHue, LuvHue, Lch, Rgb, and Luma.

I've tried both cargo clean and building with -Znext-solver but neither works. I've also cloned the repo of the direct dependent on palette which does not yield the error on cargo check or cargo test. I'm at a complete loss as to what is causing the error so any and all help is greatly appreciated.

I'm running the latest stable:

$ rustc -vV
  rustc 1.76.0 (07dca489a 2024-02-04)
  binary: rustc
  commit-hash: 07dca489ac2d933c78d3c5158e3f43beefeb02ce
  commit-date: 2024-02-04
  host: aarch64-apple-darwin
  release: 1.76.0
  LLVM version: 17.0.6
Ogeon commented 7 months ago

Oh, no. 😬 Do you think you could provide a minimal example that reproduces it? I have some idea of where it may be from, but it helps to have an example for testing.

Strosel commented 7 months ago

A minimal example might be quite difficult as the application I'm working on is a decently large GUI using iced (which is the direct dependent of palette)

If you still would like to have a look I could push the current state of my repo to a new branch and send you a link in a couple of hours.

Ogeon commented 7 months ago

That would also be helpful, thanks. Do you know if the problem started after any particular change?

Strosel commented 7 months ago

The problem started when updating to the latest version of iced, I fear that it may be some other bug that causes this and makes the compile go a little haywire.

I committed my working changes here https://github.com/Smalands-Nation/register-rs/tree/iced_migration_palette_bug

Ogeon commented 7 months ago

Thank you. If it's anything like the original issue, it's basically a latent problem that just didn't get triggered until now. A trait requirement that happens to send the trait resolver into a loop when the stars happen to align. I'm asking all these questions because it's so difficult to reproduce it without those triggering circumstances.

Ogeon commented 7 months ago

It's ultimately a compiler bug, and I hoped the next resolver would be able to handle it (it does handle some cases of it), but I'll have to find some workaround that isn't too breaking for now.

Strosel commented 7 months ago

I feared as much...

I've only taken a very quick glance but couldn't this be solved for some to the types by introducing a trait bound on the inner iterator that is not met by the colors themselves.

A hasty example for Rgb could look something like

impl<'a, S, C> IntoIterator for Rgb<S, C>
where
    C: IntoIterator + num_traits::Num,
{
  ...
}

Of course this assumes the inner iterator is never another colortype which I do not know is always the case as I've only skimmed through palette.

Ogeon commented 7 months ago

That's also my prime suspect. The feature that introduced the iterator is the ability to convert an array of colors into a struct of arrays, so Vec<Rgb<S, T>> could become Rgb<S, Vec<T>>. I think something like what you are suggesting could work, but the constraint should probably be something that makes sure it's array-like. Alternatively, since that's surprisingly hard, have specific implementations for Vec, &[T], &mut [T], [T; N], etc. and avoid the risk of recursion entirely.

Strosel commented 7 months ago

Could we not then impose bounds on Item as Rgb<S, Vec<Vec<T>> and similar seem unlikely if not impossible

Another hasty example

impl<'a, S, C> IntoIterator for Rgb<S, C>
where
    C: IntoIterator,
    <C as IntoIterator>::Item: num_traits::Num,
{
  ...
}
Ogeon commented 7 months ago

The library isn't relying on num_traits in its current form, but that would have been something to try otherwise, assuming that's enough to convince the resolver. Specific types may be more robust in general, though, since it doesn't even imply that something like Rgb<_, Rgb<_, f32>> would be a possibility. I have been a bit busy today, so sorry for not jumping onto this immediately, but I will have a closer look tomorrow and see what works.

Ogeon commented 7 months ago

I tried restricting the item type, but it didn't help. I will make implementations for specific collection types instead.

fenhl commented 7 months ago

I'm also running into this while attempting to upgrade iced. Here's a minimal example:

use {
    std::convert::Infallible,
    iced::{
        Application,
        Command,
        Element,
        Settings,
        widget::Column,
    },
};

struct State;

impl Application for State {
    type Executor = iced::executor::Default;
    type Message = Infallible;
    type Theme = iced::Theme;
    type Flags = ();

    fn new((): Self::Flags) -> (Self, Command<Infallible>) { (Self, Command::none()) }
    fn title(&self) -> String { String::default() }
    fn update(&mut self, _: Infallible) -> Command<Infallible> { Command::none() }

    fn view(&self) -> Element<'_, Infallible> {
        Column::with_children(Vec::default().into_iter().map(|()| Column::new()).collect()).into()
    }
}

fn main() -> iced::Result {
    State::run(Settings::default())
}
Ogeon commented 7 months ago

Oh, thanks for the minimal example! I'll see if I can make it into a test later.

Ogeon commented 7 months ago

I have pushed a preliminary work-around as #380. The solution for Alpha isn't great, so I may change it before publishing for real. You can try it out by adding this to your Cargo.toml.

[patch.crates-io]
palette = { git = "https://github.com/Ogeon/palette", branch = "issue_283_into_iterator" }
fenhl commented 7 months ago

With this patch applied, the compiler seems to get stuck (2 minutes of CPU time and counting on the example above, 19 minutes and counting on the original project).

Ogeon commented 7 months ago

That's probably what I thought was slow linking, but I had to cancel it and go. 😬 It could be the Alpha wrapper.

Rrogntudju commented 7 months ago

I upgraded my app to iced 0.12 and i have the same issue with Luv

overflow evaluating the requirement &_: IntoIterator required for &palette::luv::Luv<_, _> to implement `IntoIterator

Ogeon commented 7 months ago

I added a commit with less generic implementations for Alpha, which I got to build after fixing a couple of mistakes in the code @Strosel submitted. Nothing big, just ambiguous output types for collect and attempts to call into in const. Probably masked by this bug.

You should get the new commit with cargo update, as I understand it, if you have the patch from above.

fenhl commented 7 months ago

Looks good!

Ogeon commented 7 months ago

That's great! If nothing more pops up, I'll finish it next time I have time (this week is a bit cramped :grimacing:) and publish it.

Ogeon commented 6 months ago

Version 0.7.5 has been released with the fix. I will remove the temporary branch and you can remove the Cargo.toml patch. Thanks for reporting the error and testing the fix!

max397574 commented 5 months ago

I'm still getting a really similar error to the ones mentioned above for me it's just with Id<Id<Id<...>>>>>

knkski commented 4 months ago

@Ogeon Thanks for the fix. That made it apparent what the issue was. On 0.7.3, I got the recursion error as above, and on 0.7.5 I got an error like this:

error[E0283]: type annotations needed
   --> proj/src/lib.rs:123:68
    |
123 |         Column::with_children(Vec::default().into_iter().map(|()| Column::new()).collect()).into()
    |         ---------------------                                                    ^^^^^^^ cannot infer type of the type parameter `B` declared on the method `collect`
    |         |
    |         required by a bound introduced by this call
    |
    = note: cannot satisfy `_: IntoIterator`
note: required by a bound in `iced::widget::Column::<'a, Message, Theme, Renderer>::with_children`

If I change the code to use .collect::<Vec<_>>(), everything works fine, even on 0.7.3

Ogeon commented 4 months ago

Hi, that looks like a regular ambiguity that can happen with collect. Did you perhaps also update iced at the same time?

I'm assuming this is the function you are calling, which takes a generic type that can be iterated (impl Trait in argument position is the same as T where T: Trait), which doesn't give collect enough information to infer its return type. However, it does look like you can solve this one by not calling collect, since any iterator also implements IntoIterator.