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

Tracking issue for `invalid_type_param_default` compatibility lint #36887

Open
petrochenkov opened this Issue Oct 1, 2016 · 13 comments

Comments

Projects
None yet
10 participants
@petrochenkov
Contributor

petrochenkov commented Oct 1, 2016

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

  • #30724 introduces the invalid_type_param_default lint as warn-by-default
  • #36894 makes the invalid_type_param_default lint deny-by-default
  • #42136 tried to make the invalid_type_param_default lint a hard error, but there was still too much breakage
  • PR ? makes the invalid_type_param_default lint a hard error
@petrochenkov

This comment has been minimized.

Contributor

petrochenkov commented Oct 1, 2016

@nikomatsakis, could you write a description for this?

@nikomatsakis

This comment has been minimized.

Contributor

nikomatsakis commented Oct 11, 2016

@nrc

This comment has been minimized.

Member

nrc commented Nov 28, 2016

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

@nikomatsakis

This comment has been minimized.

Contributor

nikomatsakis commented Nov 28, 2016

@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

This comment has been minimized.

Contributor

RReverser commented Apr 23, 2017

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

This comment has been minimized.

Contributor

nikomatsakis commented Apr 24, 2017

@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

This comment has been minimized.

Contributor

RReverser commented Apr 24, 2017

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?

bors added a commit that referenced this issue Jun 1, 2017

Auto merge of #42136 - petrochenkov:oldhard, r=nikomatsakis
Turn sufficiently old compatibility lints into hard errors

It's been almost 7 months since #36894 was merged, so it's time to take the next step.

[breaking-change], needs crater run.

PRs/issues submitted to affected crates:
alexcrichton/ctest#17
Sean1708/rusty-cheddar#55
m-r-r/helianto#3
azdle/virgil#1
rust-locale/rust-locale#24
mneumann/acyclic-network-rs#1
reem/rust-typemap#38

cc https://internals.rust-lang.org/t/moving-forward-on-forward-compatibility-lints/4204
cc #34537 #36887
Closes #36886
Closes #36888
Closes #36890
Closes #36891
Closes #36892
r? @nikomatsakis

frewsxcv added a commit to frewsxcv/rust that referenced this issue Jun 1, 2017

@randomPoison

This comment has been minimized.

Contributor

randomPoison commented Jun 5, 2017

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

This comment has been minimized.

Contributor

est31 commented Oct 16, 2017

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

This comment has been minimized.

Contributor

ExpHP commented Nov 8, 2017

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

This comment has been minimized.

Contributor

RReverser commented Nov 8, 2017

@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

This comment has been minimized.

Contributor

randomPoison commented Nov 8, 2017

@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

This comment has been minimized.

Contributor

petrochenkov commented Nov 8, 2017

@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. rust-lang/rfcs#2176).
See #27336 (including links to other threads) for some history.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment