Third opinion about attaching JSDoc

[John Lenz]

Nick -

Dimitris has been working on removing the hacky support for JSDoc from the Rhino parser with something much more robust.  This work is a great improvement, but we have come to a bit of an impasse and are hoping you can break the tie.

Traditionally, we have always said that JSDoc "cast" were formed by a @type express attached to a parenthese expression:

   /** @type {type} */( expr )

So far so good.  Dimitris and I have agreed that a CAST is a @type expression attach to a parenthese expression.

Were we have a disagreement is the rule for attaching the JSDoc to the parenthese.  From the parsers point of view it is consistent to always attach the JSDoc using simple precedence rules:

  /** attach */ (x).prop    -->   attaches to GETPROP not the LP (which we would make a CAST)

  /** attach */ x.prop      -->   attaches to the GETPROP not the NAME

  /** attach */ x + y         -->   attaches to the NAME X not the operator

For the first case, would need an extra set of parenthese to create the cast:

  

  (/** @type {foo} */ (x)).prop 

I would prefer that we treat the cast as an operator:

   /** @type {foo} */(x).prop

To do this properly without changing the parser to recognize @type JSDoc is a bit tricky we don't really want to attach other JSDoc to the parenthese.  But we can do it in the IRFactory when we convert to our internal AST, without changing how the attach JSDOC in general:

When we see a node with a JSDoc attached, and it is a @type expression we look down the lhs of the tree for a parenthesized expression and if found insert a CAST node and attach it the JSDoc there.

I prefer this "extra work" as I think of the @type JSDoc and the parenthese as a single entity that we are trying to reconstructed while Dimitris finds the special handling odd and confusing.

Thoughts?

[Nick Santos]

I do find it a bit strange that
/** attach */ (x).prop
attaches to the GETPROP instead of the NAME. Is that a product
requirement? Or is that required by the parser architecture? or
something else? I feel like we've talked about this before but I
forgot what the reason was.

More generally, I've always found it strange that the JSDoc attaches
to a different node based on its contents. maybe that's the underlying
tension here?

[Johannes Nel]

Hi

(/** @type {foo} */ (x)).prop  is non intuitive given the fact that x is wrapped for the cast in parentheses. 

it seems almost java like in the approach, logically similar to ((CAST)x).prop. given the structuring, if votes here counted at all, I would say attach the cast to the name.

--
 
---
You received this message because you are subscribed to the Google Groups "Closure Compiler Discuss" group.
To unsubscribe from this group and stop receiving emails from it, send an email to closure-compiler-d...@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.
 
 

--
j:pn
\\no comment

[Dimitris Vardoulakis]

@Nick: What is happening currently that is strange from a parser's perspective is that the operator precedence is mixed. Between two operators op1 and op2, sometimes op1 wins and sometimes op2. Normally there is a definite order, eg, op1 always binds tighter than op2.

Currently, 

/** asdf */ a.b  => the comment attaches to getprop (eg, stub declarations)

/** asdf */ (a).b  => the comment attaches to (a).

/** asdf */ a = b  => the comment attaches to the assignment.

/** asdf */ (a) = b  => the comment attaches to (a).

What I want is a clear order of operator precedence, like with all other operators in JS, and like with casts in other languages. So, either getprop binds tighter than jsdoc, or the other way around, but the precedence doesn't depend on peeking at the lhs of the getprop. 

From the two options for definite precedence, I prefer that dot binds tighter than jsdoc, so then to cast the lhs of a getprop, you write:

(/** asdf */ (a)).b  => the comment attaches to (a).

I think of the jsdoc by itself as having precedence rules, and John thinks of the cast operator as being /** @type {asdf} */ ( ), the type-jsdoc coupled with the parens.

The drawback of my approach is that it's not backwards compatible so it requires fixes of the code that uses casts. Eg, if we decide that dot binds tighter than jsdoc, code that looks like this

/** asdf */ (a).b

must change to this

(/** asdf */ (a)).b

If we decide that jsdoc binds tighter than the dot, this code

/** asdf */ a.b;

must change to this

/** asdf */ (a.b);

But I think clear precedence is worth the code fixing. (We also decided to special-case and allow mixed precedence in a couple places for now as a workaround, but I won't go into the details of that unless it becomes necessary.)

@Johannes: Before the change that I'm currently working on, jsdoc attachment was hacky and that's why we introduced the parens around x in your example; they helped a lot in deciding where to attach jsdocs. With the approach currently in progress, the parens around x aren't strictly necessary, but we're keeping them because of the vast amount of fixes we would have to do in google code otherwise.

--
Dimitris

[Ben Lickly]

Is there any reason why we can't support

(/** attach */ x).prop

?

This seems completely unambiguous to the parser, yet still less gross than:

(/** attach */ (x)).prop

[Ben Lickly]

P.S. I'm not suggesting that we remove support for 

/** attach */ (x)

I'm just thinking that if those parens were optional, it might make the other parens more bearable.

[John Lenz]

Ben-

The trouble people ran into is that they were thinking of "(/** @type {Type} */ expr)" as the operator so ran into trouble with expressions like "(/** @type {Type} */ x + y)" (where the @type is attached to "x" instead of "x +y" and we had no way of saying "this @type is misplaced" because we have to allow it everywhere.  With /** @type {} */() the @type is attached to the parenthese and we can disallow inappropriate uses.

On Tue, May 21, 2013 at 1:22 PM, Ben Lickly <bli...@google.com> wrote:

[Ilia Mirkin]

On Tue, May 21, 2013 at 4:33 PM, John Lenz <conca...@gmail.com> wrote:
> Ben-
>
> The trouble people ran into is that they were thinking of "(/** @type {Type}
> */ expr)" as the operator so ran into trouble with expressions like "(/**
> @type {Type} */ x + y)" (where the @type is attached to "x" instead of "x
> +y" and we had no way of saying "this @type is misplaced" because we have to
> allow it everywhere. With /** @type {} */() the @type is attached to the
> parenthese and we can disallow inappropriate uses.

How about adding (/**@type {}*/ x) to the list of allowed things,
while leaving (/**@type {}*/x+y) disallowed. Or is that too
difficult/annoying to implement?

-ilia

[Dimitris Vardoulakis]

@john: Will it be hard for people to just think of /** @type {Type} */ as the cast, and the parens are needed only when sm wants to change the default precedence?

As Ben said, we can still support

/** @type {Type} */ (x)

so, we won't have to go and remove parens everywhere. The upside is that I can do my proposed precedence fixes without increasing the number of parens.

Dimitris

[Erik Neumann]

I vote for "clear order of operator precedence".

I think of casts as a kind of cludge in general, so some syntactic oddness is appropriate.  If you don't like a statement like:

    (/** attach */ (x)).prop

then you could instead do:

    var foo = /** attach */(x);

    foo.prop

It would be AWESOME to be able to use JSDoc on my google closure code base!

--ErikN

On Tue, May 21, 2013 at 1:22 PM, Ben Lickly <bli...@google.com> wrote:

[Nick Santos]

@ilia: we used to do that. It was unmaintainable, both technically and
socially. We saw repeated cases where users would get into bikeshed
arguments about whether the parens went on the inside or the outside.
Then they'd get it wrong and complain that the cast didn't work. There
are a few examples in the issue tracker of this happening.

[Nick Santos]

@dimitris: could we just treat "/** jsdoc */" with different
precedence rules than "/** jsdoc */ ("? like by parsing them as
different tokens/operators?

I understand your objection w/r/t peeking ahead, but i feel like
there's already some precedence (zing!) for this.

[Dimitris Vardoulakis]

On Tue, May 21, 2013 at 1:58 PM, Nick Santos <nicholas...@gmail.com> wrote:

@dimitris: could we just treat "/** jsdoc */" with different
precedence rules than "/** jsdoc */ ("? like by parsing them as
different tokens/operators?

It's not my preference, but it may be the path of least resistance. As far as I can tell, this will not require changes in google code.

 

I understand your objection w/r/t peeking ahead, but i feel like
there's already some precedence (zing!) for this.

On Tue, May 21, 2013 at 4:44 PM, Nick Santos
<nicholas...@gmail.com> wrote:
> @ilia: we used to do that. It was unmaintainable, both technically and
> socially. We saw repeated cases where users would get into bikeshed
> arguments about whether the parens went on the inside or the outside.
> Then they'd get it wrong and complain that the cast didn't work. There
> are a few examples in the issue tracker of this happening.
>
> On Tue, May 21, 2013 at 4:38 PM, Ilia Mirkin <imi...@alum.mit.edu> wrote:
>> On Tue, May 21, 2013 at 4:33 PM, John Lenz <conca...@gmail.com> wrote:
>>> Ben-
>>>
>>> The trouble people ran into is that they were thinking of "(/** @type {Type}
>>> */ expr)" as the operator so ran into trouble with expressions like "(/**
>>> @type {Type} */ x + y)" (where the @type is attached to "x" instead of "x
>>> +y" and we had no way of saying "this @type is misplaced" because we have to
>>> allow it everywhere.  With /** @type {} */() the @type is attached to the
>>> parenthese and we can disallow inappropriate uses.
>>
>> How about adding (/**@type {}*/ x) to the list of allowed things,
>> while leaving (/**@type {}*/x+y) disallowed. Or is that too
>> difficult/annoying to implement?
>>
>>   -ilia

--

---
You received this message because you are subscribed to the Google Groups "Closure Compiler Discuss" group.
To unsubscribe from this group and stop receiving emails from it, send an email to closure-compiler-d...@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.

--
Dimitris