The Great Generics Generalisation: HIR Edition #48149

varkor · 2018-02-11T23:26:05Z

This is essentially a followup to #45930, consolidating the use of separate lifetime and type vectors into single kinds vectors wherever possible. This is intended to provide more of the groundwork for const generics (#44580).

r? @eddyb
cc @yodaldevoid

eddyb · 2018-02-12T13:22:44Z

src/librustc/hir/intravisit.rs

@@ -718,11 +718,11 @@ pub fn walk_ty_param_bound<'v, V: Visitor<'v>>(visitor: &mut V, bound: &'v TyPar

 pub fn walk_generic_param<'v, V: Visitor<'v>>(visitor: &mut V, param: &'v GenericParam) {
    match *param {
-        GenericParam::Lifetime(ref ld) => {
+        GenericParameter::Lifetime(ref ld) => {


General comment: we prefer the shorter Param to Parameter (changes should be from the latter to the former, not the other way around).

petrochenkov · 2018-02-12T19:47:11Z

src/libsyntax/ast.rs

-    /// The type parameters for this path segment, if present.
-    pub types: Vec<P<Ty>>,
+    /// The parameters for this path segment.
+    pub parameters: Vec<GenericAngleBracketedParam>,


Similarly to #45930, I don't see how this makes us closer to const generics.
Okay, maybe with merging lifetime and type arguments into an enum and using exhaustive matching everywhere we will less likely forget to add const generics somewhere, but that's all.

On the other hand, we get methods fn lifetimes and fn types below that have to allocate just to get the separate lists of lifetime and type arguments.
In addition, binding arguments are still separate, and we can't unify with fn-like arguments as well because they use totally different structure.

Seems like a net negative to me.

Doing the same transformation in HIR (and in ty) may be a better idea because we can unify with fn-like generic arguments, but from what I see it still makes things more complex, less convenient and we have to allocate just to visit something.
Can we maybe implement AST/HIR support for const generics first and only then exercise in questionable refactoring?

I'm also not a fan of generic enum GenericParameter used both in HIR and in ty.
There's plenty of opportunities to do this generalization in various places in HIR and ty, but this is not generally done, because... that's not useful, there's no reason to generalize here, let's keep HIR and ty separate.

Okay, I guess without enum GenericParameter the HIR refactoring is not that bad if we avoid allocation and use the same iterator-based approach to fn lifetimes/fn types as #45930 does.

Also the things in this PR are GenericArgs, not GenericParameters (these already exist) or especially PathParameters (it's hard to come up with worse name, they are neither parameters, nor apply to a path), good opportunity to rename!

I'm going to do a more thorough review but 1. I'm against abstracting enum definitions when it doesn't have a drastically positive impact (I've debated this on IRC already) 2. I have high hopes for getting rid of lifetimes / types methods in most places, and the few remaining places should just do it by hand with exhaustive matches.

Oh, also, one of the reasons I'm against over-abstracting enums is because in some places there are actual common fields (like IDs & names) and it'd be useful to extract those out.

In addition to not doing this refactoring on AST, I'd actually prefer to revert the AST part of #45930, at least until all kinds of generic parameters are allowed to intermingle (which may never happen). AST is supposed to be close to the source and I don't think it's desirable to make things impossible in source (like <'a, T, U, 'b>) representable in it.

That might be a worthwhile goal, but didn't the const generics RFC impose no order between type and const parameters? cc @withoutboats @nikomatsakis

Can we maybe implement AST/HIR support for const generics first and only then exercise in questionable refactoring?

FWIW I've been asking (on IRC) for the refactoring before landing const generics, because I'm really worried that certain parts of the compiler which hackily use separate vectors for lifetimes and types will get even worse and make the refactoring more difficult.

@petrochenkov: It doesn't really make const generics closer — it's intended to reduce the amount of boilerplate when they're introduced, as well as trigger errors if we forget to handle them in a particular spot.

I dislike the types()/lifetimes() usages too. I'd like to remove them wherever possible, as @eddyb suggests. I've removed the obvious instances where they were unnecessary, but hopefully there's a lot more clean up that can be done.

The names are effectively all placeholders at this stage — my main aim was to keep them separate, so they were easy to identity. It's very easy to rename them all!

@eddyb: if lifetimes()/types() can be mostly removed, then the common enum is less useful. But if no-one else likes it, I'll remove it now...

In addition to not doing this refactoring on AST, I'd actually prefer to revert the AST part of #45930, at least until all kinds of generic parameters are allowed to intermingle (which may never happen).

That might be a worthwhile goal, but didn't the const generics RFC impose no order between type and const parameters?

Ordering of type and const parameters was left as an open question in the RFC. However, I think it really makes sense to restrict const parameters (at least to begin with) to come after type parameters because of the dependencies it implies:

Types can depend on lifetimes; consts can depend on types (possible in theory, though not part of RFC 2000).

If consts can come before types, to me that implies that types dependent on consts can be specified, and Rust definitely isn't ready for those yet.

eddyb · 2018-02-12T20:31:47Z

src/librustc/hir/mod.rs

@@ -340,12 +341,12 @@ impl PathSegment {
    }
 }

+pub type GenericPathParam = GenericParameter<Lifetime, P<Ty>>;


IMO this should be called just PathParam (and PathParameters below just PathParams). Same for parameters vs params, but avoid changing too much existing code unless you want a separate PR.

eddyb · 2018-02-12T22:38:44Z

src/librustc/ty/subst.rs

@@ -39,6 +39,20 @@ const TAG_MASK: usize = 0b11;
 const TYPE_TAG: usize = 0b00;
 const REGION_TAG: usize = 0b01;

+pub enum GenericParameterKind<'tcx> {


Do we really need this? I know Kind is a bit awkward to work with... but if you an UnpackedKind it should maybe be a separate PR? Because you should remove as_{lifetime,type} and force everything to go through UnpackedKind.

It simplifies one loop so far. I'll rename it now and think about whether it's worth committing to or not.

eddyb · 2018-02-12T22:40:33Z

src/librustc/hir/intravisit.rs

-    walk_list!(visitor, visit_lifetime, &path_parameters.lifetimes);
-    walk_list!(visitor, visit_ty, &path_parameters.types);
+    walk_list!(visitor, visit_lifetime, path_parameters.lifetimes());
+    walk_list!(visitor, visit_ty, path_parameters.types());


The reason methods like lifetimes / types should be avoided is that this code needs to look much different (it should iterate and match over path_parameters - maybe with a visit_path_param method in Visitor).

eddyb · 2018-02-12T22:40:51Z

src/librustc/hir/lowering.rs

@@ -871,6 +872,20 @@ impl<'a> LoweringContext<'a> {
        }
    }

+    fn lower_param(&mut self,


lower_path_param

yodaldevoid · 2018-02-12T22:43:03Z

src/librustc/hir/print.rs

@@ -1680,17 +1680,27 @@ impl<'a> State<'a> {
                }
            };

-            if !parameters.lifetimes.iter().all(|lt| lt.is_elided()) {
-                for lifetime in &parameters.lifetimes {
-                    start_or_comma(self)?;


Did you forget to include start_or_comma in the final code?

Yes, I forgot one of the edge cases! Thanks!

petrochenkov · 2018-02-13T07:00:38Z

@varkor

However, I think it really makes sense to restrict const parameters (at least to begin with) to come after type parameters because of the dependencies it implies:
Types can depend on lifetimes; consts can depend on types (possible in theory, though not part of RFC 2000).
If consts can come before types, to me that implies that types dependent on consts can be specified, and Rust definitely isn't ready for those yet.

I agree with placing constant parameters/arguments between type parameters/arguments and type bindings (when applicable) for now, but I don't think dependencies you've described are the right reason.

Generic parameters don't form a stack, they are introduced "simultaneously", you can refer to later parameters from earlier parameters, the only restriction is absence of cycles.

struct S<T: PartialEq<U>, U>(T, U); // OK

I'm mostly concerned about matching generic parameters with provided generic arguments, especially when some arguments are omitted.
Now lifetime arguments and type arguments are effectively separate sets of generics:

fn f<'a, T>(...) { ... }

f::<'lifetimes, Types>(...)
// is effectively
f::<'lifetimes><Types>(...)
// so if you omit lifetimes
f::<Types>(...)
// then it's still okay, even if `Types` is in the "wrong" position (first, not second)
// because this is equivalent to
f::<><Types>(...)
// i.e. inferred lifetime arguments, provided type arguments

If intermingled generic parameters/arguments are allowed, they will have to support these kind of rules somehow.

petrochenkov · 2018-02-13T07:03:07Z

src/librustc/hir/mod.rs

@@ -340,12 +340,16 @@ impl PathSegment {
    }
 }

+#[derive(Clone, PartialEq, Eq, RustcEncodable, RustcDecodable, Hash, Debug)]
+pub enum PathParam {


See #48149 (comment)

Also the things in this PR are GenericArgs, not GenericParameters (these already exist) or especially PathParameters (it's hard to come up with worse name, they are neither parameters, nor apply to a path), good opportunity to rename!

If you keep this naming, I'll rename it later anyway, but it would be nice to avoid double cruft.

Oh, sorry, I forgot you mentioned this! What do you suggest renaming PathParameters to?

PathParam -> GenericArg, PathParameters -> GenericArgs.

petrochenkov · 2018-02-13T11:23:36Z

src/librustc/hir/mod.rs

-    /// The type parameters for this path segment, if present.
-    pub types: HirVec<P<Ty>>,
+    /// The generic parameters for this path segment.
+    pub parameters: HirVec<PathParam>,
    /// Bindings (equality constraints) on associated types, if present.
    /// E.g., `Foo<A=Bar>`.
    pub bindings: HirVec<TypeBinding>,


@eddyb
I wonder if bindings should be included to the list as well.
On one hand, if the goal is to employ exhaustive matching to avoid errors, then they should be included. I would certainly expect a visitor method like fn visit_generic_arg (aka fn visit_path_param) to visit them as well.
On the other hand they are not quite like other generic arguments and don't directly mirror anything from GenericParams.

I've seen ICEs caused by associated type bindings being forgotten about (fixed some of them in #43532).

Yeah, I was considering this too. I think @eddyb's opinion was that they were different enough that it was better keeping them separate, but it'd be nice to have some way to ensure they were always handled too.

eddyb · 2018-02-14T01:48:24Z

src/librustc/traits/select.rs

@@ -564,7 +564,7 @@ impl<'cx, 'gcx, 'tcx> SelectionContext<'cx, 'gcx, 'tcx> {
                    let trait_ref = &mut trait_pred.trait_ref;
                    let unit_substs = trait_ref.substs;
                    let mut never_substs = Vec::with_capacity(unit_substs.len());
-                    never_substs.push(UnpackedKind::Type(tcx.types.never).pack());
+                    never_substs.push(From::from(tcx.types.never));


This should be Kind::from - or preferably .into().

yodaldevoid · 2018-02-17T06:31:13Z

Is there anything left to do here or any more concerns that need to be voiced? If not, is this ready to be moved along or should it be dropped?

petrochenkov · 2018-02-17T09:14:04Z

Is there anything left to do here

fn visit_generic_arg is not introduced for AST.
fn lifetimes/fn types are still vector-based and not iterator-based
I'm still waiting for @eddyb's opinion on (now hidden) The Great Generics Generalisation: HIR Edition #48149 (comment) (this is certainly a trade-off, but I think in AST and HIR it's better to merge associated type bindings into the common list with "normal" generic arguments).
Optionally: reverting this refactoring in AST.

varkor · 2018-02-17T13:09:54Z

I've addressed the latest comments — I can include type bindings in the refactoring if the decision is made. I think it's also reasonably something that could be done in a later PR if a conclusive decision is not made now.

bors · 2018-02-22T14:23:23Z

☔ The latest upstream changes (presumably #48399) made this pull request unmergeable. Please resolve the merge conflicts.

eddyb · 2018-02-22T14:32:15Z

src/librustc/hir/mod.rs

+#[derive(Clone, PartialEq, Eq, RustcEncodable, RustcDecodable, Hash, Debug)]
+pub struct GenericArgs {
+    /// The generic parameters for this path segment.
+    pub parameters: HirVec<GenericArg>,


Shouldn't this be called args?

eddyb · 2018-02-22T14:32:32Z

src/librustc/hir/mod.rs

@@ -355,30 +359,48 @@ pub struct PathParameters {
    pub parenthesized: bool,


Does this field make sense without this struct being called PathArgs or something?

Yes, the "parenthesized" property still applies to a single segment, not the whole path, e.g. Fn(Args1)::Output<Args2>.

eddyb · 2018-02-22T14:33:30Z

src/librustc/middle/resolve_lifetime.rs

@@ -150,7 +149,7 @@ impl Region {
        }
    }

-    fn subst(self, params: &[hir::Lifetime], map: &NamedRegionMap) -> Option<Region> {
+    fn subst(self, params: Vec<&hir::Lifetime>, map: &NamedRegionMap) -> Option<Region> {


This seems potentially expensive, why not use an iterator?

Ah, I thought I had replaced these all with iterators; must have overlooked that one.

eddyb · 2018-02-22T14:34:52Z

src/librustc/ty/mod.rs

+}
+
+#[derive(Clone, Debug, RustcEncodable, RustcDecodable)]
+pub struct KindIndexed<L, T> {


Do we really want this? Also I'd keep such a refactoring separate from AST/HIR IMO.

Yeah, it's not used in very many places... I did it simply to avoid magic constants, but I was debating whether it was worth the extra declaration. I can separate it out regardless, though.

Restioson · 2018-06-21T16:35:33Z

👏 👏 Thanks so much!!

varkor · 2018-06-21T16:43:40Z

So, as far as I'm aware, there was one more small piece of refactoring to do — but I'm quite happy to make a follow-up PR for it to end the constant rebasing with this one. (I'm going to try to get to it by the weekend.)

alexreg · 2018-06-21T16:57:56Z

@varkor Sounds fair. Really good work on this. I think we all can’t wait to see some form of const generics support in nightly!

bors · 2018-06-21T20:58:59Z

⌛ Testing commit daf7e35 with merge 662c70a...

@eddyb

The Great Generics Generalisation: HIR Edition This is essentially a followup to #45930, consolidating the use of separate lifetime and type vectors into single kinds vectors wherever possible. This is intended to provide more of the groundwork for const generics (#44580). r? @eddyb cc @yodaldevoid

Restioson · 2018-06-21T21:02:23Z

🎉

bors · 2018-06-21T23:08:27Z

☀️ Test successful - status-appveyor, status-travis
Approved by: petrochenkov
Pushing 662c70a to master...

eddyb · 2018-06-22T02:56:10Z

@petrochenkov My bad (going through my GitHub notifications right now), and thanks for pushing this through! IIRC everything should've been addressed, but I'll take another look now to be sure.

eddyb · 2018-06-22T03:01:34Z

src/librustc/middle/reachable.rs

@@ -37,21 +37,30 @@ use hir::intravisit;

 // Returns true if the given set of generics implies that the item it's
 // associated with must be inlined.
-fn generics_require_inlining(generics: &hir::Generics) -> bool {
-    generics.params.iter().any(|param| param.is_type_param())
+fn generics_require_inlining(generics: &ty::Generics) -> bool {


Doesn't this usecase already have a method on ty::Generics?

I don't think so. At least, nothing with a similar name.

I meant the behavior, not name. I'm pretty sure it's requires_monomorphization.

Okay, I'll change it.

eddyb · 2018-06-22T03:07:42Z

src/librustc_lint/types.rs

+                    ty::GenericParamDefKind::Lifetime { .. } => {},
+                    ty::GenericParamDefKind::Type { .. } => return,
+                }
+            }


You already have Err(ty::layout::LayoutError::Unknown(_)) => return, below, so you don't need this check anymore.

eddyb · 2018-06-22T03:09:09Z

src/librustc_metadata/encoder.rs

+                    let has_types = generics.params.iter().any(|param| match param.kind {
+                        ty::GenericParamDefKind::Type { .. } => true,
+                        _ => false,
+                    });


Again, isn't there a method on ty::Generics for this check?

There's a way to count the number of types, but that won't return early. I could add a method if it's frequent enough.

Again, I think it's requires_monomorphization.

eddyb · 2018-06-22T03:09:54Z

src/librustc_metadata/encoder.rs

-            let has_default = Untracked(ty_param.default.is_some());
-            self.record(def_id, IsolatedEncoder::encode_info_for_ty_param, (def_id, has_default));
-        }
+        generics.params.iter().for_each(|param| match param.kind {


Unnecessary for_each.

eddyb · 2018-06-22T03:11:37Z

src/librustc_passes/ast_validation.rs

+                data.args.iter().for_each(|arg| match arg {
+                    GenericArg::Type(ty) => self.visit_ty(ty),
+                    _ => {}
+                });


Why doesn't this use visit_generic_arg? Or is there no such method?

eddyb · 2018-06-22T03:12:34Z

src/librustc_privacy/lib.rs

+            GenericParamKind::Type { .. } => {
+                for bound in &param.bounds {
+                    self.check_generic_bound(bound);
+                }


This can avoid looking at param.kind.

eddyb · 2018-06-22T03:14:45Z

src/librustc_resolve/lib.rs

+                GenericParamKind::Type { ref default, .. } => {
+                    if found_default || default.is_some() {
+                        found_default = true;
+                        return Some((Ident::with_empty_ctxt(param.ident.name), Def::Err));


return in a closure can be confusing - if-else works here. Also, an unconditional found_default |= default.is_some() followed by if found_default may be clearer.

eddyb · 2018-06-22T03:17:00Z

src/librustc_typeck/check/method/confirm.rs

+                    GenericParamDefKind::Type { .. } => (false, true),
+                };
+                provided.as_ref().and_then(|data| {
+                    for arg in &data.args {


Still quadratic - is fixing this part of the cleanup you mentioned?

eddyb · 2018-06-22T03:22:05Z

I've left some comments. There are 11 uses of for_each which should be for loops, and other mostly minor things. The only big change I'm expecting is to rustc_typeck::{astconv,check}.

cc rust-lang/rust#48149.

Addresses the errors produced by (re)moving, merging or renaming structs, fields and methods by rust-lang/rust#48149 and rust-lang/rust#51580

@eddyb

…=eddyb The Great Generics Generalisation: HIR Followup Addresses the final comments in #48149. r? @eddyb, but there are a few things I have yet to clean up. Making the PR now to more easily see when things break. cc @yodaldevoid

rust-highfive assigned eddyb Feb 11, 2018

pietroalbini added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 11, 2018

petrochenkov self-assigned this Feb 12, 2018

eddyb reviewed Feb 12, 2018

View reviewed changes

petrochenkov reviewed Feb 12, 2018

View reviewed changes

eddyb reviewed Feb 12, 2018

View reviewed changes

varkor force-pushed the generics-generalisation branch from 3e8baf0 to f9e0747 Compare February 12, 2018 22:26

eddyb reviewed Feb 12, 2018

View reviewed changes

yodaldevoid reviewed Feb 12, 2018

View reviewed changes

varkor force-pushed the generics-generalisation branch from 27bf7a3 to 5aeb66b Compare February 13, 2018 00:08

Centril added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 13, 2018

petrochenkov reviewed Feb 13, 2018

View reviewed changes

varkor force-pushed the generics-generalisation branch from 6c11160 to 48b7730 Compare February 13, 2018 14:37

eddyb reviewed Feb 14, 2018

View reviewed changes

varkor force-pushed the generics-generalisation branch from b53a2bb to 5940768 Compare February 17, 2018 10:13

varkor force-pushed the generics-generalisation branch from 9bbcbed to 4a919c9 Compare February 17, 2018 14:51

varkor mentioned this pull request Feb 19, 2018

Rebase const-generics branch varkor/rust#1

Closed

eddyb reviewed Feb 22, 2018

View reviewed changes

bors merged commit daf7e35 into rust-lang:master Jun 21, 2018

This was referenced Jun 21, 2018

Suggest replacing named lifetime with 'static #51513

Closed

Remove quote_*! macros #51285

Merged

async/await #51580

Merged

Misc lowering cleanups #51654

Closed

eddyb reviewed Jun 22, 2018

View reviewed changes

This was referenced Jun 22, 2018

Update codegen for 2018-06-22 nightly. rwf2/Rocket#666

Closed

Does not compile on rustc 1.28.0-nightly (cbc4c8380 2018-06-22) rust-lang/rust-clippy#2869

Closed

topecongiro added a commit to topecongiro/rustfmt that referenced this pull request Jun 25, 2018

Fix compile errors from breaking changes in libsyntax

e5e1e0c

cc rust-lang/rust#48149.

topecongiro mentioned this pull request Jun 25, 2018

Update rustc-ap-* to 174.0.0 rust-lang/rustfmt#2806

Merged

flip1995 added a commit to flip1995/rust-clippy that referenced this pull request Jun 25, 2018

Resolve field, struct and function renaming

d9a80d2

Addresses the errors produced by (re)moving, merging or renaming structs, fields and methods by rust-lang/rust#48149 and rust-lang/rust#51580

flip1995 mentioned this pull request Jun 25, 2018

The Great Generics Generalisation: Clippy edition rust-lang/rust-clippy#2871

Merged

varkor mentioned this pull request Jun 28, 2018

The Great Generics Generalisation: HIR Followup #51880

Merged

		@@ -355,30 +359,48 @@ pub struct PathParameters {
		pub parenthesized: bool,

The Great Generics Generalisation: HIR Edition #48149

The Great Generics Generalisation: HIR Edition #48149

Conversation

varkor commented Feb 11, 2018

Choose a reason for hiding this comment

petrochenkov Feb 12, 2018 • edited Loading

Choose a reason for hiding this comment

petrochenkov Feb 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petrochenkov Feb 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eddyb Feb 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petrochenkov commented Feb 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yodaldevoid commented Feb 17, 2018

petrochenkov commented Feb 17, 2018

varkor commented Feb 17, 2018

bors commented Feb 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Restioson commented Jun 21, 2018

varkor commented Jun 21, 2018 • edited Loading

alexreg commented Jun 21, 2018

bors commented Jun 21, 2018

Restioson commented Jun 21, 2018

bors commented Jun 21, 2018

eddyb commented Jun 22, 2018

Choose a reason for hiding this comment

varkor Jun 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eddyb Jun 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eddyb commented Jun 22, 2018 • edited Loading

petrochenkov Feb 12, 2018 •

edited

Loading

petrochenkov Feb 12, 2018 •

edited

Loading

petrochenkov Feb 12, 2018 •

edited

Loading

eddyb Feb 12, 2018 •

edited

Loading

petrochenkov commented Feb 13, 2018 •

edited

Loading

varkor commented Jun 21, 2018 •

edited

Loading

varkor Jun 22, 2018 •

edited

Loading

eddyb Jun 22, 2018 •

edited

Loading

eddyb commented Jun 22, 2018 •

edited

Loading