Ticket #389 (closed deprecation: fixed)

Opened 6 years ago

Last modified 5 years ago

subs with :method should not be entered as symbols in the namespace

Reported by: pmichaud Owned by: allison
Priority: critical Milestone:
Component: core Version: trunk
Severity: high Keywords:
Cc: Language: perl6
Patch status: new Platform:

Description

At the OSCON 2008 hackathon, and again at PDS, we had decided that using any of :anon, :method, or :vtable would suppress a sub from being entered as a symbol in the namespace. PDD 19 also indicates that :method implies the sub is not stored in the namespace (line 501).

To have a sub be both a method and a symbol in the namespace, one would use the :nsentry flag.

I thought this had already been completed, but I tried it just now and it doesn't work (r37066, Kubuntu 8.04 i386) -- subs with :method still appear as symbols in the namespace:

$ cat x.pir
.sub 'main' :main
    $P0 = newclass 'XYZ'
    $P1 = new 'XYZ'

    $P1.'xyz'()

    $P2 = get_hll_global ['XYZ'], 'xyz'
    $P2($P1)
.end

.namespace ['XYZ']
.sub 'xyz' :method
    say 'XYZ::xyz'
.end

$ ./parrot x.pir
XYZ::xyz
XYZ::xyz
$

A related item, equally problematic, is that placing an :anon flag on a :method sub causes it to no longer be a valid method:

$ cat y.pir
.sub 'main' :main
    $P0 = newclass 'XYZ'
    $P1 = new 'XYZ'

    $P1.'xyz'()

    $P2 = get_hll_global ['XYZ'], 'xyz'
    $P2($P1)
.end

.namespace ['XYZ']
.sub 'xyz' :method :anon
    say 'XYZ::xyz'
.end

$ ./parrot y.pir
Method 'xyz' not found for invocant of class 'XYZ'
current instr.: 'main' pc 11 (y.pir:5)
$

This is currently a blocker of sorts for processing exported subs and methods in Rakudo. I may be able to work around it in PCT, but it would be far better to have this fixed prior to 1.0 release. Indeed, I'm wondering if it got overlooked on our roadmap somehow, because it's definitely an important feature for 1.0 .

Thanks,

Pm

Attachments

389.patch Download (1.6 KB) - added by bacek 6 years ago.
Proposed fix for ticket.
nsentry.patch Download (7.5 KB) - added by bacek 5 years ago.
Salvaged stuff from nsentry branch
no_methods_in_namespace.patch Download (3.0 KB) - added by allison 5 years ago.
patch to prevent methods and vtable functions from being inserted into the namespace
no_methods_in_namespace.2.patch Download (2.3 KB) - added by allison 5 years ago.

Change History

  Changed 6 years ago by allison

  • priority changed from blocker to normal
  • severity changed from high to medium
  • milestone changed from 1.0 to 1.1

We didn't overlook it in the roadmap, we decided namespaces needed a more extensive review, and scheduled that review for 1.1. There are a number of lingering questions, and every time we talk about it we come away with a different answer. (Like, if :method implies :anon, why should :anon even be allowed with :method?)

Moving this ticket to the 1.1 milestone.

  Changed 6 years ago by pmichaud

This doesn't match my recollection of things -- as far as I can remember, our interpretation of these flags has been fairly consistent since OSCON 2008.

At any rate, can we get an enumeration of the lingering questions on this issue? As I noted in the original ticket, this is a bit of a blocker for Rakudo at the moment.

For the posed question "If :method implies :anon, why allow :anon with :method?" -- the answer to that isn't really significant for the problem I'm having. My problem is simply that at present there's not a way to create a compile-time method in PIR that isn't also entered in a namespace. That said, from a code generator point of view, it would bug me if ":method :anon" threw an exception simply because the flags seemed redundant.

I'm not claiming this issue has to be resolved prior to 1.0; but it is a long-standing item that is rapidly becoming a blocker for Rakudo (as we try to create Perl 6 builtins in Perl 6).

Pm

  Changed 6 years ago by pmichaud

  • summary changed from :method should imply :anon, :anon should work with :method to subs with :method should not be entered as symbols in the namespace

Since posting my latest comment on this ticket, I've come across a case where the :anon flag is needed with :method. In particular, Rakudo and PGE sometimes need to create "anonymous methods" -- i.e., Parrot subs that act like methods (i.e., they have a 'self') but that don't pose role composition conflicts just because two separate compilations happen to end up with a name collision. (Enforcing name uniqueness across compilations is quite tricky.)

It turns out the :anon flag is doing _exactly_ what we need it to do in this case, so I'd like it to remain the same.

So, the only issue remaining in this ticket is that subs with a :method flag should not be automatically entered as symbols in the namespace. This is consistent with what was discussed at OSCON 2009, PDS, and with what is currently described in PDD 19. (I'm also changing the ticket subject to reflect this.)

Thanks!

Pm

  Changed 6 years ago by allison

  • owner set to allison

  Changed 6 years ago by pmichaud

  • lang set to perl6
  • priority changed from normal to major

Changed 6 years ago by bacek

Proposed fix for ticket.

follow-up: ↓ 7   Changed 6 years ago by bacek

  • patch set to new

I've attached proposed fix for this ticket. It's kinda cheating. I add all subs into namespaces and filter them out in Parrot_find_global. This allows Class.pmc to find them out, but get_*_global ops will not.

There is result of running initial x.pir and y.pir.

bacek@icering:~/src/parrot$ ./parrot x.pir 
XYZ::xyz
Null PMC access in invoke()
current instr.: 'main' pc 23 (x.pir:8)
bacek@icering:~/src/parrot$ ./parrot y.pir 
XYZ::xyz
Null PMC access in invoke()
current instr.: 'main' pc 23 (y.pir:8)

Unfortunately, many other parrot's parts are broken. For example PGE can't be compile with next message:

../../parrot ../../runtime/parrot/library/PGE/Perl6Grammar.pir  --output=PGE/builtins_gen.pir PGE/builtins.pg
Null PMC access in invoke()
current instr.: 'parrot;PGE;Perl6Grammar;Compiler;compile' pc 212 (../../runtime/parrot/library/PGE/Perl6Grammar.pir:162)
called from Sub 'parrot;PCT;HLLCompiler;eval' pc 919 (src/PCT/HLLCompiler.pir:522)
called from Sub 'parrot;PCT;HLLCompiler;evalfiles' pc 1281 (src/PCT/HLLCompiler.pir:694)
called from Sub 'parrot;PCT;HLLCompiler;command_line' pc 1489 (src/PCT/HLLCompiler.pir:798)
called from Sub 'parrot;PGE;Perl6Grammar;Compiler;main' pc 18 (../../runtime/parrot/library/PGE/Perl6Grammar.pir:69)

So, on positive note - it can be done. On negative - it will require a lot of cleanups and probably deprecation cycle.

-- Bacek

in reply to: ↑ 6   Changed 6 years ago by pmichaud

Replying to bacek:

I've attached proposed fix for this ticket. It's kinda cheating.

The proposed patch to me looks Very Wrong. Not all methods will be anonymous in the namespace -- so using the SUB_FLAG_PF_ANON flag to filter out methods is probably not a good approach.

In particular, the following method declarations should still appear as entries in the namespace:

    .sub 'xyz' :method :nsentry
        ...
    .end

    .sub '' :method('abc') :nsentry('def')
        ...
    .end

Also, the patch appears to cause the following to fail:

    .sub 'main' :main
        #  get 'abc' sub and store in namespace as 'def'
        .const 'Sub' $P0 = 'abc'
        set_global 'def', $P0

        $P1 = get_global 'def'     # should be valid lookup
        $P1()
    .end
    
    .sub 'abc' :anon
        say 'Hello from abc masquerading as def'
    .end

Pm

  Changed 5 years ago by whiteknight

  • owner changed from allison to pmichaud
  • milestone changed from 1.3 to 1.4

  Changed 5 years ago by allison

  • status changed from new to assigned
  • owner changed from pmichaud to allison

  Changed 5 years ago by pmichaud

  • priority changed from major to critical
  • severity changed from medium to high

  Changed 5 years ago by coke

  • milestone 1.4 deleted

  Changed 5 years ago by pmichaud

The original OSCON 2008 discussion of :nsentry (and the pieces that have been implemented thus far) were described in RT #53302. Thanks to Coke++ for locating this ticket.

Pm

Changed 5 years ago by bacek

Salvaged stuff from nsentry branch

  Changed 5 years ago by bacek

Hello.

I salvaged as much as possible from nsentry branch. Branch can be deleted now.

There is some bits related to rakudo which left. I'll prepare patch for rakudo soonish.

-- Bacek

  Changed 5 years ago by coke

This branch fails for partcl because .subs declared with :method aren't invokable as methods on the object.

$ ./tclsh
Method 'getListValue' not found for invocant of class 'TclString'
$ ack :method src/class/tclstring.pir
.sub getListValue :method
.sub getDictValue :method

I pretty much universally use this method as:

 foo = foo.'getListValue'() # insure arbitrary PMC can be used as a list.

I'm not (intentionally) relying on the namespace here at all. (if I add :nsentry here, this error goes away. However, I cannot make it go away for the method I'm attempting to add to the core String that I'm using.) However, I read this right, I shouldn't have to be adding nsentry, since I'm not expecting it to be in the namespace anyway.

I would also argue that the proper way to do introduce this that allows people to adopt it painlessly would be: * provide a way to override the default with the desired new behavior. (e.g. :no_nsentry) * provide a way to explicitly declare you want the default behavior (e.g. :nsentry) * With the next release, declare that the old default will change after the release after that.

This allows users to update their code in advance and have it keep working when the default changes; it also allows users to adopt the new behavior asap.

  Changed 5 years ago by bacek@…

Parrot wrote:
> Comment(by coke):
> 
>  This branch fails for partcl because {{{.sub}}}s declared with :method
>  aren't invokable as methods on the object.

Oookey. It's looks like a real bug.

  >  I'm not (intentionally) relying on the namespace here at all. (if I add
>  :nsentry here, this error goes away. However, I cannot make it go away for
>  the method I'm attempting to add to the core String that I'm using.)
>  However, I read this right, I shouldn't have to be adding nsentry, since
>  I'm not expecting it to be in the namespace anyway.

You definitely shouldn't. It's some shenanigans in 
Class/Object/Namespace methods handling. I'll try to investigate what's 
going on.

-- 
Bacek

  Changed 5 years ago by pmichaud@…

On Wed, Sep 23, 2009 at 02:00:15PM -0000, Parrot wrote:
> #389: subs with :method should not be entered as symbols in the namespace
>  I would also argue that the proper way to do introduce this that allows
>  people to adopt it painlessly would be:
>  * provide a way to override the default with the desired new behavior.
>  (e.g. :no_nsentry)
>  * provide a way to explicitly declare you want the default behavior (e.g.
>  :nsentry)
>  * With the next release, declare that the old default will change after
>  the release after that.
> 
>  This allows users to update their code in advance and have it keep working
>  when the default changes; it also allows users to adopt the new behavior
>  asap.

Note that the existing (incorrect) default has already been listed as
deprecated in DEPRECATED.pod, so _technically_ we can make the change
anytime we wish.

That said, I'm fine with the interim approach given above -- if a :no_nsentry
flag is added, then PCT can immediately start defaulting generation of methods
to automatically include that flag (unless 'nsentry' is provided in the PAST
structure).

Thanks!

Pm

  Changed 5 years ago by allison

Sorry folks, but this is the wrong fix. It sets the sub's ns_entry_name to an empty string, then calls 'Parrot_store_sub_in_namespace'. But, 'Parrot_store_sub_in_namespace' is also what stores methods on the class. (It extracts sub->ns_entry_name, then calls 'Parrot_store_global_n' which calls 'VTABLE_set_pmc_keyed_str' on the namespace object. Inside 'set_pmc_keyed_str' in NameSpace is the logic that decides whether to store a sub in the namespace, or in the cache of methods/vtable overrides associated with the namespace, or directly in the class if there is one associated with the namespace.)

The right fix is to change 'set_pmc_keyed_str' in NameSpace so that it stores methods () by calling the 'add_to_class' function, and skipping the code that stores the method in the namespace. Method objects are identifiable because they are sub objects marked with the SUB_COMP_FLAG_METHOD flag in 'sub->comp_flags'. The 'add_to_class' function calls 'add_method' on the class object when there is one, and stores the method in a special hash for methods inside the namespace object when the class object doesn't exist yet. With the current code in 'set_pmc_keyed_str' in NameSpace, it's very difficult to store the sub in the class and skip storing it in the namespace. The code badly needs a cleanup and refactor into subroutines.

Throw away the nsentry2 branch and start over (it's a pretty lightweight branch anyway, mostly test changes adding :nsentry where it shouldn't be needed).

Please write a set of tests to go with any fixes for this feature that make sure:

  • :method alone stores the sub in the class but not in the namespace (this means the sub object can be called as a method but not as a sub)
  • :method with :nsentry stores the sub in the class and in the namespace (this means the sub object can be called as either a method or a sub)
  • :vtable alone stores the sub in the class but not in the namespace
  • :vtable with :nsentry stores the sub in the class and in the namespace
  • that both options work with :multi
  • that regular subs are still stored appropriately in the namespace (and can be called as a sub, but not as a method or vtable override)

  Changed 5 years ago by allison

When implementing this, also test that :vtable is properly stored in the class when combined with :anon. See RT #44471.

  Changed 5 years ago by coke

  • type changed from bug to deprecated

Changed 5 years ago by allison

patch to prevent methods and vtable functions from being inserted into the namespace

follow-up: ↓ 22   Changed 5 years ago by allison

I've attached a patch that implements this fix. I'll apply it after the 2.3 release. Even though this feature has been deprecated a long time (over a year), some external libraries still depend on the old feature and I don't want to disrupt them less than a week before a supported release (especially not NQP-rx).

This patch includes a fix to a generated file in NQP-rx (so people can apply it and test it now). NQP-rx itself should be updated to generate the file correctly, so that part of the patch is not necessary.

This patch requires rebuilding imcc's lex/yacc files (configuring with --maintainer), because it adds code to set an NSENTRY flag in IMCC when a sub is marked with :nsentry.

Changed 5 years ago by allison

  Changed 5 years ago by allison

I went ahead and committed the IMCC changes, since they don't alter any behavior in existing code (only set a flag that isn't checked anywhere yet). I've attached a revised patch dropping those two lines of changes.

It's no longer necessary to rebuild imcc's lex/yacc files to test the patch.

in reply to: ↑ 20   Changed 5 years ago by pmichaud

Replying to allison:

This patch includes a fix to a generated file in NQP-rx (so people can apply it and test it now). NQP-rx itself should be updated to generate the file correctly, so that part of the patch is not necessary.

NQP-rx has now been updated to no longer need the :nsentry flag on quotemod_check, and I've updated the generated files in parrot trunk to match.

Pm

  Changed 5 years ago by allison

  • status changed from assigned to closed
  • resolution set to fixed

Final patch applied in r45827. This issue is now resolved (and tested). Closing ticket.

Note: See TracTickets for help on using tickets.