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

Should @super.dec be allowed? #461

Open
JLHwung opened this issue Mar 30, 2022 · 21 comments
Open

Should @super.dec be allowed? #461

JLHwung opened this issue Mar 30, 2022 · 21 comments

Comments

@JLHwung
Copy link

JLHwung commented Mar 30, 2022

The current syntax

DecoratorMemberExpression[Yield, Await] :
  IdentifierReference[?Yield, ?Await]
  DecoratorMemberExpression[?Yield, ?Await] . IdentifierName
  DecoratorMemberExpression[?Yield, ?Await] . PrivateIdentifier
  ( Expression[+In, ?Yield, ?Await] )

disallows super.dec as a decorator member expression, instead one would have to parenthesize it into @(super.dec).

Similar to #460.

@ljharb
Copy link
Member

ljharb commented Mar 30, 2022

Also new.target, and arguments

@JLHwung
Copy link
Author

JLHwung commented Mar 30, 2022

AFAIK arguments is covered in IdentifierReference. I will open a new issue for new.target.

@Jamesernator
Copy link

Jamesernator commented Mar 31, 2022

Doesn't the evaluation of the decorator itself happen at the same time as computed fields? In which case @super.blah (or @(super.blah)) would throw a runtime error anyway?

i.e. Like this isn't allowed today with computed fields:

class AbstractFoo {
    static methodKey = Symbol();
}

class Bar extends AbstractFoo {
    // Uncaught SyntaxError: 'super' keyword unexpected here
    [super.methodKey]() {
        // ...
    }
}

@ljharb
Copy link
Member

ljharb commented Mar 31, 2022

@Jamesernator

class Base {
  foo() {}
}
class Outer extends Base {
  foo() {
    return @super.foo class Inner {}
  }
}

?

@Jamesernator
Copy link

@Jamesernator

Ah true, yeah for class level decorators probably makes sense to just allow it. And make it an early error for inside-class decorators like computed fields are.

@bakkot
Copy link

bakkot commented Mar 31, 2022

Doesn't the evaluation of the decorator itself happen at the same time as computed fields?

My understanding was that it's a different phase. There's no reason it can't work the same way it does in a static {} block, which is that it would refer to the superclass. (I haven't actually checked that it does, and I agree that if it's a runtime error there's no reason to allow it.)

Assuming it refers to the superclass,

class A {
  static capture() {}
}

class B extends A {
  @super.caputure
  foo() {}
}

is a totally reasonable thing to write.

@pzuraq
Copy link
Collaborator

pzuraq commented Mar 31, 2022

Decorator evaluations are interwoven with dynamic field evaluations, timing wise:

class C {
  [(console.log(1), 'a')];

  @(console.log(2), dec)
  [(console.log(3), 'b')];
}

// 1, 2, 3

Decorator application is in a different phase, after these expressions are evaluated. It would be odd to me that dynamic properties would be interwoven with decorator expressions and have a different meaning for super.

@bakkot
Copy link

bakkot commented Mar 31, 2022

Ehhh I don't think the timing concern is particularly important. For most readers the fact that the timing is interwoven is obscure trivia; the visible fact is that the syntax is interwoven, and we've already bitten that bullet with static fields and blocks having a different super than computed properties do.

@nicolo-ribaudo
Copy link
Member

nicolo-ribaudo commented Mar 31, 2022

Even with class elements decorators, this code is already valid and has well-defined semantics:

class A {
  decorator() { console.log("Do something"); }
}

class B extends A {
  method() {
    class C {
      @(super.decorator) foo() {};
    }
  }
}

I think this issue should only focus on the syntax aspect, i.e. if also this is valid (and if it is, it must have the same semantics of the other already valid code):

class A {
  decorator() { console.log("Do something"); }
}

class B extends A {
  method() {
    class C {
      @super.decorator foo() {};
    }
  }
}

@JLHwung
Copy link
Author

JLHwung commented Mar 31, 2022

A related question: should we allow @super()?

class C extends class B {
  constructor() { return () => () => "hello" }
}
{
  constructor() {
    return class D {
      @super() p;
    }
  }
}

@bathos
Copy link

bathos commented Jul 16, 2022

A related question: should we allow @super()?

Probably not unless there’s a definite use case? It could be done with parentheses regardless and if really doing this, parentheses would probably make it more “readable” anyway (relatively i mean. the readability bar to clear is pretty low there lol).

Since SuperCall is its own thing with unique semantics and doesn’t “come along for the ride” with SuperProperty, it doesn’t (afaict) make things simpler to permit it either — is there an advantage to permitting it I’m not catching?

@ljharb
Copy link
Member

ljharb commented Jul 16, 2022

super isn’t it’s own expression thing, so i don’t think it would work at all if not allowed in this way.

@bathos
Copy link

bathos commented Jul 16, 2022

@ljharb If I understood it correctly, @JLHwung’s example intended the parentheses to be those of SuperCall, not those of the existing decorator grammar — i.e. @@(super()) with its outer parens being optional, not the non-existent @@(super)(). The question may have stemmed from the fact that SuperCall is potentially valid in computed property names:

new class extends Map {
  constructor() {
    console.assert("[object Map]" in class { static [super()]; });
  }
}

I think the answer to the question oughta be “no” despite @@(super()) being potentially valid since I see no reason to explicitly bless that with “no parentheses needed” in the decorator grammar.

@ljharb
Copy link
Member

ljharb commented Jul 16, 2022

@bathos i read @super() p; as invoking a decorator with zero arguments - ie, super IS a decorator - as opposed to invoking a decorator returned from super().

@JLHwung
Copy link
Author

JLHwung commented Jul 18, 2022

Per DecoratorEvaluation spec:

@(super()) p means invoking a decorator on p, the decorator is the result evaluated from (super()), where (super()) is covered by DecoratorParenthesizedExpression.

@call() p means invoking a decorator on p, the decorator is the result evaluated from call(), where call() is covered by DecoratorCallExpression.

The question "should we allow @super()?" is equivalent with "should DecoratorCallExpression also cover SuperCall?" And if it should, it must share the same semantics with the already valid @(super()).

Expanding DecoratorCallExpression benefits JS minimizer, those have readability concerns can always disable such patterns via linter rules.

@bathos
Copy link

bathos commented Jul 18, 2022

Expanding DecoratorCallExpression benefits JS minimizer, those have readability concerns can always disable such patterns via linter rules.

If a minifier can remove two bytes from an expression that nobody’s writing in a rainforest, does it still burn? :)

@JLHwung
Copy link
Author

JLHwung commented Jul 18, 2022

an expression that nobody’s writing in a rainforest

"Nobody is writing @(super())" does not imply that "Expanding DecoratorCallExpression to cover SuperCall does not benefit JS minimizer", because a SuperCall at that position can be an intermediate result of a minimizer.

For example, the code

function id(x) {
  return x;
}
class DecoratorFactoryGen {
  construtor() {}
}
class C extends DecoratorFactoryGen {
  constructor() {
    const b = super();
    return id(b);
  }
}
new C();

will be minified by Terser as

new class extends class{construtor(){}}{constructor(){return super()}};

where b = super() is inlined within id(b). Now think of @id(b) foo, the minifier can then generate @super() foo if it were allowed.

@bathos
Copy link

bathos commented Jul 18, 2022

Is the possibility of it being intermediate substantially higher than the possibility of it being written directly? Wouldn’t you still need to be authoring decorators as classes which use return override? I’m an unapologetic return override lover myself (🤫) but am having a hard time picturing scenarios where it would be useful for that.

In any case I don’t think this is a consideration which should inform language design. A language is created to benefit people, not minifiers, and half the time brotli alone produces comparable savings.

@clshortfuse
Copy link

clshortfuse commented Oct 27, 2022

Right now my syntax is:

class Button extends CustomElement {
 // 
}

// Uses CustomElement.idl(), but `Button` instead for `this`
// Assignment just to appease Typescript
Button.prototype.disabled = Button.idl('disabled', {type: 'boolean'});

From my understanding decorators will pass this, so it could be rewritten as

class Button extends CustomElement {
  @super.idl
  disabled;
}

Side note: I'm not sure where to put the options though because I also have some things like:

Input.prototype.formEnctype = Input.idl('formEnctype', { attr: 'formenctype', ...DOMString });

I think that could be:

@super.idl formEncType = super.idlOptions({ attr: 'formenctype', ...DOMString });

@trusktr
Copy link
Contributor

trusktr commented Nov 12, 2022

Based on this proposal, #465, @super.dec could totally work, and would make more sense if #465 were true because then property access would have meaning.

@JLHwung
Copy link
Author

JLHwung commented Nov 13, 2022

@trusktr This is a syntax-only issue as @(super.dec) is already valid per current spec, see also #461 (comment).

// They are equivalent.
@(foo.bar)
@foo.bar
@(super.foo) // Already valid
@super.foo // Should we allow this form? If so they should be equivalent

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

No branches or pull requests

9 participants