Demote self to an (almost) regular argument and remove the env param. #11595

eddyb · 2014-01-16T06:16:57Z

Non-exhaustive change list:

self is now present in argument lists (modulo type-checking code I don't trust myself to refactor)
methods have the same calling convention as bare functions (including the self argument)
the env param is gone from all bare functions (and methods), only used by closures and procs
bare functions can only be coerced to closures and procs if they are statically resolved, as they now require creating a wrapper specific to that function, to avoid indirect wrappers (equivalent to impl<..Args, Ret> Fn<..Args, Ret> for fn(..Args) -> Ret) that might not be optimizable by LLVM and don't work for procs
refactored some trans::closure code, leading to the removal of trans::glue::make_free_glue and ty_opaque_closure_ptr

erickt · 2014-01-17T15:23:44Z

src/libsyntax/print/pprust.rs

    }

-    for arg in decl.inputs.iter() {
+    // HACK(eddyb) ignore the sepparately printed self argument.


Minor nit: there is a typo in separately here.

alexcrichton · 2014-01-17T17:34:52Z

I don't really consider myself able to review, but it would be nice to have a summary of changes as opposed to "No description given" with one monolithic commit.

It also looks like this changes language semantics (bare function pointers can no longer coerce to a closure?), so this should probably be discussed.

cc @nikomatsakis

erickt · 2014-01-17T17:37:44Z

@eddyb: I'm very excited for this as it brings us that much closer to universal function call syntax (#6974). Do you have a sense of how much more work beyond this patch is required to support it?

erickt · 2014-01-17T18:14:38Z

src/librustc/middle/cfg/construct.rs

-            ast::ExprMethodCall(_, rcvr, _, _, ref args, _) => {
-                self.call(expr, pred, rcvr, *args)
+            ast::ExprMethodCall(_, _, _, ref args, _) => {
+                self.call(expr, pred, args[0], args.slice_from(1))


It makes me a bit weary seeing these args[0] all over the place. It seems like it could be pretty easy to treat a regular fn call's args[0] as a receiver, or forget to extract out the receiver from a method call. Since there are now only a few cases where regular fn arguments seem to be different from method arguments, could we combine the two into a ast::CallArgs enum? That might help make things more clear when we are dealing with a special case.

actually it seems like moving the self into the args list makes things cleaner overall, I'd say

erickt · 2014-01-17T18:16:55Z

cc @luqmana, who has also mucked around with the self argument, and might have a good eye to review this as well.

nikomatsakis · 2014-01-17T21:35:46Z

I will review (not to say others shouldn't read).

nikomatsakis · 2014-01-17T21:37:40Z

And yes, this will change language semantics -- per our recent discussion in meeting, I was initially opposed, but was swayed by @eddyb's argument concerning possible future extension to a Fn trait. I'll bring it up next meeting, but review in the meantime, because I don't anticipate major objections, given the tenor of our last discussion.

eddyb · 2014-01-18T18:41:31Z

@erickt about UFCS - only thing left is to allow calling them as functions - see error: first-class methods are not supported in middle::resolve.
Feel free to bring it up for discussion.

brson · 2014-01-21T18:16:23Z

We discussed this in today's meeting and agreed it's fine.

nikomatsakis · 2014-01-23T05:04:34Z

src/librustc/middle/borrowck/check_loans.rs

@@ -777,50 +774,41 @@ fn check_loans_in_expr<'a>(this: &mut CheckLoanCtxt<'a>,

    let method_map = this.bccx.method_map.borrow();
    match expr.node {
-      ast::ExprSelf |


Can you please revert this reformatting? I don't see any changes here and I'd rather avoid unnecessary conflicts.

I may have actually hit this during rebasing, will revert (there's actually a couple changes in there).

nikomatsakis · 2014-01-24T03:07:12Z

Still reading. Probably 50% of the way through (though I think I'm past the hard bits). Too tired to finish tonight, hopefully tomorrow morning. Really nice.

nikomatsakis · 2014-01-24T16:39:22Z

src/librustc/middle/typeck/check/method.rs

-        debug!("after subst, fty={}", self.ty_to_str(fty));
-
-        // Replace any bound regions that appear in the function
-        // signature with region variables
        let bare_fn_ty = match ty::get(fty).sty {


Under the new flow, bare_fn_ty here is always equal to candidate.method_ty.fty. Moreover, the construction of fty at the top of the function is unnecessary; it is only used here (where it is not needed) and in enforce_object_limitations(), where it appears it is not needed. I'd rather we just strip it out. The logic of this function is complex enough that unnecessary variables is rather distracting.

Perhaps add this comment:

// This method performs two sets of substitutions, one after the other: // 1. Substitute values for any type/lifetime parameters from the impl and method declaration into the method type. // This is the function type before it is called; it may still include late bound region variables. // 2. Instantiate any late bound lifetime parameters in the method itself with fresh region variables.

nikomatsakis · 2014-01-25T02:14:13Z

OK, I finished the review. Very nice work. r+ given that comments are addressed and the following tests are added:

A compile-fail test checking that coercion from a variable of fn() type fails to coerce to a closure and fails to coerce to a proc. (name it someting like coerce-bare-fn-to-closure-and-proc.rs)
run-pass tests covering coercion from fn items, variant constructors, and tuple structs. I'd be particularly interested to see tests for generic fns and the cross-crate case as well (iiuc, this results in generating a wrapper in the second crate, not the crate where the struct is actually defined, right?).

Sorry it took me some time! A lot of changes here, but good stuff to be sure.

eddyb · 2014-01-26T21:50:53Z

Seems I broke the test for #6919 (though it didn't actually fail locally - maybe it never ran as part of make check-stage1-rpass).

The issue is with a public static coercing a private bare fn to a closure in a crate. When the static is used from another crate, it's recreated.
Previously, the bare fn was kept alive by the static (in the first crate), now only the bare-fn-to-closure wrapper is kept alive.
And when the static is recreated in another crate, it's recreating the wrapper, which references the function from the first crate, which is no longer kept alive, so we get a linker error.

I am not sure how to solve this, short of a wrapper cache mapping bare function symbol names to wrapper symbol names, exported in crate metadata, but that's a bit excessive, I would say.

huonw · 2014-01-27T05:38:17Z

Can we make the static keep alive the bare fn too?

eddyb · 2014-01-27T05:46:36Z

@huonw it's all happening inside LLVM, both the wrapper and the original function are internal.
EDIT: I think I've solved this, propagating the fact that it's inside an inlined static, all the way to closure::get_wrapper_for_bare_fn.

nikomatsakis · 2014-01-27T10:47:10Z

It seems like this is an error in the reachability computation.

Fixes rust-lang#10667 and closes rust-lang#10259.

Non-exhaustive change list: * `self` is now present in argument lists (modulo type-checking code I don't trust myself to refactor) * methods have the same calling convention as bare functions (including the self argument) * the env param is gone from all bare functions (and methods), only used by closures and `proc`s * bare functions can only be coerced to closures and `proc`s if they are statically resolved, as they now require creating a wrapper specific to that function, to avoid indirect wrappers (equivalent to `impl<..Args, Ret> Fn<..Args, Ret> for fn(..Args) -> Ret`) that might not be optimizable by LLVM and don't work for `proc`s * refactored some `trans::closure` code, leading to the removal of `trans::glue::make_free_glue` and `ty_opaque_closure_ptr`

…et,GuillaumeGomez Allow more attributes in `clippy::useless_attribute` Fixes rust-lang#12753 Fixes rust-lang#4467 Fixes rust-lang#11595 Fixes rust-lang#10878 changelog: [`useless_attribute`]: Attributes allowed on `use` items now include `ambiguous_glob_exports`, `hidden_glob_reexports`, `dead_code`, `unused_braces`, and `clippy::disallowed_types`.

erickt reviewed Jan 17, 2014
View reviewed changes

eddyb mentioned this pull request Jan 22, 2014

Removed take_glue from tydesc. #11723

Merged

nikomatsakis reviewed Jan 23, 2014
View reviewed changes

nikomatsakis reviewed Jan 24, 2014
View reviewed changes

Demote self to an (almost) regular argument and remove the env param.

15ba0c3

Fixes rust-lang#10667 and closes rust-lang#10259.

bors closed this Jan 27, 2014

bors merged commit 15ba0c3 into rust-lang:master Jan 27, 2014

eddyb deleted the env-et-self-no-more branch January 27, 2014 18:51

petrochenkov mentioned this pull request May 18, 2016

Remove ExplicitSelf from AST #33644

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Demote self to an (almost) regular argument and remove the env param. #11595

Demote self to an (almost) regular argument and remove the env param. #11595

eddyb commented Jan 16, 2014

erickt Jan 17, 2014

alexcrichton commented Jan 17, 2014

erickt commented Jan 17, 2014

erickt Jan 17, 2014

nikomatsakis Jan 24, 2014

erickt commented Jan 17, 2014

nikomatsakis commented Jan 17, 2014

nikomatsakis commented Jan 17, 2014

eddyb commented Jan 18, 2014

brson commented Jan 21, 2014

nikomatsakis Jan 23, 2014

eddyb Jan 23, 2014

nikomatsakis commented Jan 24, 2014

nikomatsakis Jan 24, 2014

nikomatsakis commented Jan 25, 2014

eddyb commented Jan 26, 2014

huonw commented Jan 27, 2014

eddyb commented Jan 27, 2014

nikomatsakis commented Jan 27, 2014

Demote self to an (almost) regular argument and remove the env param. #11595

Demote self to an (almost) regular argument and remove the env param. #11595

Conversation

eddyb commented Jan 16, 2014

erickt Jan 17, 2014

Choose a reason for hiding this comment

alexcrichton commented Jan 17, 2014

erickt commented Jan 17, 2014

erickt Jan 17, 2014

Choose a reason for hiding this comment

nikomatsakis Jan 24, 2014

Choose a reason for hiding this comment

erickt commented Jan 17, 2014

nikomatsakis commented Jan 17, 2014

nikomatsakis commented Jan 17, 2014

eddyb commented Jan 18, 2014

brson commented Jan 21, 2014

nikomatsakis Jan 23, 2014

Choose a reason for hiding this comment

eddyb Jan 23, 2014

Choose a reason for hiding this comment

nikomatsakis commented Jan 24, 2014

nikomatsakis Jan 24, 2014

Choose a reason for hiding this comment

nikomatsakis commented Jan 25, 2014

eddyb commented Jan 26, 2014

huonw commented Jan 27, 2014

eddyb commented Jan 27, 2014

nikomatsakis commented Jan 27, 2014