Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Option to not BeginLift method declarations #8

Closed
schwern opened this issue Mar 21, 2011 · 20 comments
Closed

Option to not BeginLift method declarations #8

schwern opened this issue Mar 21, 2011 · 20 comments

Comments

@schwern
Copy link
Contributor

schwern commented Mar 21, 2011

method and func both happen at compile time, just like sub. This really annoys some people and it's incompatible with MooseX::Method::Signatures.

Make an option so that you can turn this behavior off.

@chloe-zen
Copy link

What does "compatibility" with a competing module even mean? I'm confused.

Also, having it happen at compile time seems like a feature, not a bug. I'm glad you're not removing it but I wonder why anyone would want to.

@schwern
Copy link
Contributor Author

schwern commented Mar 22, 2011

There's a bunch of things going on.

First, I agree. Compile time loading of subroutines is the correct thing. If nothing else, it makes them work just like sub. It will remain the default.

Compatibility with MXMS is part of the plan to get MXMS out of MXD so MXD performs well. I also have an evil plan to write MouseX::Declare which would use MS.

Florian was pretty adamant at the last QA Hackathon that I stop using Devel::BeginLift but never explained why. Still, he's the author and I'd like an escape plan if he decides to stop maintaining it.

He also claimed that nobody wanted to use MS because of Devel::BeginLift... but never explained why. I know there's folks who think sub happening at compile time is a confusing misfeature in Perl, which is arguable... but not a deal breaker. MS isn't here to have that fight, this isn't Sub::At::Runtime, but if they really care that much, fine, turn it off.

And if you want compile time methods, the option to turn it off has no effect.

Finally, it's really trivial to add, it will shut people up and remove a piece of FUD.

@ghost ghost assigned barefootcoder Sep 15, 2011
@barefootcoder
Copy link
Contributor

Bringing the relevant bits of the discussion over from #22:

Do you have an idea of what that interface should look like, or shall I take a crack at it? My first instinct would just be to tack on a NO_BEGIN_LIFT or somesuch to the use line, but ISTR you didn't like that for some reason.

Negative options are not unconfusing, it can instead default to true.

Sure.

AND I SEE NO NEED TO SHOUT.

I don't think of it as shouting; it's a constant. All caps for constants is pretty standard. But I don't really care that much either way. :)

Ideally something more descriptive of what it does, rather than the particular module. Umm... "early_compile" or "compile_like_sub" or "compile_at_begin".

So basically something like:

use Method::Signatures compile_at_BEGIN => 0;

? In this case, I don't think the BEGIN is shouting, but rather matches the BEGIN block, which is essentially what happens with BeginLift (i.e. it acts as if you'd wrapped the declaration in a BEGIN).

Do we want any other ways to affect it? environment variables or anything?

@schwern
Copy link
Contributor Author

schwern commented Sep 15, 2011

While it's technically a constant, more importantly it's an option or switch. For the same reason you don't write ls --COLOR or $obj->method( KEY => "value" ) you wouldn't write use Method::Signatures BEGIN_LIFT => 1.

I do like compile_at_BEGIN and agree BEGIN is not shouting.

I don't see this as the sort of thing you'd want to effect with environment variables. You don't want to affect code not written with this in mind, same as you wouldn't want to turn on strict in an environment variable.

Alas, this reminds me that this behavior will have to be lexical. A simple internal switch won't cut it else multiple uses in one processes will stomp on each other. Crap. That's more than I can think about at the moment.

@barefootcoder
Copy link
Contributor

Not sure how I can see to make it lexical, but I'll think on it.

@schwern
Copy link
Contributor Author

schwern commented Sep 15, 2011

On 2011.9.15 12:47 AM, Buddy Burden wrote:

Not sure how I can see to make it lexical, but I'll think on it.

I started feature/compile_at_BEGIN to work on it.

Normally we'd use %^H (see perlpragma) but that was introduced in 5.10. For
5.8 we have to flip bits on $^H which is dangerous as they're for internal use
and we might collide with someone else. See utf8.pm in 5.8 for an example.

Maybe we should just drop 5.8.

@barefootcoder
Copy link
Contributor

Just riffing here ..

What if we make our own pragma? It would be part of MS, so it's not like it would be out there in general. The pragma could use local to flip a var in the MS namespace. So you would do:

use Method::Signatures;
no compile_at_BEGIN;

or somesuch. I haven't completely thought this through, but it seems like it should be doable.

Maybe we should just drop 5.8.

As a last resort, I'd consider it. But, I have to tell you: we're using 5.8 in production right now. In fact, the MXMS problems are what are keeping us from upgrading to 5.12. Now, obviously, all the work I've been doing on MS/MSM is to fix all that and enable us to upgrade to 5.12 eventually. But I have no idea if TPTB around here will want us to do those two things simultaneously, or upgrade to MSM first, then upgrade Perl versions, to reduce the number of variables changed at once, you know. So it's possible that dropping 5.8 altogether might really screw me.

@barefootcoder
Copy link
Contributor

Okay, further thought without cold medicine kicking around my brain. :-) So obviously the problem here is how to make the local stick from the POV of the calling code. As you say, %^H would be the standard way to do it ... but that's not available in 5.8? It is listed in perlvar for my 5.8.8 installation, although admittedly it says "for internal use only."

Failing that, why not just use local? I do this for my Debuggit module, and it's not a bad interface. Something like:

use Method::Signatures;

func compile_time();
{
    local $Method::Signatures::compile_at_BEGIN = 0;
    func run_time();
}
func compile_time_again();

That's not awful, is it? And it doesn't preclude adding a no compile_at_BEGIN style pragma in the future, once 5.8 is a distant memory.

@schwern
Copy link
Contributor Author

schwern commented Sep 16, 2011

Oh, I hadn't realized %^H is in 5.8. The 5.10 docs said it was a 5.10 feature. It appears to have been introduced back in 2000. I'll see if it works.

Otherwise, local won't work because the assignment happens after BEGIN.

local $Foo = 42;
BEGIN { print $Foo }  # use of unintialized value

And you can't put the local inside a BEGIN block because then it will be local to that block.

Let's hope %^H works. It might necessitate bumping the minimum Perl up to 5.8.8 or something. I don't actually test against 5.8.1 anyway. :)

@barefootcoder
Copy link
Contributor

Otherwise, local won't work because the assignment happens after BEGIN.

D'oh! Yes, you're quite right.

Let's hope %^H works. ...

Yeah, ditto that. I'll think some more on it, but I'm coming up blank on alternatives myself ATM.

@schwern
Copy link
Contributor Author

schwern commented Sep 21, 2011

There is an additional complication (yay). Checking the value of %^H requires looking up the call stack to the call level which is using Method::Signatures. We have to do this in code_for() which is called deep inside Devel::Declare::MethodInstaller::Simple. There needs to be logic which looks up the call stack for the hint.

The question is, how high does it look? We can hard code the current height, but that will change from version to version. We can look up the call stack until we see the existence of our hint, and that seems the most robust.

Oh, the other problem is it appears %^H can't be used in 5.8. At least there's no way to retrieve it, caller() does not return the hint hash like it does in 5.10. Looking for alternatives.

@schwern
Copy link
Contributor Author

schwern commented Sep 21, 2011

A possibility is to use Devel::Pragma which seems to provide access to %^H on 5.8 via some XS voodoo and may hold a solution to the call stack problem.

@barefootcoder
Copy link
Contributor

Okay, Devel::Pragma FTW. I'll try to come up with some failing tests to exercise the pragma and we'll see how it fares in 5.8, 5.10, and 5.12.

barefootcoder added a commit that referenced this issue Sep 22, 2011
create a way to distinguish compile-time from run-time definition
check pragma's default value, turning on and off, and lexical scope
pragma's are commented out for now; just want to see if this satisfies testing for git issue #8
schwern added a commit that referenced this issue Sep 22, 2011
@schwern
Copy link
Contributor Author

schwern commented Sep 22, 2011

Ta da! Devel::Pragma solved all our problems... except one. It broke "into".

The problem is "into" deliberately activates signatures from outside the scope it's acting on. For example...

{
    package Bar;
    use Method::Signatures { into => 'Foo' };
}

{
    package Foo;

    print Foo->foo(42);

    method echo($arg) {
        return $arg;
    }
}

Not sure what to do about that. Lexical scoping rules explicitly don't apply, it's acting on a namespace.

@schwern
Copy link
Contributor Author

schwern commented Sep 22, 2011

Fixed it by making double sure compile_at_BEGIN defaults to on. The down side is you can't combine "into" and "compile_at_BEGIN" but I'm not worried about that right now. I'm pretty sick of the whole thing.

All that's left is docs.

@barefootcoder
Copy link
Contributor

Looks great! I don't mind doing the docs. I'll see if I can knock it out today.

@schwern
Copy link
Contributor Author

schwern commented Sep 23, 2011

Oh, I did it before I read that. Go ahead and review/amend the docs. If you're happy with it, say "approved" and one of us will merge the branch.

barefootcoder added a commit that referenced this issue Sep 23, 2011
this adds the Devel::BeginLift version number requested by Schwern in #22
also covers pre-5.8 versions
adds to doco for #8
barefootcoder added a commit that referenced this issue Sep 23, 2011
covers into + compile_at_BEGIN for #8
barefootcoder added a commit that referenced this issue Sep 23, 2011
barefootcoder added a commit that referenced this issue Sep 23, 2011
although now that I look at it, I think we might have a redundancy here
might need to remove either this section or the one I added earlier
@barefootcoder
Copy link
Contributor

Okay, there's all I had that you didn't cover. (Plus I added a brief section to the MSM doco, but I apparently forgot to add the issue reference.) And I think one of my bits may overlap one of yours; sorry about that. I didn't really see it until afterwards. Let me know if you think this needs further tweaking.

schwern added a commit that referenced this issue Sep 23, 2011
Nit: 5.8 isn't quite "early" as "earlier".

For #8
schwern added a commit that referenced this issue Sep 23, 2011
Rather than have the user crawl through the notes.

For #8
@schwern
Copy link
Contributor Author

schwern commented Sep 23, 2011

I merged the redundant docs and have merged the branch. Closing it up.

@schwern schwern closed this as completed Sep 23, 2011
@barefootcoder
Copy link
Contributor

Looks good.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants