Skip to content

Removed encode-unicode dependency.#125

Closed
markhildreth wants to merge 1 commit intophsym:masterfrom
markhildreth:remove-encode-unicode-dependency
Closed

Removed encode-unicode dependency.#125
markhildreth wants to merge 1 commit intophsym:masterfrom
markhildreth:remove-encode-unicode-dependency

Conversation

@markhildreth
Copy link

@markhildreth markhildreth commented Apr 20, 2020

The reason for attempting to remove this is that encode-unicode provides
some additional iterators that can make some type inferences break by
including it (or a crate that depends on it, like prettytable-rs).

For example, the following line of code works as expected if you include
the rand crate:

use rand::thread_rng;
use rand::distributions::{Distribution, Standard};

fn main() {
  let mut rng = thread_rng();
  let v: Vec<u8> = Standard.sample_iter(&mut rng).take(5).collect();
  println!("{:}", v);
}

However, once prettytable-rs is included and used, the types will no
longer be able to be inferred.

error[E0282]: type annotations needed
  --> src/main.rs:16:31
   |
16 |     let v: Vec<u8> = Standard.sample_iter(&mut rng).take(5).collect();
   |                               ^^^^^^^^^^^
   |                               |
   |                               cannot infer type for type parameter `T`

The reason that this occurs is because prettytable includes encode-unicode, which itself adds additional ways that a Vec<u8> can be created (specifically from an Iterator<Utf8Char>) Since Rust cannot tell if Standard.sample_iter might return an Iterator<Utf8Char> vs an Iterator<u8>, it cannot provide type inference.

To be sure, this is not technically a bug, nor does it even need to be fixed. Type inference breaks are not really considered "breaking changes", but it can be a pain
point. Seeing that prettytable-rs doesn't seem to make much use of the
additional items in encode-unicode, perhaps it's not worth the pain to
users to include it.

The reason for attempting to remove this is that encode-unicode provides
some additional iterators that can make some type inferences break by
including it (or a crate that depends on it, like prettytable-rs).

For example, the following line of code works as expected if you include
the `rand` crate:

```
use rand::thread_rng;
use rand::distributions::{Distribution, Standard};

fn main() {
  let mut rng = thread_rng();
  let v: Vec<u8> = Standard.sample_iter(&mut rng).take(5).collect();
  println!("{:}", v);
}
```

However, once prettytable-rs is included and used, the types will no
longer be able to be inferred.

```
error[E0282]: type annotations needed
  --> src/main.rs:16:31
   |
16 |     let v: Vec<u8> = Standard.sample_iter(&mut rng).take(5).collect();
   |                               ^^^^^^^^^^^
   |                               |
   |                               cannot infer type for type parameter `T`
```

While not technically a problem that NEEDS to be fixed (type inference
breaks are not really considered "breaking changes, it can be a pain
point. Seeing that prettytable-rs doesn't seem to make much use of the
additional items in encode-unicode, perhaps it's not worth the pain to
users to include it.
@codecov
Copy link

codecov bot commented Apr 20, 2020

Codecov Report

Merging #125 into master will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #125   +/-   ##
=======================================
  Coverage   85.89%   85.89%           
=======================================
  Files           5        5           
  Lines        1326     1326           
=======================================
  Hits         1139     1139           
  Misses        187      187           
Impacted Files Coverage Δ
src/format.rs 70.31% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cf7dca3...4abf2c0. Read the comment docs.

@pinkforest
Copy link
Collaborator

Repro has different issue with now stable @ 1.65:

$ cargo run

  Downloaded libc v0.2.139
  Downloaded 1 crate (639.0 KB) in 1.45s
   Compiling libc v0.2.139
   Compiling cfg-if v1.0.0
   Compiling ppv-lite86 v0.2.17
   Compiling getrandom v0.2.8
   Compiling rand_core v0.6.4
   Compiling rand_chacha v0.3.1
   Compiling rand v0.8.5
   Compiling test-pretty v0.1.0 (/home/foobar/cc/test-pretty)
error[E0277]: `Vec<u8>` doesn't implement `std::fmt::Display`
 --> src/main.rs:7:19
  |
7 |   println!("{:}", v);
  |                   ^ `Vec<u8>` cannot be formatted with the default formatter
  |
  = help: the trait `std::fmt::Display` is not implemented for `Vec<u8>`
  = note: in format strings you may be able to use `{:?}` (or {:#?} for pretty-print) instead
  = note: this error originates in the macro `$crate::format_args_nl` which comes from the expansion of the macro `println` (in Nightly builds, run with -Z macro-backtrace for more info

For more information about this error, try `rustc --explain E0277`.
error: could not compile `test-pretty` due to previous error

Switching to appropriate {:?} produces as expected:

cargo run

   Compiling test-pretty v0.1.0 (/home/foobar/cc/test-pretty)
    Finished dev [unoptimized + debuginfo] target(s) in 0.17s
     Running `target/debug/test-pretty`
[227, 6, 247, 165, 134]

This was fixed in upstream I think - closing.

@pinkforest pinkforest closed this Dec 27, 2022
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

Successfully merging this pull request may close these issues.

2 participants