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

Allow uncallable method impls to be omitted #1699

Closed
wants to merge 3 commits into from

Conversation

canndrew
Copy link
Contributor

@canndrew canndrew commented Aug 3, 2016

For your consideration.

Rendered

@Stebalien
Copy link
Contributor

Stebalien commented Aug 3, 2016

Note: This has implications beyond !. It would also allow one to write:

trait Foo {
    fn foo(self) where Self: Sized;
}

impl<T> Foo for [T] {}

Currently, this can't even be implemented. This is a bit ridiculous considering the fact that the following impl is fine (because T may be sized):

impl<T: ?Sized> Foo for T {
    fn foo(self) where Self: Sized {}
}

@canndrew
Copy link
Contributor Author

canndrew commented Aug 3, 2016

@Stebalien Interesting, I never thought of cases like that. I was really only thinking of cases where a method takes an uninhabited type.

@withoutboats
Copy link
Contributor

@Stebalien's example seems orthogonal from the RFC to me. In @Stebalien's case, it would be a type error to call that method because a constraint was not met, whereas in the RFC the method is well typed, but will never execute.

@Stebalien's example seems like something that should be allowed, but like the previous RFC, I am not convinced that implementing traits for ! is a frequent enough activity that it deserves any special sugar or consideration.

@nrc nrc added the T-lang Relevant to the language team, which will review and decide on the RFC. label Aug 4, 2016
@nrc
Copy link
Member

nrc commented Aug 4, 2016

@withoutboats it doesn't seem orthogonal to this RFC to me. This RFC states that if a method can't be called, then you don't have to implement it. The example in the RFC and @Stebalien's example are orthogonal reasons for a method being un-callable, but the end result is the same - can't be called => don't need to impl.

@nrc
Copy link
Member

nrc commented Aug 4, 2016

Or to be clear, the examples are different by the rules of the RFC, but not by the spirit.

@withoutboats
Copy link
Contributor

withoutboats commented Aug 4, 2016

@nrc In its motivation and detailed design, this RFC discusses methods which are uncallable because they take an uninhabited argument. I don't think we can consider an RFC which proposes any property for all "methods which can't be called," since that is not a clearly defined set of cases.

For example, here is another case where a method is uncallable. Does this RFC include this in its scope?

A dependency defines and exports this struct and trait:

struct Foo {
    _secret: ()
}

trait Bar {
    fn bar(&self, foo: Foo) -> Foo;
}

The library defines no public constructor for Foo, and contains no functions which take a T: Bar or a Bar trait object as arguments.

When I implement Bar for my own types, the method bar will never be called, because I cannot construct a Foo or pass my type to that library. Moreover, I cannot provide a definition which doesn't diverge, because I cannot construct a Foo.

This is an "uncallable method," should I be allowed not to write it? I don't think so (that opens a huge can of backcompat problems for libraries!), but more importantly, I don't think this is in scope for this RFC.

@Stebalien
Copy link
Contributor

@withoutboats that's only uncallable in safe/sane code. However, I didn't read the detailed design so I simply assumed that the examples given in the motivation/summary were just that, examples.

Given that being able to simply implement my examples is orthogonal to being able to implement them without explicitly defining the uncallable methods, I've filed a separate issue rust-lang/rust#35369.

Also, there's a big drawback that hasn't been mentioned: a type can go from uninhabited to inhabited without changing any public type definition:

struct TBD(!);

trait MyTrait {
    // For now, this always panics...
    fn foo(&self) -> TBD;
}

I don't really see how this could be useful outside of prototyping but it's still an issue.

@withoutboats
Copy link
Contributor

@withoutboats that's only uncallable in safe/sane code.

True, you can construct a Foo through mem::uninitialized.

Also, there's a big drawback that hasn't been mentioned: a type can go from uninhabited to inhabited without changing any public type definition

Also true! I don't think we should make this a breaking change (and I don't think it should be visible to users who don't read the source that TBD is uninhabited).

@nikomatsakis
Copy link
Contributor

cc rust-lang/rust#20021

@nikomatsakis
Copy link
Contributor

Sorry, more context would have been good. =) rust-lang/rust#20021 was an issue opened with respect to where-clauses -- sometimes, in a specific impl, you can tell that a given fn will never be called because its where-clauses cannot be satisfied. This also comes up for trait objects.

Example:

trait Foo { fn bar() where Self: Copy; }
impl Foo for Box<i32> { fn bar() { panic!() } }

Nobody could call x.bar() in the case where x: Box<i32>.

@nagisa
Copy link
Member

nagisa commented Aug 10, 2016

It didn’t occur to me before Niko’s comment, that adding trait implementations for arbitrary types would become a breaking change (which to the best of my knowledge isn’t considered to be one today):

For example:

trait Foo { fn bar() where Self: Copy; }
impl Foo for SomethingNonCopyNow { }

Then somebody comes along and adds #[derive(Copy)] on top of the SomethingNonCopyNow and you got compilation error.

@Stebalien
Copy link
Contributor

@nagisa unless I'm mistaken, coherence ensures that that breakage would be crate-local. IMO, a crate-local error at compile time is preferable to a runtime panic.

@nagisa
Copy link
Member

nagisa commented Aug 10, 2016

@Stebalien You can implement locally defined traits for external types.

IMO, a crate-local error at compile time is preferable to a runtime panic.

I do not disagree here, but I don’t think there’s a case in stable rust where adding a trait implementation upstream can cause previously never called method become called. Specialisation might be one of the features which would make my statement invalid in near future, though. With this feature you will receive an error even if you never use the method.

@Stebalien
Copy link
Contributor

Ah. I didn't think of that case. So,

Crate foo, v1:

pub struct Foo;

Crate bar:

extern create foo;
trait Bar {
    fn bar(&self) where Self: Debug;
}
impl Bar for foo::Foo {
    fn bar(&self) where Self: Debug; // statically impossible
}

Crate foo, v2:

#[derive(Debug)]
pub struct Foo;

One solution might be to allow the function to be written assuming the bound is satisfiable (that is, assuming the type in question does implement the necessary trait). If the bound doesn't actually hold, it will be uncallable. However, if the bound becomes satisfiable in the future, the function will be defined. The major drawback here is that the method is untestable. It may also introduce some other type checking problems... (actually, this would probably cause the compiler team headaches for years to come...).

@nikomatsakis
Copy link
Contributor

@nagisa

It didn’t occur to me before Niko’s comment, that adding trait implementations for arbitrary types would become a breaking change (which to the best of my knowledge isn’t considered to be one today)

That's a really interesting point; I think the key take-away is that we would have to subject the reasoning here to the usual coherence rules about negative reasoning.

@nikomatsakis nikomatsakis self-assigned this Aug 11, 2016
@canndrew
Copy link
Contributor Author

canndrew commented Sep 5, 2016

I'm thinking of rewriting this to talk specifically about ! arguments. Or closing it and creating a new RFC. The stuff about methods being uncallable due to trait constraints is important but I feel it's a slightly separate issue.

@canndrew
Copy link
Contributor Author

I've narrowed this RFC to talk specifically about methods with uninhabited arguments (as opposed to methods with unsatisfiable trait bounds). About this:

struct TBD(!);

trait MyTrait {
    // For now, this always panics...
    fn foo(&self) -> TBD;
}

This effects more than just this RFC. It effects extensions to pattern-matching to allow patterns on uninhabited types to be omitted for example. I think I agree with @withoutboats here that these sorts of features should only be available to publicly uninhabited types, not secretly uninhabited ones.

@withoutboats
Copy link
Contributor

withoutboats commented Dec 7, 2016

@rfcbot postpone

I'm proposing we postpone this RFC for now. Uninhabited types are currently an in progress feature, and its not obvious how valuable it will be to leverage their uninhabitedness in ways like this. For that reason, I don't think this RFC's motivation is understood enough to justify it in the near term.

As @canndrew noted, this RFC also raises questions about types which are "publicly uninhabited" and "privately uninhabited," which introduces a whole load of additional complexity around uninhabitedness.

Indeed the definition of what is and isn't an uninhabited type itself is not fully agreed on right now (I'm speaking of the issues with &Void in FFI libraries).

That said, some time in the future, if & when uninhabited types become a fully normal part of the language, I could see this RFC as worth revisiting.

@withoutboats
Copy link
Contributor

@rfcbot fcp postpone

I'm not great at using this interface

@rfcbot
Copy link
Collaborator

rfcbot commented Dec 7, 2016

Team member @withoutboats has proposed to postpone this. The next step is review by the rest of the tagged teams:

No concerns currently listed.

Once these reviewers reach consensus, this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@aturon
Copy link
Member

aturon commented Dec 13, 2016

I agree with @withoutboats. This is an interesting line of work to pursue, and we've long discussed something like this for methods with unfulfillable where clauses, but it just seems to early to consider adding this now. We need more experience and real-world examples.

@rfcbot
Copy link
Collaborator

rfcbot commented Jan 6, 2017

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot rfcbot added the final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. label Jan 6, 2017
@briansmith
Copy link

IMO it's going to be difficult to decide how uninhabited types work piecemeal, one feature at a time. Instead this and how it interacts with other features regarding uninhabited types need to be considered as a whole. I don't see how this can be prudently postponed without postponing all work related to uninhabited types.

@aturon
Copy link
Member

aturon commented Jan 21, 2017

@briansmith Can you make your argument at all more concrete? At this level of abstraction, the argument you're making could apply to literally any two language features.

In general, features interact, and we have to be aware of that, but we have to balance it with making incremental, continuous progress. That's one of the basic challenges of language design and evolution.

@briansmith
Copy link

@briansmith Can you make your argument at all more concrete? At this level of abstraction, the argument you're making could apply to literally any two language features.

Specifically regarding how uninhabited types work, there are big questions about how to deal with non-generic code dealing with &T where T is uninhabited type, as well as how to deal with generic code dealing with &T which is instantiated with both inhabited and uninhabited types as T. All else being equal, it would be good to consider &T uninhabited if T is uninhabited. Also, all else being equal, it would be nice to reject statically code that is dead because it is operating on values that cannot possible exist.

However, "all else being equal" isn't necessarily the case as we understand things now. However, if this RFC were accepted then it could significantly reduce the amount of generic code that would operate on potentially-uninhabited types, which would get us closer to "all else being equal." Then we could seek out the remaining instances of where people want to write code that's potentially dead when instantiated with uninhabited types and see if there are creative ways of avoiding that code.

More generally, I think the ability to implement what is suggested in this RFC should be a significant factor in decisions regarding how uninhabited types work in general. In particular, it seems likely that any design decisions that would make this proposed feature work less often or less well are bad.

But postponing the RFC would imply the opposite of that, as it means literally: "we want neither to think about evaluating the proposal nor about implementing the described feature until some time in the future, and we believe that we can afford to wait until then to do so," according to the RFC RFC.

In general, features interact, and we have to be aware of that, but we have to balance it with making incremental, continuous progress. That's one of the basic challenges of language design and evolution.

Sure. Everybody knows that. But I think we default assumption when breaking up a feature into smaller features is that breaking it up cannot be done, because often we can be surprised by how N things interact when we're implementing the Nth thing, and we can easily get painted into a corner. People are already making arguments to the effect that we're already painted into a corner because the ! RFC was accepted without the rest of the related stuff being agreed upon, for example.

@withoutboats
Copy link
Contributor

However, if this RFC were accepted then it could significantly reduce the amount of generic code that would operate on potentially-uninhabited types, which would get us closer to "all else being equal."

How?

@canndrew
Copy link
Contributor Author

@briansmith

Also, all else being equal, it would be nice to reject statically code that is dead because it is operating on values that cannot possible exist.

For the sake of macros, these cases should always be possible to allow (by using #[allow(...)] at least).

However, if this RFC were accepted then it could significantly reduce the amount of generic code that would operate on potentially-uninhabited types

I don't know what you mean. Can you give an example?

Then we could seek out the remaining instances of where people want to write code that's potentially dead when instantiated with uninhabited types and see if there are creative ways of avoiding that code.

I don't understand what you mean here either.

In particular, it seems likely that any design decisions that would make this proposed feature work less often or less well are bad.

I don't know of any design proposals - other than postponing this RFC - that would interfere with this RFC. I'd like this RFC to be accepted because it seems like a clear-cut win to me and would make ! more user-friendly when we roll it out. But there's no reason in principle that we couldn't just implement this later.

People are already making arguments to the effect that we're already painted into a corner because the ! RFC was accepted without the rest of the related stuff being agreed upon, for example.

What are these arguments and what corners have we painted ourselves into? Are you talking about the mem::uninitialized thing?

@rfcbot
Copy link
Collaborator

rfcbot commented Jan 23, 2017

The final comment period is now complete.

@aturon
Copy link
Member

aturon commented Jan 23, 2017

The FCP has elapsed, but there's been some new points raised, so I'm going to hold off on taking any action until we've gotten to the bottom of these latest comments.

@arielb1
Copy link
Contributor

arielb1 commented Jan 24, 2017

People are already making arguments to the effect that we're already painted into a corner because the ! RFC was accepted without the rest of the related stuff being agreed upon, for example.

What are these arguments and what corners have we painted ourselves into? Are you talking about the mem::uninitialized thing?

The mem::uninitialized thing, and the &! thing, are unsafe code guidelines issues that does not have much relation to !, except for being exposed by a compiler refactoring. I don't remember any case where uninhabited types require special treatment on the UB front (that's it, they are just types with no inhabitant).

@nikomatsakis
Copy link
Contributor

@briansmith

I am having trouble reconciling this sentence, which I agree with:

IMO it's going to be difficult to decide how uninhabited types work piecemeal, one feature at a time.

with this one, from the same comment, which I don't believe follows:

I don't see how this can be prudently postponed without postponing all work related to uninhabited types.

Basically, my feeling is that precisely because I want to have a unified plan of how to deal with uninhabited types, I think we should postpone this RFC, which only deals with one tiny part of the problem.

That said, I think @arielb1 and I, at least, are pretty close to agreement on that general framework, though. It boils down to the question of when values must be valid for their types. I'm not inclined to write everything out here in a comment though -- I'm rather inclined to try and writeup an RFC or at least a blog post trying to spell it out. (I suspect @briansmith you have some idea what I'm talking about anyway, since it's come up in numerous internals threads.)

I think a consequence of this would be that functions and methods which have arguments whose types include ! -- but not behind a reference -- would certainly be uncallable, because they would expect their arguments to be "weakly valid". (I'd expect that "in safe code", suitably defined, we could go further and say that &! is also uncallable.)


I think though that there is also a somewhat orthogonal question about what to do in the myriad cases where the compiler can see that a method is uncallable. It feels like we should be consistent. (With respect to @nagisa's point about the potential for adding an impl to be a breaking change, I think the answer is that we should use the same negative reasoning that coherence employs.)

So TL;DR I could imagine someone filing a distinct RFC that simply addresses the question of "when the compiler determines that a method is uncallable -- for whatever reason -- can it be omitted and if so with what syntax".

I have wondered if simply omitting the method from the impl is the right way to signal that you believe it is not callable. It feels a bit "ambiguous" -- i.e., by omitting the method, you could be declaring that you want the default to be used, or that you believe it's uncallable. I could imagine that you think a method is uncallable (and the default wouldn't be appropriate) but then later a refactoring makes the method callable, and now you start silently using the default. OTOH, maybe I just worry too much, since after all one might have added a new method (with a default) to the trait and all existing code would keep compiling, without forcing you to go through each existing impl and "acknowledge" whether the default applies.

@nikomatsakis
Copy link
Contributor

In case it wasn't clear, personally I'm still inclined to postpone this RFC, and I don't feel we need a second FCP.

@aturon
Copy link
Member

aturon commented Mar 3, 2017

Per the latest round of comments after the close of FCP, I'm going to go ahead and closed as postponed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. T-lang Relevant to the language team, which will review and decide on the RFC.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants