Demote self to an (almost) regular argument and remove the env param. by eddyb · Pull Request #11595 · rust-lang/rust

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

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

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

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

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.

erickt reviewed Jan 17, 2014
View reviewed changes

src/libsyntax/print/pprust.rs Outdated

Copy link

Contributor

erickt Jan 17, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor nit: there is a typo in separately here.

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

Uh oh!

Conversation

eddyb commented Jan 16, 2014

Uh oh!

erickt Jan 17, 2014

Choose a reason for hiding this comment

Uh oh!

alexcrichton commented Jan 17, 2014

Uh oh!

erickt commented Jan 17, 2014

Uh oh!

erickt Jan 17, 2014

Choose a reason for hiding this comment

Uh oh!

nikomatsakis Jan 24, 2014

Choose a reason for hiding this comment

Uh oh!

erickt commented Jan 17, 2014

Uh oh!

nikomatsakis commented Jan 17, 2014

Uh oh!

nikomatsakis commented Jan 17, 2014

Uh oh!

eddyb commented Jan 18, 2014

Uh oh!

brson commented Jan 21, 2014

Uh oh!

nikomatsakis Jan 23, 2014

Choose a reason for hiding this comment

Uh oh!

eddyb Jan 23, 2014

Choose a reason for hiding this comment

Uh oh!

nikomatsakis commented Jan 24, 2014

Uh oh!

nikomatsakis Jan 24, 2014

Choose a reason for hiding this comment

Uh oh!

nikomatsakis commented Jan 25, 2014

Uh oh!

eddyb commented Jan 26, 2014

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants