-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
impl Trait in argument position #44721
Comments
This is a showstopper, IMO. |
Am I going to expect a shift in what is considered "idiomatic" Rust by this? As in, will the new rule say that you should almost always use impl Trait? I don't really care if its an optional feature, but I don't think impl Trait is in any way better or nicer than actual generics. |
@petrochenkov as a safe starting point, we can simply make it illegal to use explicit argument lists if |
I'd prefer not to debate too much the "policy" of this question in this particular issue, since I think this should focus more on how to implement. |
Mentoring instructionsThe goal is to desugar Basically, we will make it so that when the AST looks like this: fn foo(x: impl Iterator) { ... } The HIR looks like this: fn foo<T: Iterator>(x: T) { ... } We have to be careful though. For example, we don't want people to be able to explicitly supply type arguments (e.g., First step: Introduce the idea of a synthetic type parameter into the HIR. We could do this in a few ways. One would be to have a flag on the HIR enum SyntheticKind { ImplTrait } This way, if we add more kinds of synthetic type parameters in the future, we can track why they are synthetic for better error messages. =) To allow us to write unit tests, we can add a testing attribute Next, we need to adjust the OK, now, finally, we want to make it an error to specify explicit type parameters for some value (e.g., a function) if any of the type parameters on that function are synthetic. To do that, we want to modify the function that resolves paths during typeck, which is the What we want to do is to iterate through all the let err = match synthetic_kind {
hir::SyntheticKind::ImplTrait => {
struct_span_err!(
self.tcx.sess,
span,
EXXX, // we'll have to add a new error code...
"cannot provide explicit type parameters when `impl Trait` is used in argument position")
}
};
// bonus (described below) would go here
err.emit(); For bonus points, we could include a With all this, we should be able to add a test like this (maybe named fn foo<#[rustc_synthetic] T>(_: T) {
}
fn main() {
let x = foo::<u32>; //~ ERROR cannot provide explicit type parameters
foo(22); // ok
} This seems like enough for a first PR! |
I'm tagging this as |
I'd love to try and take a shot at this. If I understand your instructions correctly, the first PR you want doesn't include the parsing nor the conversion from |
Yes, that was what I suggested. However, note that the parsing is actually already done -- we accept (I actually think this will wind up looking rather similar to refactors that we are doing on our support for |
@mdevlamynck btw please do reach out on Gitter, either in the room for WG-compiler-middle or the room for WG-compiler-traits if you have any pressing questions. I will try to stay on top of GH notifications, but it's harder. |
I am also interested in working on this issue, and I've started trying to tackle it. @mdevlamynck, if you would like to coordinate or take a look at anything that I've already done, my work is here. |
@mdevlamynck @chrisvittal just checking in -- have either of you had much time to hack on this? |
I have. I'm having difficulties though. My work is here. At current, I can't get the test case you've outlined to pass. I believe this is because as written, my code does not distinguish between explicit and implicit type parameters. As such, when I check the type parameters in Thanks! |
Same here, I haven't had time to investigate why the test fails though. |
I have a working version, just need to add a specific error code and I'll submit a pull request. Is there a procedure or a documentation on how to add a new error code ? |
First step toward implementing impl Trait in argument position First step implementing #44721. Add a flag to hir and ty TypeParameterDef and raise an error when using explicit type parameters when calling a function using impl Trait in argument position. I don't know if there is a procedure to add an error code so I just took an available code. Is that ok ? r? @nikomatsakis
@nikomatsakis I have some time this weekend to hack on this some more. I found the code in librustc_typeck::astconv::ast_ty_to_ty() raising an error if Do you think the conversion should be handled here? |
My expectation, I think, was that we would, during HIR lowering, no longer produce a I am not 100% sure that this is the best route. Another possibility would be to handle this during the "ty queries" construction. In more detail, HIR lowering produces a kind of AST, but then there are various queries that produce the "semantic view" of that AST. For example, there is a But there is a query that creates a |
Let me try to expand out my thoughts a bit. I see two possible paths for implementing this feature. Both of them involve "desugaring", it's just a question of where we do it. Desugaring during HIR lowering. The initial route I proposed would modify the HIR. This means that if you have an
Now, as we discussed on Gitter, if the user wrote the equivalent code by hand, this HIR node would be a So, this replacement type X could be a TyPath(QPath::Resolved(None, Path {
span: /* span of the impl trait node */,
def: Def::TyParam(S), // see notes below about S
segments: /* empty vector */,
})) This is basically constructing very similar HIR to what you would get if you just wrote a reference to a type parameter manually. One difference is that the list of segments would -- in the real case -- not be empty. e.g. if the user wrote Let me say a bit about the Desugaring during queries. Another way to go about it would be to intercept the if we take this route, then we also have to alter the code that converts a HIR I think we might want to split the HIR fn foo(impl Iterator) { .. }
// becomes:
fn foo<T: Iterator>(t: T) { // "universally quantified"
} The latter ( We would then modify HIR lowering to know what the context is and convert to the appropriate case, or issue an error if Presuming we did this, then the code that converts Decision time. As you can see, it feels like there is a lot of overlap between these two strategies. I'm curious whether you have an opinion which one appeals to you (also, ping @eddyb and @arielb1). At the moment I lean towards the second approach -- synthesizing HIR that the user did not type feels like it's going to be a lot of pain. |
OK, let me try to retool the mentoring instructions for the second approach in a bit more detail. The first step would be retooling the Next, we would modify HIR lowering to carry some state indicating its context. Probably my preferred way to do this would be to add a ( #[derive(Copy, Clone, Debug)]
enum TypeLoweringContext {
FnParameter(DefId), // the DefId here is the def-id of the function to which this is a parameter
FnReturnType(DefId), // here too
Other
}
impl TypeLoweringContext {
fn impl_trait_treatment(self) -> ImplTraitTreatment {
use self::TypeLoweringContext::*;
use self::ImplTraitTreatment::*;
match self {
FnParameter(_) => Universal,
FnReturnType(_) => Existential,
Other => Disallowed,
}
}
}
enum ImplTraitTreatment {
/// Treat `impl Trait` as shorthand for a new universal generic parameter.
/// Example: `fn foo(x: impl Debug)`, where `impl Debug` is conceptually
/// equivalent to a fresh universal parameter like `fn foo<T: Debug>(x: T)`.
Universal,
/// Treat `impl Trait` as shorthand for a new universal existential parameter.
/// Example: `fn foo() -> impl Debug`, where `impl Debug` is conceptually
/// equivalent to a fresh existential parameter like `abstract type T; fn foo() -> T`.
Existential,
/// `impl Trait` is not accepted in this position.
Disallowed,
} Once we have that, we can alter the code that lowers an TyKind::ImplTrait(ref bounds) => {
match context.impl_trait_treatment() {
ImplTraitTreatment::Existential => hir::TyImplTrait(self.lower_bounds(bounds)),
ImplTraitTreatment::Universal | ImplTraitTreatment::Disallowed => {
// For now, treat Universal as the same as disallowed since it's not done yet.
span_err!(tcx.sess, ast_ty.span, E0562,
"`impl Trait` not allowed outside of function \
and inherent method return types");
hir::TyErr
}
}
} We can now adjust the The final step is to introduce TyTraitUniversal(DefId, TyParamBounds) Simultaneously, we would modify the Once we have the list of inputs, we can generate the new, synthetic fn visit_ty(&mut self, ty: &hir::Ty) {
if let hir::TyTraitUniversal(..) = ty.node {
self.implicit_defs.push(
ty::TypeParameterDef {
index: /* compute next index -- this should come after the generics the user wrote, presumably */,
name: /* um what should we put here? */,
def_id: tcx.hir.local_def_id(ty.id), // <-- use def-id we created for the `impl Trait` instance
has_default: false,
object_lifetime_default: rl::Set1::Empty,
pure_wrt_drop: false,
synthetic: Some(...), // <-- this is synthetic, naturally
});
}
intravisit::walk_ty(ty);
} where We also have to modify Finally, we have to modify the TyImplTraitUniversal(fn_def_id, _) => {
let impl_trait_def_id = self.tcx.hir.local_def_id(ast_ty.id);
let generics = self.tcx.generics_of(fn_def_id);
let index = /* find the index of `impl_trait_def_id` in `generics`, see LINK1 below */;
self.tcx.mk_param(index, impl_trait_def_id)
} At this point, I think that basic examples should be working. For example: fn any_zero(values: impl IntoIterator<Item=u32>) -> bool {
for v in values { if v == 0 { return true; } }
false
} What will not work is one corner case having to do with lifetimes. In particular, I would expect this to ICE or otherwise misbehave: fn any_zero<'a>(values: impl IntoIterator<Item=&'a u32>) -> bool {
for v in values { if *v == 0 { return true; } }
false
} Actual final step: to fix that last case, we have to patch up early- vs late-bound lifetimes (some explanation on this concept -- and links to blog posts -- can be found here). This involves modifying the |
I've started on this. @nikomatsakis, You mention that we can possibly add a For example Thanks! |
Presuming you added the enum that I suggested, basically everything would be
Basically the rule is:
|
@nikomatsakis My WIP branch is here. Notes on my current implementation: I'm implementing it almost exactly as suggested here. I've named what's we've called Here are the questions I have now.
inputs: decl.inputs.iter()
.map(|arg| {
// FIXME causes ICE compliling stage 1, need to find a better solution
let def_id = self.resolver.definitions().local_def_id(arg.id);
self.lower_ty(&arg.ty, TyLoweringCtx::FnParameter(def_id))
}).collect(),
|
One thing that may not have been obvious: I meant for that to be the Presuming we do need it, though,
Also, I'm beginning to question the idea of calling this the It might also be easier to use a field than threading things around as parameters, though I think that can often be more confusing overall. Finally, I think I made a small mistake before, in that I think we want to be sure we permit impl trait in path parameters -- e.g., something like However, the calls in Just to help, here is a series of tests and the
All the cases for |
@nikomatsakis I'm very close to being done with this. I have both the simple and the complex examples from the instructions working. I'm currently having trouble with I'm getting too many errors, and I'm not seeing one error that I should be. The one I'm most concerned about is the last not found error, which corresponds to the where clause item here Where does the where clause get lowered in this example? I need to find it to create the Another question, the Thanks. Here are the messages from the failing test
|
I think that I've figured out that where clauses get lowered, in Update: So it turns out that the error missing from line 52, has to do with this call to
I'm going to try to thread some parameter into Update 2: I think the easiest way to take care of this may be to do some kind of query much like the one in the old |
@chrisvittal I think that |
Example callers:
|
I've just pushed the latest WIP here. I've figured out that the first offending call that then propagates a This is the file I've been using to test: #![allow(unused)]
#![feature(conservative_impl_trait,dyn_trait)]
use std::fmt::Debug;
fn dyn_universal(_: &dyn Iterator<Item = impl Debug>) { panic!() }
fn main() {} |
@chrisvittal I thought of something we should be sure to test. I'm not sure if we're just blanket disallowed In other words, I don't want to accept things that sort of "reveal" the desugaring. This seems OK: trait Foo {
fn bar(&self, x: &impl Debug);
}
impl Foo for () {
fn bar(&self, x: &impl Debug) { }
} This seems not OK: trait Foo {
fn bar(&self, x: &impl Debug);
}
impl Foo for () {
fn bar<U>(&self, x: &U) { }
} Nor this: trait Foo {
fn bar<U: Debug>(&self, x: &U);
}
impl Foo for () {
fn bar(&self, x: &impl Debug) { }
} Eventually we might want to accept these examples, but maybe not, it kind of hinges on this question we decided to defer earlier about how much to reveal the desugaring when doing things like |
@nikomatsakis As of right now, I think that we accept 1, reject 2, and I'm not sure about 3. I will be sure to test all of them. |
Okay, as it stands right now, all of the cases there seem to succeed. I think we need to check that for a given parameter, if it is 'synthetic' in the trait definition that it is 'synthetic' in the impl. One more question, is the following okay. trait Foo {
fn bar(&self, &impl Debug);
}
impl Foo for () {
fn bar(&self, &impl Debug + Display) { }
} |
@nikomatsakis For the examples above with the deshugaring, all of them currently compile. I think that what I need to do, is in I need some advice on properly getting all the spans for error reporting. |
@chrisvittal Your example is definitely not ok (E0276), a better question is whether this is ok: trait Foo {
fn bar(&self, _: &impl (Debug + Display));
}
impl Foo for () {
fn bar(&self, _: &impl Debug) { }
} |
@kennytm As of right now your example compiles. Thanks for pointing the other error out. |
@nikomatsakis I just pushed a preliminary solution to detect matching up. It can be seen here. Thoughts? |
Regarding things like |
Implement `impl Trait` in argument position (RFC1951, Universal quantification) Implements the remainder of #44721, part of #34511. **Note**: This PR currently allows argument position `impl Trait` in trait functions. The machinery is there to prevent this if we want to, but it currently does not. Rename `hir::TyImplTrait` to `hir::TyImplTraitExistential` and add `hir::TyImplTraitUniversal(DefId, TyParamBounds)`. The `DefId` is needed to extract the index of the parameter in `ast_ty_to_ty`. Introduce an `ImplTraitContext` enum to lowering to keep track of the kind and allowedness of `impl Trait` in that position. This new argument is passed through many places, all ending up in `lower_ty`. Modify `generics_of` and `explicit_predicates_of` to collect the `impl Trait` args into anonymous synthetic generic parameters and to extend the predicates with the appropriate bounds. Add a comparison of the 'syntheticness' of type parameters, that is, prevent the following. ```rust trait Foo { fn foo(&self, &impl Debug); } impl Foo for Bar { fn foo<U: Debug>(&self, x: &U) { ... } } ``` And vice versa. Incedentally, supress `unused type parameter` errors if the type being compared is already a `TyError`. **TODO**: I have tried to annotate open questions with **FIXME**s. The most notable ones that haven't been resolved are the names of the `impl Trait` types and the questions surrounding the new `compare_synthetic_generics` method. 1. For now, the names used for `impl Trait` parameters are `keywords::Invalid.name()`. I would like them to be `impl ...` if possible, but I haven't figured out a way to do that yet. 2. For `compare_synthetic_generics` I have tried to outline the open questions in the [function itself](https://github.com/chrisvittal/rust/blob/3fc9e3705f7bd01f3cb0ea470cf2892f17a92350/src/librustc_typeck/check/compare_method.rs#L714-L725) r? @nikomatsakis
If impl trait in argument position is just syntactic sugar for a generic, there's still no way to have the implementation of the function determine the type, instead of the caller. What are the plans on resolving that issue, cause otherwise impl trait in argument position seems kinda pointless (other than the ergonomic changes)? In particular something like this should work somehow: fn fill_futures(futures: &mut Vec<impl Future<Item = i32, Error = Error>>) { ... } where the function is filling up a Vector of Futures, where the type of Future is determined by the function itself. This does not seem to be possible with impl Trait in argument position. |
@CryZe yes, impl trait even in argument position shouldn't have anything to do with generics IMO, only with "this type is specified by the callee". |
I think this is basically done. Closing. |
Is there a tracking issue for allowing this case? It is counter-intuitive that this is forbidden. |
(Part of #34511)
As part of the latest
impl Trait
RFC, we decided to acceptimpl Trait
in argument position:this is roughly equivalent to
fn foo<T: Iterator<Item = u32>>(t: T)
, except that explicitly specifying the type is not permitted (i.e.,foo::<u32>
is an error).Mentoring instructions can be found here.
Questions to resolve:
fn foo<T>(x: impl Iterator<Item = T>>)
? I think yes,foo::<u32>
would be accepted (thus bindingT = u32
explicitly).impl Trait
arguments.The text was updated successfully, but these errors were encountered: