Fix super-related tagging of Code nodes

[marchaefner]

• super compiles now in static methods with reserved names (Compiler error when calling super from a method named with a reserved word #2949)
• @fn: -> super and @fn = -> super produce the same compilation (super compiles differently depending on how class method is defined #3232)
• super will no longer compile for ambiguous / wrong super-targets (see Implications below)

Theory of Operation / Changes

super compiles only within a Code node that either

• is marked as a constructor (by Class)
• has both .name and .klass set
• has no own .name but is contained by another Code with both properties

If the Code node also has a truthy .static the super call will be compiled accordingly.

.static is set by Class for object assigns with a @. Changed: 97d9a63 looks also at non-object assignments. This fixes #3232.

.klass is set by

• Class to all its functions (on the class-level, not nested functions)
• Assign to prototype functions, i.e. if the LHS begins with the form {CLASS_NAME}.prototype.{IDENTIFIER|INDEX}. This has been changed to match only this exact form. (see also below)

.name is set by Assign which inspects its LHS with the help of the METHOD_DEF regexp. (Except constructors, they get their name from Class.ensureConstructor.)

Changed: 138c25f restricts METHOD_DEF to match only

• assignments to a prototype property, e.g. C::f = ->
  But no longer to properties of objects on prototypes, e.g. C::f.g = ->
• assignments to a property of a possible class, e.g. C.static = ->
  But no longer to anything else that ends with an IDENTIFIER, e.g. C.a.b = ->

Both cases are now detected in the same way. This fixes #2949.

Implications

The following examples do not work anymore and will throw a 'cannot call super ...' error. None of those look correct or useful to me.

    class Foo 
      bar = -> super
      # bar = function() {
      #   return Foo.__super__.bar.apply(this, arguments);
      # };

      bar.baz.qux = -> super
      # bar.baz.qux = function() {
      #   return Foo.__super__.qux.apply(this, arguments);
      # };

      # This (regrettably) still works
      bar.baz = -> super

    Foo::bar.baz = ->
    # Foo.prototype.bar.baz = function() {
    #   return Foo.__super__.bar.apply(this, arguments);
    # };  

The following examples will no longer throw 'cannot call super ...' but compile. (This is related to #1606 -- but by no means complete or usable).

    class Foo
      @bar: ->
        baz = -> super
        # return baz = function() {
        #   return Foo.__super__.constructor.bar.apply(this, arguments);
        # };

    Foo::bar = ->
      baz = -> super
      # return baz = function() {
      #   return Foo.__super__.bar.apply(this, arguments);
      # };

[vendethiel]

could we refactor this, maybe ?

Good work otherwise.

[marchaefner]

Certainly, but I'm not quite sure what you have in mind exactly.

In the long run I would like to replace the METHOD_DEF logic by a function that operates on nodes, instead of regexp-matching a temporary compilation. That would then be utilized here as well. But this PR is just meant as a band-aid to get those issues fixed before 1.7

Or did you just look for something like

    else if node instanceof Assign and
            node.variable.base.value in ['this', name] and
            prop = node.variable.properties[0] and
            name = prop.name and
            name.value isnt 'prototype'

[vendethiel]

no, I meant something like node instanceof Assign and node.isStatic() indeed.

[hpaulj]

Looks like static is still being set in 2 different places, here for the @= version, and addParameters for the @: version.

In #3232 (comment) I put a testStatic method that handles both @foo: and @foo=.

As best I can tell addProperties does not need to set context; setContext takes care of that.

if func.bound
   func.context = name`

That testStatic works at this setContext location with:

      else
        Class::testStatic node # try alt location

I haven't identified the comparative advantages of this testing in setContext versus a bit later in walkBody.

[marchaefner]

@hpaulj:

Looks like static is still being set in 2 different places

I think this duplication is preferable to iterating twice over object properties (in different places), as in your solution.

I haven't identified the comparative advantages of this testing in setContext versus a bit later in walkBody.

In walkBody we don't have to check for this and the class name (an edge case that's very dear to me ;) And, well, it doesn't actually 'setContext'.

As best I can tell addProperties does not need to set context; setContext takes care of that.

Very nice find. And a brainfart on my side: The current solution also tags this: -> and properties with the same name as the class as static.

Ammended the commit -- looks even better now. (fuck yeah code reviews :)

[jashkenas]

Fantastic deductive work. Do you think this is ready to merge ... or would you like to keep going on it a bit more?

[marchaefner]

@jashkenas: Maybe we could let it sit for a day or so, to give everyone (e.g. @Nami-Doc) a chance to reply / voice concerns.

[jashkenas]

Maybe we could let it sit for a day or so, to give everyone (e.g. @Nami-Doc) a chance to reply / voice concerns.

Sounds good. I agree completely about the METHOD_DEF bit — I wonder how that ever got added in the first place ;)