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

Add safe integral auto-coercions #225

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
6 participants
@tommit
Copy link

tommit commented Sep 3, 2014


# Drawbacks

?

This comment has been minimized.

@lilyball

lilyball Sep 3, 2014

Contributor

Drawback: silent conversions are confusing.

fn bar(x: u32) { /* ... */ }
fn foo(x: u32) {
    let y = x + mystery_func(); // clearly a u32, right? It's derived from x
    // ... code goes here
    bar(y); // error, argument must be u32?
}

This comment has been minimized.

@tommit

tommit Sep 3, 2014

Author

What is the type of bar()?

This comment has been minimized.

@lilyball

lilyball Sep 3, 2014

Contributor

Oops, I re-used the name bar(), that's a bit confusing. I'll fix it. I meant for the first call to bar() to be some unknown function with an unknown return value. The expectation is if the line let y = x + bar() compiles, then bar()'s return value must be compatible with x, which is a u32, and addition on integral types is known to return the receiver type.

This comment has been minimized.

@tommit

tommit Sep 3, 2014

Author

Ok, but let's say that mystery_func() returns u64. Today the compiler would give the error (mismatched types: expected u32, found u64) on the line: let y = x + mystery_func();. If this RFC lands, then the compiler would give the same error on the line: bar(y);. I don't see this (the location of the error) as a big issue; after all, you can fix your code by down-casting either mystery_func() or y to u32, so it's not that clear what the best place for the error is anyway.

This comment has been minimized.

@lilyball

lilyball Sep 4, 2014

Contributor

The problem is it divorces the reported error from the actual error. The line bar(y); is not the bad code. The line let y = x + mystery_func(); is the bad code. Yes, you can get the same issue today merely by using a bad type, e.g. if bar() took x: i32 instead of x: u32. But in the cited example, it looks straightforward, it looks like the typing should all match, and the error on bar(y); is therefore confusing.

@cmr

This comment has been minimized.

Copy link
Member

cmr commented Sep 3, 2014

@zwarich and I discussed this at some length a few weeks ago. We came to the conclusion that we didn't think there would be any problems with widening within signedness, but that changing signedness could be confusing, especially in the face of overflow. I can't find the IRC logs from it, he may recall more, but I remember wanting to find numbers of something. We were discussing it in the context of int/uint always being u32/i32 (ie, using an LP64 model). Perhaps we wanted to know how many uses would need explicit casts.

@tommit

This comment has been minimized.

Copy link
Author

tommit commented Sep 3, 2014

Regarding my "Unresolved Questions" section, at least the following auto-coercions should be safe: from i8 to int and from u8 to uint. But can we do better? What kind of guarantees do we have about int and uint?

@petrochenkov

This comment has been minimized.

Copy link
Contributor

petrochenkov commented Sep 3, 2014

An example where the conversions, if allowed, lead to unintended result:

use std::iter::AdditiveIterator;
fn silly_hash(string: &str, modulo: u32) -> u32 {
    string.bytes().sum() % modulo
}

Anyway, the idea probably comes to mind to every person who encounters Rust for the first time, but the conversions are still prohibited. Wasn't it discussed at least for a dozen times and rejected?

@tommit

This comment has been minimized.

Copy link
Author

tommit commented Sep 3, 2014

@petrochenkov How does your example demonstrate my RFC leading to an unintended result?

Just to break it down... your example's silly_hash function takes a string and a modulo value, creates an iterator over the bytes the string consists of, computes the sum of those byte-values and returns that sum mod the given modulo value. The thing that may (and probably will) lead to unintended results with that function is the fact that .sum() returns the sum of the byte-values as u8 and therefore it easily overflows.

Today you'd have to write string.bytes().sum() as u32 % modulo (to get it to compile) which is more difficult to read and doesn't fix the aforementioned overflow-issue with the function.

@petrochenkov

This comment has been minimized.

Copy link
Contributor

petrochenkov commented Sep 4, 2014

@tommit If compiler wants you to write as u32 you see the error and can change sum() to something more appropriate, otherwise the error slips unnoticed.

@petrochenkov

This comment has been minimized.

Copy link
Contributor

petrochenkov commented Sep 4, 2014

Example 2: Interaction with traits.

trait Foo {
    // Conversion in the first argument
    fn foo(self) {}
    // Conversion in the non-first argument
    fn baz(self, a: u32) {}
}

impl Foo for u32 {}

0u8.foo(); // Should it work?
Foo::foo(0u8); // Should it work?
0u32.baz(0u8); // Should it work?

trait Bar {
    fn bar(self) {}
}

// Conversion ranks!
impl Bar for u16 {}
impl Bar for u32 {}

0u8.bar(); // Should it work?

// "Addition"
trait Add_<RHS, Result> {
    fn add_(&self, rhs: &RHS) -> Result;
}
impl Add_<u32, u32> for u32 {
    fn add_(&self, rhs: &u32) -> u32 { 0 }
}

0u32.add_(&0u8); // Should it work?
0u8.add_(&0u32); // Should it work?
// Conceptually the operator+ works through the trait, which is identical to Add_, should it work with mixed types?
0u8 + 0u32; // Should it work?
0u32 + 0u8; // Should it work?

Of course, it is possible to specify all the cases, but then built-in types become special, built-in operators become special, initializations of variables and function arguments may become different, rules become more complex, hello C++.
And what are the profits? Widening conversions are relatively rare compared to "dangerous" uint <-> int conversions, which create more noise, but can't be made implicit.
I suppose it's better idea to emulate some of the conversions by addidional traits implementations like

impl Add<u8, u32> for u32

but I'm not sure about it either.
In general, I think explicit conversions is one of the good features Rust has.

@tommit

This comment has been minimized.

Copy link
Author

tommit commented Sep 4, 2014

@petrochenkov Ok, I can see how getting the error message in the silly_hash example would help in catching that overflow-bug. Therefore I'm not going to go too much into detail about the questions raised in your follow-up example.

That said, I do think the interplay between traits and auto-coercible integral types could work and that some more magic in the built-in integral types wouldn't make the language that much more complicated. Numeric types are already a bit special in that they, unlike user-defined types, can be cast using the as expression. The only thing in your Example 2 that shouldn't compile in my opinion is the line:
0u8.bar();
That line should cause an ambiguity error. But, the code would work if you added this line:
impl Bar for u8 {}

But, since I do think that helping to catch that silly_hash bug is important, perhaps I should try to address explicitly the particular issues I have with explicit casts:

  1. Definite-sized vectors should auto-coerce their size argument (given that it's a positive value)
static n: u8 = 10;
let mut a = [111i, ..n as uint]; // explicit cast just visual noise
  1. Indexing operators should be generalized to take any unsigned integral instead of uint as subscript
// continuing from the previous code snippet
let i: u8 = 5;
a[i as uint] = 222; // explicit cast just visual noise
@petrochenkov

This comment has been minimized.

Copy link
Contributor

petrochenkov commented Sep 5, 2014

@tommit I'd suggest just using uint for sizes and indexes, as the standard library does. Of course, there are some exceptions, but it reduces the conversion noise to minimum.

@alexcrichton alexcrichton force-pushed the rust-lang:master branch from 6357402 to e0acdf4 Sep 11, 2014

@aturon aturon force-pushed the rust-lang:master branch from 4c0bebf to b1d1bfd Sep 16, 2014

@brson

This comment has been minimized.

Copy link
Contributor

brson commented Sep 18, 2014

Thank you for the RFC @tommit but we're not interested in adding such coercions right now.

@brson brson closed this Sep 18, 2014

@P1start P1start referenced this pull request Nov 12, 2014

Closed

Non-uint array indexing #18878

@Yamakaky

This comment has been minimized.

Copy link

Yamakaky commented Nov 13, 2014

At least why not allow all unsigned number types to index an array ? See the issue above.

wycats pushed a commit to wycats/rust-rfcs that referenced this pull request Mar 5, 2019

Merge pull request rust-lang#225 from status200/ember-engines-mount-p…
…arams

Allow {{mount}} syntax to accept parameters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.