#37653 support `default impl` for specialization #37860

giannicic · 2016-11-18T16:22:35Z

this commit implements the first step of the default impl feature:

all items in a default impl are (implicitly) default and hence
specializable.

In order to test this feature I've copied all the tests provided for the
default method implementation (in run-pass/specialization and
compile-fail/specialization directories) and moved the default keyword
from the item to the impl.
See referenced issue for further info

r? @aturon

rust-highfive · 2016-11-18T16:22:41Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @aturon (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

giannicic · 2016-11-18T16:38:38Z

AppVeyor was unable to build non-mergeable pull request.

Should I resolve the merge and commit again?

bluss · 2016-11-20T17:00:39Z

@giannicic It's best to rebase your branch and force push it, please don't insert extra merges (unless absolutely necessary)

aturon · 2016-11-22T02:42:09Z

Hi @giannicic! I'm so excited to see this PR!

I'm also in the process of moving house this week. I will try my best to review this PR in the next couple of days, but there's a chance I won't be able to get to it until next week. Apologies in advance if that happens -- I'm eager to see this land.

giannicic · 2016-11-22T12:58:47Z

No problem @aturon !
In the meantime I'll do another rebase to restore the llvm-3.7 support fixed yesterday in order to let travis-ci build this PR properly.

mrhota · 2016-12-21T20:11:46Z

@aturon @giannicic any updates?

bors · 2017-01-01T20:50:12Z

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

giannicic · 2017-01-26T18:44:00Z

Hi @aturon
It has been a long time since your last check.
Is there something wrong with this PR?

mtanski · 2017-01-31T18:57:46Z

I'd like to try using this in my code (it's nightly only)

tupshin · 2017-02-13T02:09:24Z

I too am interested in trying this out. Anything blocking it from merging into nightly?

mrhota · 2017-03-28T03:06:00Z

@aturon ping

nagisa

Thanks for working on this. As aturon seems to be occupied with other stuff i took liberty to review this PR myself. Overall great job. I found changes to the parser somewhat dubious but the rest looks great.

Please add a test for variations of pub unsafe default impl and optionally update src/grammar/parser-lalr.y as well to reflect the default impl.

nagisa · 2017-04-07T06:47:26Z

src/librustc/hir/lowering.rs

                let new_impl_items = impl_items.iter()
                                               .map(|item| self.lower_impl_item_ref(item))
                                               .collect();
                let ifce = ifce.as_ref().map(|trait_ref| self.lower_trait_ref(trait_ref));
                hir::ItemImpl(self.lower_unsafety(unsafety),
                              self.lower_impl_polarity(polarity),
+                              self.lower_defaultness(defaultness, true /* [1] */),


This line seemingly refers to some comment ([1]) but the footnote text does not seem to exist?

Originally I've copied the lower_defaultness method from line 1428
where there is a footnote explaining the reason for setting true in the has_value field of hir::Defaultness.
In my case the has_value method is never called in an impl's defaultness but i'll leave it as always true in order to not cause an assertion failure inside the lower_defaultness function.
I'm putting a footnote to explain this.

nagisa · 2017-04-07T06:50:02Z

src/librustc/traits/project.rs

@@ -953,7 +954,28 @@ fn assemble_candidates_from_impls<'cx, 'gcx, 'tcx>(
                        // being invoked).
                        node_item.item.defaultness.has_value()
                    } else {
-                        node_item.item.defaultness.is_default()
+                        let is_default = match selcx.tcx()


Checking whether method is default should probably factored out somewhere. Maybe TyCtxt?

Ok, I'm putting the TyCtxt's method inside src/librustc/traits/util.rs
the method signature is the following:
pub fn impl_is_default(self, node_item_def_id: DefId) -> bool

nagisa · 2017-04-07T06:53:47Z

src/librustc_metadata/encoder.rs

@@ -668,14 +668,15 @@ impl<'a, 'tcx> EncodeContext<'a, 'tcx> {
            hir::ItemDefaultImpl(..) => {
                let data = ImplData {
                    polarity: hir::ImplPolarity::Positive,
+                    defaultness: hir::Defaultness::Final,


Are you sure about this being Final? Does this mean that default impl Trait for .. {} is not possible even with this PR?

@nagisa I've read the discussion about UnwindSafe #40628, this PR however is one of a set of tasks described here #37653,
the 3rd

all items in a default impl are (implicitly) default and hence specializable

It doesn't address default trait implementations.
At least another PR must follow in order to cover the other two points.
so we could define better what @abonander says about

re-addressed this in the context of specialization

and add it to the task list.
What do you think?

EDIT: I've placed an error during parsing to avoid default impl Trait for .. {}

I’m not sure this PR is a right place to discuss UnwindUnsafe. If you have any ideas, you should post post them on the UnwindUnsafe issue.

I agree. After this PR I'll have a better look at UnwindUnsafe issue

nagisa · 2017-04-07T06:54:25Z

src/librustc_typeck/check/mod.rs

@@ -1041,7 +1041,23 @@ fn check_specialization_validity<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>,

    if let Some(parent) = parent {
        if parent.item.is_final() {
-            report_forbidden_specialization(tcx, impl_item, parent.node.def_id());
+            let is_final = match tcx.map.as_local_node_id(parent.node.def_id()) {


Similarly should be factored out.

Ok, It uses the impl_is_default above

nagisa · 2017-04-07T06:56:34Z

src/libsyntax/feature_gate.rs

@@ -1136,6 +1136,15 @@ impl<'a> Visitor<'a> for PostExpansionVisitor<'a> {
                    },
                    _ => {}
                }
+
+                match defaultness {


if let hir::Defaultness::Default = ... { ... }

@nagisa Not sure I've understood this. Why should I check for hir::Defaultness in libsyntax?

What I meant here is that this match should be replaced with a if let. hir here was my mistake, sorry.

nagisa · 2017-04-07T07:30:53Z

src/libsyntax/parse/parser.rs

@@ -5809,13 +5811,19 @@ impl<'a> Parser<'a> {
                                    maybe_append(attrs, extra_attrs));
            return Ok(Some(item));
        }
-        if self.check_keyword(keywords::Unsafe) &&
-            self.look_ahead(1, |t| t.is_keyword(keywords::Impl))
+        if (self.check_keyword(keywords::Unsafe) &&


It seems wrong to me? At the very least lookahead of 2 tokens is unnecessary.

I'm adding a test taken from src/test/run-pass/specialization/defaultimpl/specialization-basics.rs putting unsafe to all occurences of default impl.
Probably it's incorrect to look for default unsafe impl, I'm changing in order to check for unsafe default impl.

Probably it's incorrect to look for default unsafe impl, I'm changing in order to check for unsafe default impl.

For all it is worth, the syntax for functions is like this:

[default] [unsafe] fn foo...

so default impl should match it and be

[default] [unsafe] impl

Okay, I guess 2 look aheads is fine for now, although it is pretty obvious this needs some refactor, which I’ll be doing later anyway.

giannicic · 2017-04-11T16:12:13Z

@nagisa
I'll look at your review and do a rebase as well
Thank you!

arielb1 · 2017-04-18T08:59:01Z

Ping @giannicic. Are you still working on this?

giannicic · 2017-04-18T09:10:24Z

Yes @arielb1
I'm finishing and testing the rebase. Then I'll look at @nagisa reviews.

arielb1 · 2017-04-18T10:07:36Z

Just wanted to make sure this PR isn't getting lost.

this commit implements the first step of the `default impl` feature: all items in a `default impl` are (implicitly) `default` and hence specializable. In order to test this feature I've copied all the tests provided for the `default` method implementation (in run-pass/specialization and compile-fail/specialization directories) and moved the `default` keyword from the item to the impl. See referenced issue for further info

fix tidy

rebase after support for llvm-3.7

pr review

giannicic · 2017-04-25T06:54:31Z

@nagisa
PR rebased and updated accordingly to your review.
I've updated the parser-lalr.y as well.
Thanks

nagisa

So there’s two blockers at the moment:

[default] [unsafe] impl vs [unsafe] [default] impl. The PR as is currently uses one in some places and the other in another places. I think it should be [default] [unsafe] impl just because that’s the order used by default unsafe fn.
The fact that something like typeck has to understand different ways a method could become default. This is a recipe for disaster (read: discrepancies in behaviour for code that is supposedly equivalent in behaviour).

All other comments unrelated to these two problems are nits and do not block landing this PR.

nagisa · 2017-04-25T10:36:52Z

src/grammar/parser-lalr.y

@@ -588,27 +594,27 @@ impl_method
 // they are ambiguous with traits. We do the same here, regrettably,
 // by splitting ty into ty and ty_prim.
 item_impl
-: maybe_unsafe IMPL generic_params ty_prim_sum maybe_where_clause '{' maybe_inner_attrs maybe_impl_items '}'
+: maybe_unsafe maybe_default_impl generic_params ty_prim_sum maybe_where_clause '{' maybe_inner_attrs maybe_impl_items '}'


See this comment

nagisa · 2017-04-25T10:37:22Z

src/librustc/hir/print.rs

                          ref generics,
                          ref opt_trait,
                          ref ty,
                          ref impl_items) => {
                self.head("")?;
                self.print_visibility(&item.vis)?;
+                self.print_defaultness(defaultness)?;


This has a different order compared to the yacc grammar.

I've changed the parser-lalr.y accordingly to the [default] [unsafe] impl order so this file should be ok now

nagisa · 2017-04-25T10:51:11Z

src/librustc_typeck/check/mod.rs

-            report_forbidden_specialization(tcx, impl_item, parent.node.def_id());
+            if !tcx.impl_is_default(parent.node.def_id()) {
+                report_forbidden_specialization(tcx, impl_item, parent.node.def_id());
+            }


I feel that this change should not exist. That is, typecheck should not have to make the distinction between x being default because it is in a default impl and x being default because it is default fn:

default impl Foo for Bar { fn x() { ... } fn y() { ... } } impl Foo for Bar { default fn x() { ... } default fn y() { ... } }

One approach would be to change the is_final somehow, to automatically check for both scenarios. Another would be to propagate defaultness from impl to its children items during some lowering step (say, from AST to HIR).

@nagisa
When i first start to do this PR I was thinking about some propagation during lowering but I choose to not do it since I should have done something on hir/print.rs in order to show the defaultness info before the lowering occurred. An is_final method should be placed on the NodeItem struct, somthing like the following

impl<'a, 'gcx, 'tcx> NodeItem<hir::Defaultness> { pub fn is_final(&self, tcx: TyCtxt<'a, 'gcx, 'tcx>) -> bool { self.item.is_final() && !tcx.impl_is_default(self.node.def_id()) } }

Then in typecheck just call parent.is_final(tcx)

Do you think it could be ok?

nagisa · 2017-04-25T10:54:33Z

src/libsyntax/parse/parser.rs

+                                               default trait implementations");
+                }
+                _ => {}
+            }


I was going to say that parser is certainly the wrong place for such a check, but then I noticed we already have a number of similar checks just above. I guess it is fine then…?

This match should be an if let instead.

nagisa · 2017-04-25T11:08:09Z

src/libsyntax/parse/parser.rs

@@ -5809,13 +5811,19 @@ impl<'a> Parser<'a> {
                                    maybe_append(attrs, extra_attrs));
            return Ok(Some(item));
        }
-        if self.check_keyword(keywords::Unsafe) &&
-            self.look_ahead(1, |t| t.is_keyword(keywords::Impl))
+        if (self.check_keyword(keywords::Unsafe) &&


Okay, I guess 2 look aheads is fine for now, although it is pretty obvious this needs some refactor, which I’ll be doing later anyway.

giannicic · 2017-04-26T12:14:44Z

@nagisa
Updated:
[default] [unsafe] impl is the current parsing order
there is a single is_final method during typechecking

nagisa

r=me with is_final method being moved to a method on TyCtxt. (either in librustc::ty::mod, librustc::ty::util or librustc::ty::context is fine IMO).

nagisa · 2017-04-26T12:22:28Z

src/librustc/traits/util.rs

+}
+
+impl<'a, 'gcx, 'tcx> NodeItem<hir::Defaultness> {
+    pub fn is_final(&self, tcx: TyCtxt<'a, 'gcx, 'tcx>) -> bool {


I would probably have put this as a method on TyCtxt instead, as that’s how pretty much all such queries are implemented.

Ok, I'm moving it on librustc::ty::util

`[default] [unsafe] impl` and typecheck

giannicic · 2017-04-26T16:23:22Z

is_final moved inside TyCtxt. The method signature is the following:

pub fn impl_item_is_final(self, node_item: &NodeItem<hir::Defaultness>) -> bool {
    node_item.item.is_final() && !self.impl_is_default(node_item.node.def_id())
}

Thanks
@bors r=nagisa

nagisa · 2017-04-26T16:39:10Z

@bors r+

Thanks!

bors · 2017-04-26T16:39:10Z

📌 Commit b48eb5e has been approved by nagisa

nagisa · 2017-04-26T16:41:41Z

I gotta say this PR has the best"est" amount of tests I’ve ever seen – a lot. Props for that.

bors · 2017-04-27T02:48:23Z

⌛ Testing commit b48eb5e with merge 54ef800...

@aturon

#37653 support `default impl` for specialization this commit implements the first step of the `default impl` feature: > all items in a `default impl` are (implicitly) `default` and hence > specializable. In order to test this feature I've copied all the tests provided for the `default` method implementation (in run-pass/specialization and compile-fail/specialization directories) and moved the `default` keyword from the item to the impl. See [referenced](#37653) issue for further info r? @aturon

bors · 2017-04-27T05:14:53Z

☀️ Test successful - status-appveyor, status-travis
Approved by: nagisa
Pushing 54ef800 to master...

rust-highfive assigned aturon Nov 18, 2016

giannicic force-pushed the defaultimpl branch from 3c8225a to eb19417 Compare November 21, 2016 01:59

giannicic force-pushed the defaultimpl branch from 8bb23ee to 014d87b Compare November 22, 2016 18:25

giannicic force-pushed the defaultimpl branch from 014d87b to 545817f Compare January 7, 2017 13:58

abonander mentioned this pull request Mar 19, 2017

UnwindSafe is unergonomic #40628

Open

nagisa reviewed Apr 7, 2017

View reviewed changes

Mark-Simulacrum added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Apr 14, 2017

aturon assigned nagisa and unassigned aturon Apr 17, 2017

giannicic added 4 commits April 24, 2017 10:08

support default impl for specialization

6427fdc

fix tidy

support default impl for specialization

b0fca5f

rebase after support for llvm-3.7

support default impl for specialization

715811d

pr review

giannicic force-pushed the defaultimpl branch from 545817f to 715811d Compare April 25, 2017 05:30

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

nagisa requested changes Apr 25, 2017

View reviewed changes

giannicic force-pushed the defaultimpl branch 2 times, most recently from fc5133d to d110627 Compare April 26, 2017 10:17

nagisa approved these changes Apr 26, 2017

View reviewed changes

support default impl for specialization

b48eb5e

`[default] [unsafe] impl` and typecheck

giannicic force-pushed the defaultimpl branch from d110627 to b48eb5e Compare April 26, 2017 14:44

bors merged commit b48eb5e into rust-lang:master Apr 27, 2017

giannicic deleted the defaultimpl branch April 27, 2017 07:30

hcpl mentioned this pull request Jun 1, 2017

Accommodate to recent changes gsingh93/trace#10

Merged

giannicic mentioned this pull request Oct 29, 2017

#37653 support default impl for specialization #45404

Merged

#37653 support default impl for specialization #37860

#37653 support default impl for specialization #37860

Conversation

giannicic commented Nov 18, 2016 • edited Loading

rust-highfive commented Nov 18, 2016

giannicic commented Nov 18, 2016

bluss commented Nov 20, 2016

aturon commented Nov 22, 2016

giannicic commented Nov 22, 2016

mrhota commented Dec 21, 2016

bors commented Jan 1, 2017

giannicic commented Jan 26, 2017

mtanski commented Jan 31, 2017

tupshin commented Feb 13, 2017

mrhota commented Mar 28, 2017

nagisa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

giannicic Apr 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

giannicic Apr 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

giannicic Apr 21, 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

giannicic Apr 19, 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

nagisa Apr 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

giannicic commented Apr 11, 2017

arielb1 commented Apr 18, 2017

giannicic commented Apr 18, 2017

arielb1 commented Apr 18, 2017

giannicic commented Apr 25, 2017

nagisa left a 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

giannicic Apr 25, 2017 • edited Loading

Choose a reason for hiding this comment

nagisa Apr 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

giannicic commented Apr 26, 2017

nagisa left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

giannicic commented Apr 26, 2017

nagisa commented Apr 26, 2017

bors commented Apr 26, 2017

nagisa commented Apr 26, 2017

bors commented Apr 27, 2017

bors commented Apr 27, 2017

#37653 support `default impl` for specialization #37860

#37653 support `default impl` for specialization #37860

giannicic commented Nov 18, 2016 •

edited

Loading

giannicic Apr 19, 2017 •

edited

Loading

giannicic Apr 19, 2017 •

edited

Loading

giannicic Apr 21, 2017 •

edited

Loading

giannicic Apr 19, 2017 •

edited

Loading

nagisa Apr 25, 2017 •

edited

Loading

giannicic Apr 25, 2017 •

edited

Loading

nagisa Apr 25, 2017 •

edited

Loading

nagisa left a comment •

edited

Loading