Generics refactoring (groundwork for const generics) #45930

jplatte · 2017-11-11T12:14:06Z

These changes were suggested by @eddyb.

After this change, the Generics contain one Vec of an enum for the generic parameters, rather than two separate Vecs for lifetime and type parameters. Type params and const params will need to be in a shared Vec to preserve their ordering, and moving lifetimes into the same Vec should simplify the code that processes Generics.

eddyb · 2017-11-11T12:28:16Z

src/libsyntax_ext/deriving/generic/mod.rs

@@ -598,7 +598,7 @@ impl<'a> TraitDef<'a> {

                    let predicate = ast::WhereBoundPredicate {
                        span: self.span,
-                        bound_lifetimes: vec![],
+                        bound_generics: Default::default(),


Here, and in a few other places, you have Default::default(), I would prefer Generics::default() for clarity.

eddyb · 2017-11-11T12:29:37Z

src/librustdoc/clean/mod.rs

            where_predicates: simplify::where_clauses(cx, where_predicates),
        }
    }
 }

+impl Default for Generics {


You can just derive Default instead of manually implementing it.

Huh, dunno why I missed that this is the same thing that the derive would generate.

eddyb · 2017-11-11T12:33:20Z

src/librustc/hir/intravisit.rs

@@ -279,6 +279,12 @@ pub trait Visitor<'v> : Sized {
    fn visit_ty(&mut self, t: &'v Ty) {
        walk_ty(self, t)
    }
+    fn visit_ty_param(&mut self, t: &'v TyParam) {


This isn't actually used - I'd prefer if people went through visit_generic_param or visit_generics instead - that way, when const generics are added, they're harder to miss.

Also, have you considered removing visit_lifetime_def? I believe that was the reason I mentioned changing Vec<LifetimeDef> to Generics, because then all the cases can be handled together.

petrochenkov · 2017-11-11T12:55:14Z

src/libsyntax/ast.rs

-    /// Any lifetimes from a `for` binding
-    pub bound_lifetimes: Vec<LifetimeDef>,
+    /// Any generics from a `for` binding
+    pub bound_generics: Generics,


There may be reasons to change Vec<LifetimeDef> into Vec<GenericParam> in the future, but why Generics?
I'd rather just leave Vec<LifetimeDef> in place, this change doesn't help const generics in any way and permits too many things in AST that can't happen in actual code.

Same for BareFnTy and PolyTraitRef.

IIRC there are a bunch of places where visit_lifetime_def is implemented separately from type parameters (visit_generics), only because visit_generics wouldn't be called in those Vec<LifetimeDef> cases (see also #45930 (comment)).

I'm not sure the current situation is significantly worse than a bunch of impossible "span_bug" code.

If visit_lifetime_def is removed and visit_generic_param is uniformly used for visiting (#45930 (comment)) then Generics here could be replaced with Vec<GenericParam> improving situation somewhat.

FWIW I expect @nikomatsakis would eventually want HRTB to include where clauses too, on top of for<T: Trait>, but I don't know the exact plans.
Vec<GenericParam> is fine with me too.

petrochenkov · 2017-11-11T13:25:59Z

src/libsyntax/ast.rs

 #[derive(Clone, PartialEq, Eq, RustcEncodable, RustcDecodable, Hash, Debug)]
 pub struct Generics {
-    pub lifetimes: Vec<LifetimeDef>,
-    pub ty_params: Vec<TyParam>,
+    pub params: Vec<GenericParam>,


Also the invariant "all lifetime parameters go before type parameters" is lost in the new AST/HIR (enforced only by parser).
I don't remember if it was relied on anywhere, in matching against generic arguments maybe?

What happens if a syntax extension generates parameters in intermixed order and send them to further compilation?

ast_validation should probably enforce the limitation until we want to lift it.

bors · 2017-11-12T04:18:17Z

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

alexcrichton · 2017-11-12T18:37:16Z

r? @eddyb

(just making sure there's an assigned reviewer)

eddyb · 2017-11-17T23:30:17Z

src/librustc/hir/map/collector.rs

@@ -309,7 +309,7 @@ impl<'a, 'hir> Visitor<'hir> for NodeCollector<'a, 'hir> {
    }

    fn visit_generics(&mut self, generics: &'hir Generics) {
-        for ty_param in generics.ty_params.iter() {
+        for ty_param in generics.ty_params() {


This should be uniform and have a single Node variant instead of NodeTyParam and NodeLifetimeParam.

bors · 2017-11-21T12:36:07Z

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

carols10cents · 2017-11-27T14:41:56Z

@jplatte friendly triage ping! how's this going? Looks like there are some merge conflicts for you!

jplatte · 2017-11-27T15:26:39Z

I think the test failures are the bigger issue.. I've rebased a bunch of times and I will do that again now. But I've put some time into figuring out why some tests fail and haven't been able to figure it out yet. I have one idea still, but I'm not that optimistic...

eddyb · 2017-11-29T22:38:28Z

src/librustc/hir/lowering.rs

+                                                if node_id == ty_param.id {
+                                                    add_bounds.entry(ty_param.id)
+                                                        .or_insert(Vec::new())
+                                                        .push(bound.clone());


FWIW cloning isn't necessary here if the reference can be used directly.

eddyb · 2017-11-29T22:39:19Z

src/librustc/hir/map/def_collector.rs


+    fn visit_generics(&mut self, generics: &'a Generics) {
        visit::walk_generics(self, generics);


This can be removed, since walking is the default action.

eddyb · 2017-11-29T22:40:05Z

src/librustc/hir/mod.rs

    }

    pub fn is_parameterized(&self) -> bool {
-        self.is_lt_parameterized() || self.is_type_parameterized()
+        !self.params.is_empty()


Could maybe inline this, since it's about as long.

eddyb · 2017-11-29T22:42:40Z

src/librustc/middle/resolve_lifetime.rs

-                    this.check_lifetime_defs(old_scope, &generics.lifetimes);
+                    this.check_lifetime_defs(
+                        old_scope,
+                        &generics.lifetimes().cloned().collect::<Vec<_>>(),


Should avoid doing this cloning, preferably just pass in the &[GenericParam].

generics.lifetimes().collect::<Vec<_>>()[..] (without .cloned()) is a &[&GenericParam] though, is there a way of converting that or would I have to make check_lifetime_defs generic over Iterator<Item = &GenericParam>?

I'd rename check_lifetime_defs to check_lifetime_params and have it take the entire &[GenericParam], and use .lifetimes() inside itself.

eddyb · 2017-11-29T22:43:23Z

src/librustc/middle/resolve_lifetime.rs

-                        }).collect(),
+                    lifetimes: c.generic_params
+                        .iter()
+                        .flat_map(|param| if let hir::GenericParam::Lifetime(ref def) = *param {


Hmm, you're not using .lifetimes() here, but you are above, is that intentional?

Can't because this is a Vec<GenericParam>, not a Generics instance. In my WIP rebase, this case actually came up a bunch more and I added a GenericParmsExt to deal with this (where I had to write out the FlatMap type bc. impl trait in traits isn't available yet), but then came to realize that it's sometimes hir::GenericParam and sometimes ast::GenericParam.

At least in this file it's always HIR.

I just re-checked and it's actually almost always HIR, just in the form of P<[GenericParam]> in some cases. Only very few things that were new when I started the rebase were in the AST (in the ast -> hir lowering specifically).

You can impl the trait on [GenericParam] directly.

eddyb · 2017-11-29T22:44:37Z

src/librustc/middle/resolve_lifetime.rs

+                        &c.generic_params
+                            .iter()
+                            .flat_map(|param| {
+                                if let hir::GenericParam::Lifetime(ref def) = *param {


Same as #45930 (comment).

eddyb · 2017-11-29T22:45:26Z

src/librustc/middle/resolve_lifetime.rs

+                    ref bound_generic_params,
+                    ..
+                }) => {
+                    if bound_generic_params.iter().find(|p| p.is_lifetime_param()).is_some() {


This is just .any(|p| p.is_lifetime_param()), right?

eddyb · 2017-11-29T22:45:53Z

src/librustc/middle/resolve_lifetime.rs

-                                }).collect(),
+                            lifetimes: bound_generic_params.iter()
+                                .flat_map(|param| {
+                                    if let hir::GenericParam::Lifetime(ref def) = *param {


Same as #45930 (comment).

eddyb · 2017-11-29T22:46:19Z

src/librustc/middle/resolve_lifetime.rs

+                                old_scope,
+                                &bound_generic_params.iter()
+                                    .flat_map(|param| {
+                                        if let hir::GenericParam::Lifetime(ref ld) = *param {


Same as #45930 (comment).

eddyb · 2017-11-29T22:46:41Z

src/librustc/middle/resolve_lifetime.rs

@@ -519,22 +557,38 @@ impl<'a, 'tcx> Visitor<'tcx> for LifetimeContext<'a, 'tcx> {
                            _modifier: hir::TraitBoundModifier) {
        debug!("visit_poly_trait_ref trait_ref={:?}", trait_ref);

-        if !self.trait_ref_hack || !trait_ref.bound_lifetimes.is_empty() {
+        if !self.trait_ref_hack ||
+            trait_ref.bound_generic_params.iter().find(|p| p.is_lifetime_param()).is_some()


Same as #45930 (comment).

eddyb · 2017-11-29T22:47:10Z

src/librustc/middle/resolve_lifetime.rs

-                    }).collect(),
+                lifetimes: trait_ref.bound_generic_params.iter()
+                    .flat_map(|param| {
+                        if let hir::GenericParam::Lifetime(ref def) = *param {


Same as #45930 (comment).

eddyb · 2017-11-29T22:48:34Z

src/librustc/middle/resolve_lifetime.rs

+                    old_scope,
+                    &trait_ref.bound_generic_params.iter()
+                        .flat_map(|param| {
+                            if let hir::GenericParam::Lifetime(ref ld) = *param {


Same as #45930 (comment).

bors · 2017-12-21T02:05:29Z

⌛ Testing commit b43275f9081821b03058688c683a1f4383cfcec9 with merge 92c7dd4b58c9cdd56633015888cb25284ab1496a...

bors · 2017-12-21T02:48:13Z

💔 Test failed - status-travis

eddyb · 2017-12-21T02:52:27Z

I think you need to rebase, because some NLL testcases got added just now and they break.

eddyb · 2017-12-21T10:30:49Z

@bors r+

bors · 2017-12-21T10:30:49Z

📌 Commit b9e4b30 has been approved by eddyb

bors · 2017-12-21T10:56:41Z

🔒 Merge conflict

bors · 2017-12-21T11:00:13Z

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

The Generics now contain one Vec of an enum for the generic parameters, rather than two separate Vec's for lifetime and type parameters. Additionally, places that previously used Vec<LifetimeDef> now use Vec<GenericParam> instead.

eddyb · 2017-12-21T14:19:49Z

@bors r+

bors · 2017-12-21T14:19:49Z

📌 Commit 78493ed has been approved by eddyb

bors · 2017-12-21T20:12:20Z

⌛ Testing commit 78493ed with merge 250b492...

@eddyb

Generics refactoring (groundwork for const generics) These changes were suggested by @eddyb. After this change, the `Generics` contain one `Vec` of an enum for the generic parameters, rather than two separate `Vec`s for lifetime and type parameters. Type params and const params will need to be in a shared `Vec` to preserve their ordering, and moving lifetimes into the same `Vec` should simplify the code that processes `Generics`.

bors · 2017-12-21T23:01:22Z

☀️ Test successful - status-appveyor, status-travis
Approved by: eddyb
Pushing 250b492 to master...

introduced in rust-lang/rust#45930

@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

eddyb reviewed Nov 11, 2017

View reviewed changes

petrochenkov reviewed Nov 11, 2017

View reviewed changes

kennytm added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 11, 2017

jplatte mentioned this pull request Nov 11, 2017

Tracking issue for const generics (RFC 2000) #44580

Closed

16 tasks

eddyb mentioned this pull request Nov 11, 2017

Update AST to hold const generics dtolnay/syn#227

Closed

rust-highfive assigned eddyb Nov 12, 2017

kennytm 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 Nov 12, 2017

dtolnay mentioned this pull request Nov 13, 2017

Groundwork for const generics dtolnay/syn#247

Closed

eddyb reviewed Nov 17, 2017

View reviewed changes

eddyb reviewed Nov 29, 2017

View reviewed changes

kennytm added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Dec 21, 2017

kennytm added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 21, 2017

kennytm 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Dec 21, 2017

kennytm added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 21, 2017

bors merged commit 78493ed into rust-lang:master Dec 21, 2017

bors mentioned this pull request Dec 21, 2017

Remove impl Foo for .. {} in favor auto trait Foo {} #46480

Closed

Manishearth added a commit to rust-lang/rust-clippy that referenced this pull request Dec 22, 2017

Update to handle GenericParam

7e09990

introduced in rust-lang/rust#45930

varkor mentioned this pull request Feb 11, 2018

The Great Generics Generalisation: HIR Edition #48149

Merged

petrochenkov mentioned this pull request Mar 6, 2018

Warn about ignored generic bounds in for #48326

Merged

jplatte deleted the generics_refactoring branch April 25, 2018 10:35

petrochenkov mentioned this pull request May 28, 2019

[wip] Rework how type params are parsed #61254

Closed


		fn visit_generics(&mut self, generics: &'a Generics) {
		visit::walk_generics(self, generics);

Generics refactoring (groundwork for const generics) #45930

Generics refactoring (groundwork for const generics) #45930

Conversation

jplatte commented Nov 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eddyb Nov 11, 2017 • 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

petrochenkov Nov 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented Nov 12, 2017

alexcrichton commented Nov 12, 2017

Choose a reason for hiding this comment

bors commented Nov 21, 2017

carols10cents commented Nov 27, 2017

jplatte commented Nov 27, 2017 • 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

jplatte Nov 29, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jplatte Nov 29, 2017 • 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

Choose a reason for hiding this comment

bors commented Dec 21, 2017

bors commented Dec 21, 2017

eddyb commented Dec 21, 2017

eddyb commented Dec 21, 2017

bors commented Dec 21, 2017

bors commented Dec 21, 2017

bors commented Dec 21, 2017

eddyb commented Dec 21, 2017

bors commented Dec 21, 2017

bors commented Dec 21, 2017

bors commented Dec 21, 2017

jplatte commented Nov 11, 2017 •

edited

Loading

eddyb Nov 11, 2017 •

edited

Loading

petrochenkov Nov 11, 2017 •

edited

Loading

jplatte commented Nov 27, 2017 •

edited

Loading

jplatte Nov 29, 2017 •

edited

Loading

jplatte Nov 29, 2017 •

edited

Loading