rust-lang / rust

Empowering everyone to build reliable and efficient software.
https://www.rust-lang.org
Other
98.2k stars 12.7k forks source link

Tracking issue for future-incompatibility lint `invalid_type_param_default` #36887

Open petrochenkov opened 8 years ago

petrochenkov commented 8 years ago

This is the summary issue for the invalid_type_param_default future-compatibility warning and other related errors. The goal of this page is describe why this change was made and how you can fix code that is affected by it. It also provides a place to ask questions or register a complaint if you feel the change should not be made. For more information on the policy around future-compatibility warnings, see our breaking change policy guidelines.

What is the warning for?

Type parameter defaults outside of type declarations were never intended to be permitted, but some early versions of Rust did accept them. For example:

struct Foo<T=i32> { 
    // the default of `i32` is legal here, since this
    // is a type declaration
}

impl<T=i32> Debug for Foo<T> { .. }
//   ^^^^^ default not legal here, in an impl

fn bar<T=i32>(x: T) { }
//     ^^^^^ default not legal here, in a fn

When will this warning become a hard error?

At the beginning of each 6-week release cycle, the Rust compiler team will review the set of outstanding future compatibility warnings and nominate some of them for Final Comment Period. Toward the end of the cycle, we will review any comments and make a final determination whether to convert the warning into a hard error or remove it entirely.

Current status

petrochenkov commented 8 years ago

@nikomatsakis, could you write a description for this?

nikomatsakis commented 8 years ago

@petrochenkov done.

nrc commented 7 years ago

Where can I find discussion of this issue? I don't see any on RFC 213, nor its tracking issue (#27336).

nikomatsakis commented 7 years ago

@nrc perhaps I should have phrased the summary with slightly differently. I think the main thing is that defaults were being accepted in stable code but ignored, unless you opt in to a feature gate, which gives them some semantics (but semantics that I now think I disagree with).

RReverser commented 7 years ago

Is there a reason this is being phased out on functions?

It's sometimes useful to have fn do_smth<A, B=A>(a: A, b: B) { ... }.

nikomatsakis commented 7 years ago

@RReverser Yes. Because, IIRC, they are basically ignored on stable Rust. On nightly Rust, meanwhile, we were considering various possible meanings, but have not yet found one that we are satisfied with.

RReverser commented 7 years ago

Ok, I'm now curious... where can I read about other possible meanings? The expectation seems pretty straightforward, just like in case with struct - that with fn do_smth<T=SomeType>() {}, do_smth() would be equivalent do do_smth::<SomeType>() but maybe I'm missing some issues?

randomPoison commented 7 years ago

I'm also curious as to what the issue is for default type arguments in generic functions. I'm running into a case where it would greatly help ergonomics of using a function with an optional generic parameter: https://is.gd/Gy9bXW

est31 commented 7 years ago

I think this feature is very useful for API evolution e.g. when you add a generic type parameter and want to keep stability, and I think much better than inferring the type instead.

Apparently there are some issues with the implementation. Can they maybe be fixed instead of removing the feature? Would that require an RFC?

ExpHP commented 6 years ago

I think I see what the difficulty is. Bear with me:

The expectation seems pretty straightforward, just like in case with struct - that with fn do_smth<T=SomeType>() {}, do_smth() would be equivalent do do_smth::<SomeType>()

So, yes, this is exactly how it works for types and traits, and it is exactly what we don't want for functions. Let's look at @randomPoison's example. Here's what we would love to be able to write, correct?

fn main() {
    let _ = foo(Some("I'm a string"));
    let _ = foo(None);  // ERROR: type annotation needed
}

pub fn foo<S = String>(opt: Option<S>) -> String
    where S: Into<String>
{
    opt.map(Into::into).unwrap_or_default()
}

Okay, now, let's assume generic type parameters for functions worked just like those for types. Turns out this code still wouldn't compile. Why? Because it would be equivalent to this:

fn main() {
    // nothing was specified, so use the default
    let _ = foo::<String>(Some("I'm a string")); // ERROR: &str vs String

    // nothing was specified, so use the default
    let _ = foo::<String>(None);
}

when what we really want is this:

fn main() {
    // infer the type *if possible*...
    let _ = foo::<_>(Some("I'm a string"));

    // ...otherwise, use a default
    let _ = foo::<String>(None);
}

which appears to require a far greater application of pixie dust.

RReverser commented 6 years ago

@exphp Personally I'd rather prefer the explicit default to be used in that example over the contextual inference, although I can see some people wanting the opposite. More common usecase would be where type can't be inferred at all, just like currently used for optional type params in structs like HashMap.

randomPoison commented 6 years ago

@ExpHP That's a good point. I guess what I really wanted was more of a default type hint, i.e. "always try to infer the correct type, and if there's not enough information to infer a type, use the specified type as the default. That's probably a different feature than the one being discussed in this issue, though.

petrochenkov commented 6 years ago

@randomPoison

I guess what I really wanted was more of a default type hint, i.e. "always try to infer the correct type, and if there's not enough information to infer a type, use the specified type as the default".

It's not clear how exactly this should be done, and this is the single most important blocker for progress on this issue and all related features (e.g. https://github.com/rust-lang/rfcs/pull/2176). See https://github.com/rust-lang/rust/issues/27336 (including links to other threads) for some history.

burtonageo commented 5 years ago

Is there a plan to make this work for generic associated types e.g. in the following code (playground link)? At the moment, this trips the invalid_type_param_default lint.

#![feature(generic_associated_types)]

use std::ops::Deref;

trait SomeTrait {
    type Assoc<T = i32>: Deref<Target = T>;
}

fn main() {
}
mankinskin commented 4 years ago

@ExpHP

fn main() {
    let _ = foo(Some("I'm a string"));
    let _ = foo(None);  // ERROR: type annotation needed
}

pub fn foo<S = String>(opt: Option<S>) -> String
    where S: Into<String>

I don't think this is the point of using default type parameters in functions. In your example it could easily be deduced what the type of foo(None) should be, simply the same as foo::<String>::(None), as the type parameter was omitted and the default kicks in.

I have a use-case where I want to be able to apply a default value to a trait impl:

enum FilterOp<T=()> {
    Binary(T),
    Unary
}
trait Filter<T=()> {
    fn filter(self, field: &str, op: FilterOp<T>) -> Self;
    ...
}

impl<T=()> Filter<T> for Query {
    fn filter(self, field: &str, op: FilterOp<T>) -> Self {
        ...
    }
}

And then use the function filter with FilterOp::Unary:

query.filter("field", FilterOp::Unary) // should use default -> FilterOp<()>

But for some reason filter doesn't apply the default for FilterOp<T> and it is not allowed to define a default in the trait implementation of Filter<T>.

I have opened a thread on the rust-lang user forum: here

ExpHP commented 4 years ago

In your example it could easily be deduced what the type of foo(None) should be, simply the same as foo::<String>::(None), as the type parameter was omitted and the default kicks in.

My point was that foo(Some("I'm a string")); would suddenly stop working for the same reason.

impl<T=()> Filter<T> for Query {
    fn filter(self, field: &str, op: FilterOp<T>) -> Self {
        ...
    }
}

Defining a default in a impl seems like a wild idea. Impls could come into play for all sorts of reasons, and before long you could have two impls trying to specify conflicting defaults for the same type parameter. (this can happen with functions too, but for impls its more subtle because they are not explicitly named in the function body!)

But for some reason filter doesn't apply the default for FilterOp<T> and it is not allowed to define a default in the trait implementation of Filter<T>.

The reason FilterOp::Unary doesn't use the type default is because defaults currently only apply to types, not expressions. FilterOp in this case is only an ident in a path expression. Surrounding it in angle brackets (for UFCS) however would force it to be parsed as a type:

written            equivalent
FilterOp::Unary    FilterOp::<_>::Unary
<FilterOp>::Unary  <FilterOp<()>>::Unary

Like it or not, I'm pretty sure it would be a breaking change to make the former behave like the latter, as there may be code out there which currently uses an enum isomorphic to FilterOp, but where the variant similar to Unary is used for T not necessarily equal to the default. (inferred by later function calls)

jhpratt commented 3 years ago

For what it's worth, having a default type parameter on a function would be incredibly useful for what I'm trying to accomplish. I'm currently trying to use typestate to statically guarantee a format description is valid (for the time crate). Part of this is the parsing of the format description; the parsing itself depends on what it'll be used for (this should be inferred). As a component may or may not be present (think true/false in const generics), the compiler currently can't determine what type should be used for this generic parameter. A default would resolve this issue.

tbraun96 commented 3 years ago

Please keep/allow default type params on functions

MagicRB commented 3 years ago

1 up for default function parameters, especially useful for Enums where only some variants include the generic types...

xNxExOx commented 3 years ago

I have bit different example:

fn some_fn<T: MyTrait = DefaultMyTrait>(text: &str, another_param: usize) -> Result<(), ()>{
    T::check_text(text);
    // some more operations
    Ok(())
}

in this case there can not be mistakes about type interference because T can NOT be interfered. How probable is that this case will stay working?

@petrochenkov "The goal of this page is describe why this change was made and how you can fix code that is affected by it." am I missing the part "how you can fix code that is affected by it", or am I just bad reader and it is already explained here?

nikomatsakis commented 3 years ago

@xNxExOx there really isn't a fix apart from removing the default. I'm not sure how likely it is that this case in particular will keep working -- we may never stabilize defaults outside of type declarations. I hadn't considered the idea of spearating out "type parameters that do not appear in the parameter list", though certainly one could do so.

xNxExOx commented 3 years ago

Well I find a way how to change my code to use iterator like structures instead, so I can provide the defaults in another way (not sure if it was best decision yet). But my questions about the case I write stands, just not a priority anymore, did I understood it correctly that this default is safe to use, and does not have any unexpected behaviors? And from your answer it seems like this way of avoiding code duplicity was not even considered, so it is good to offer a case that might be useful.

nikomatsakis commented 3 years ago

@xNxExOx I don't see any particular problems from your example, correct. And yes, thanks for raising it! It's a useful pattern to consider.

ohsayan commented 3 years ago

I think having type params in fns is very useful. For example, think about a situation where:

pub trait FromBytes {
    fn from_bytes(inp: &[u8]) -> Self;
}

pub fn get_from_bytes<T: FromBytes = Vec<u8>>(inp: &[u8]) -> T {
    // ... do something ...
    T::from_bytes(inp)
}

So, for the get_from_bytes this can be extremely useful: if the user doesn't pass any type param for this fn, then we can just return Vec<u8>, else we can return whatever type param was passed:

let myslice = &somevar[..];
let x = get_from_bytes(myslice); // <- x gets a type of `Vec<u8>`
let y: MyFancyType = get_from_bytes(myslice); // <- y gets a custom type implementing `FromBytes`

I don't see why we want to remove this. The consequence of removing this would be to force the user to assign a type to x as in the above example. This would be a nice thing to have

(Similar to this comment)

ExpHP commented 3 years ago

I don't see why we want to remove this.

This functionality isn't being removed because it never existed in the first place.

https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=88aab65540b9a215ae0c67c74312516c

   Compiling playground v0.0.1 (/playground)
error[E0282]: type annotations needed
  --> src/main.rs:33:9
   |
33 |     let x = get_from_bytes(myslice); // <- x gets a type of `Vec<u8>`
   |         ^ consider giving `x` a type

The only change is that the compiler now rejects this syntax, rather than ignoring it.

ohsayan commented 3 years ago

I don't see why we want to remove this.

This functionality isn't being removed because it never existed in the first place.

https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=88aab65540b9a215ae0c67c74312516c

   Compiling playground v0.0.1 (/playground)
error[E0282]: type annotations needed
  --> src/main.rs:33:9
   |
33 |     let x = get_from_bytes(myslice); // <- x gets a type of `Vec<u8>`
   |         ^ consider giving `x` a type

The only change is that the compiler now rejects this syntax, rather than ignoring it.

Better put, it's something that would be a good thing to have.

AljoschaMeyer commented 3 years ago

Trying to use a default parameter in a generic associated type (via #![feature(generic_associated_types)]) points to this issue, although the discussion here seems to be focused on function parameters. Any chance that default parameters for GATs could become supported even if they are rejected for functions?

Example use case: allocator-aware smart pointers that mimic the API patterns of the standard library:

pub trait SmartPointer {
    type P<T, A: Allocator = Global>;

    fn new<T>(t: T) -> Self::P<T>;

    fn new_in<T, A>(t: T, alloc: A) -> Self::P<T, A>;
}
rouanth commented 2 years ago

I was playing around with making a wrapper that performed mutations and then returned the mutated argument, so that some elegant one-liners could be written for tests.

Without the default type parameters, I fail to imagine a definition such that both lines using mutated compile without extra type annotations.

fn mutated<T, F, S: ?Sized = T>(mut t: T, mut f: F) -> T where F: FnMut(&mut S), T: AsMut<S> {
    f(t.as_mut());
    t
}

fn main() {
    let my_vec1 = mutated(vec![0, 3, 2, 1], |v| { v.sort() });
    let my_vec2 = mutated(vec![0, 3, 2, 1], <[i32]>::sort);
    println!("{:?}", my_vec1); // [0, 1, 2, 3]
}

Now that I think of it, this function has limited use anyway, given how few few implementations of the form T: AsMut<T> there are, but the use case of using default parameters for suggesting a type boundary for intermediate values in a higher-order function should hold anyway.

burrbull commented 2 years ago

Any chance this restriction to be removed for functions?

Starwort commented 1 year ago

I'd also like this restriction to be lifted for functions, especially for trait functions. It would be much more ergonomic to have an API which allows default type parameters; For example, Iterator::collect could default to Vec<T>, while still allowing to be overridden by either type inference or an explicit turbofish; additionally Iterator::sum could be optimised for the common case where S = T (e.g. usize -> usize); other use-cases where an obvious default could be picked should be fairly common

rambip commented 1 year ago

It is especially usefull for optional functions:

fn foo<F: Fn() -> String>(bar:Option<F>){}

fn main(){
    foo(Some(|| String::new()));
    foo(None)
}

Here, I have to use turbofish syntax even if it is irrelevant

If I do:

fn foo<F: Fn() -> String=fn() -> String>(bar:Option<F>){}

fn main(){
    foo(Some(|| String::new()));
    foo(None)
}

Now it compiles !

gsteinLTU commented 1 year ago

For example, Iterator::collect could default to Vec<T>

I have to wonder how much time this would have saved people if it was like this from the start.

rambip commented 1 year ago

And in essence, this is just a hint for the compiler saying "instead of raising type annotation needed for Foo, just try to replace with some concrete type to see if that works" Maybe it is a completely different feature and a new issue should be open (default type parameters can create a bunch of strange situations), but it would be very cool to have this feature.

QuineDot commented 7 months ago

Default type parameters on functions are not completely ignored. Inference doesn't fallback to them just like default type parameters elsewhere, and there are less workarounds because you can't name the types of functions items.

But this works, for example:

#[allow(invalid_type_param_default)]
fn partial_default<T, U: Default = String>() -> U {
    U::default()
}

fn main() {
    let x = partial_default::<()>();
    println!("{}", std::any::type_name_of_val(&x));
}

Because if you have one or more non-defaulted parameters, you can omit defaulted parameters in a turbofish, and (unlike when all parameters are elided / when there is no turbofish), the elided parameters will take on their default values instead of being inference variables.

This is required for things like Vec<Element, Allocator> to work as intended with the default allocator, but happens to work for functions and probably other things, as well.

RalfJung commented 3 months ago

FWIW I did a crater run in https://github.com/rust-lang/rust/pull/127655 and there's still quite a few crates affected by this lint -- mostly because of typemap.

tjpalmer commented 2 months ago

It is especially usefull for optional functions:

fn foo<F: Fn() -> String>(bar:Option<F>){}

fn main(){
    foo(Some(|| String::new()));
    foo(None)
}

Here, I have to use turbofish syntax even if it is irrelevant

Similar problem for things like Option<impl ToString>. Having to say something like None::<String> is sort of silly.

EndilWayfare commented 1 month ago

euclid is another example of a class of use-case in which this is useful for ergonomic/noise-reduction reasons.

The geometric primitives from this crate have an "optional" type parameter to specify unit of measure, but it's not really optional. You don't have to make active use of it, but you do have to mention it in your client code regardless.

use euclid::UnknownUnit;
// Equivalent to 
// `type Point2D<T> = euclid::Point<T, UnknownUnit>;`
use euclid::default::Point2D;

pub fn my_silly_point<U>() -> euclid::Point2D<i32, U> {
    Point2D::new(42, 21)
}

pub fn also_my_silly_point() -> Point2D<i32> {
    my_silly_point()
}

fn no_units_pls() {
    let leaky_implementation_details = my_silly_point::<UnknownUnit>();
    let strange_and_awkward = my_silly_point::<()>();

    let but_i_can = also_my_silly_point();
}

fn im_already_using_units() {
    struct ScreenSpace;

    let center = euclid::Point2D::<i32, ScreenSpace>::new(2560, 1440) / 2;

    let i_can = center - my_silly_point();

    let but_i_have_to = center - also_my_silly_point().cast_unit();
    let or_maybe_for_some_reason = center - euclid::Point2D::from_untyped(also_my_silly_point());
}

I don't know why euclid chooses to uses a type alias instead of a default type parameter, or even if this is even a deliberate design decision. But, if a crate author decides they don't like default type parameters and you do like default type parameters, functions-with-default-type-parameters would let you have/emulate them without, e.g., a redundant wrapper struct.

RalfJung commented 1 month ago

Yeah default type params in type aliases are common. AFAIK they do not cause any kind of inference fallback so they do not share the issues of what is being tracked here.

So for code that currently hits this future compat error, that seems like a good thing to migrate to.