Object comments

[bezoerb]

This fix prevents comments inside objects from being dropped

[Constellation]

extra.comment check is necessary. Replacing expr.leadingComments || expr.trailingComments with extra.comment

[Constellation]

I've reviewed.

[Constellation]

I think 2 options are needed, commentStatement and commentExpression. I'll add them later.

[bezoerb]

Thanks

[bezoerb]

or should i fix these things and make another PR?

[getify]

Does it make sense to fix such things now, or to defer until the concrete syntax tree stuff works out?

[Constellation]

@bezoerb:

or should i fix these things and make another PR?

Could you fix the reviewed point and rebase this PR?

@getify:

As discussed at #133 (comment), I think escodegen need to provide good default code generation for AST (not for CST). So this PR makes sense. It provides good default comment code generation.

After CST works out, escodegen will also provide AST to CST functionality with these good default format. ;)

[Constellation]

And escodegen will generate CST internally.

[getify]

To be honest, that approach doesn't make much sense to me, in several respects.

I plan to create my own CST and shove it into escodegen to output code for me. It's a negative, IMO, if there are multiple different ways to specify how whitespace/comments can be outputted. Are you going to add this stuff, then turn around and deprecate/remove it later, or will they both co-exist?

It's an expressed goal of CST that it holds all the standardized way of representing all whitespace and comments (and any other "extras"). This means parsers need to see that standard CST format, and it also means that code generators need so use that same standard CST format.

The ad hoc tracking of whitespace and comments in both parsers and code generators as it stands today should, IMO, be replaced by a CST, not sitting along side it.

So, if we're heading soon towards a CST (which I really hope we are), I'm not sure why work should continue on things that are clearly not informed by, nor likely compatible with, where CST is heading in the short-term.

Again, just my 2 cents.

[Constellation]

Ah, maybe there's misleading points in my comments.

not sitting along side it.

Yes, not ssitting along side it. Will be reconstructed on the top of CST.

To show my design of js tools with CST clearly, I'll create and attach figures about tool stacks later.(after I have dinner) Sorry for your inconvenience.

[Constellation]

I've created figures.

Currently JS modules infrastructure is shown as the above.
AST is more useful than CST in analysis areas such as escope and esmangle. These modules only consider source code's semantics, not textual difference. So AST catches only semantic elements and it is suitable for semantic analysis.

However, AST has a problem, it doesn't have any textual information such as whitespaces.
So if you'd like to control textual information, it's not good for this use.

Now I have the new JS modules design, the following figure describe it.

In this newly reconstructed modules, I'm planning to implement AST to CST and CST Codegen as Escodegen.

This PR adds good default comment handling part to AST to CST in the above structure. This module is based on CST Codegen. So this PR makes sense in the redesigned architectures.

OK, so I'll answer your questions.

I plan to create my own CST and shove it into escodegen to output code for me.

If you would like to pass your own CST to escodegen, you can use CST Codegen entry point. It will works perfectly as you think.

It's a negative, IMO, if there are multiple different ways to specify how whitespace/comments can be outputted. Are you going to add this stuff, then turn around and deprecate/remove it later, or will they both co-exist?

These options work in AST to CST modules. So these options only work for AST. AST to CST module will create CST with specified whitespaces, comment etc. And after that, CST Codegen module will generate text from standardized CST generated by AST to CST. So there's only one codegen module, CST Codegen. Since they(AST to CST and CST Codegen) have different roles, they both co-exits and they don't conflict with each other.

The ad hoc tracking of whitespace and comments in both parsers and code generators as it stands today should, IMO, be replaced by a CST, not sitting along side it.

Actual code generation part will be implemented as CST Codegen. AST to CST and CST Codegen have different goals and AST to CST depends on CST Codegen, not sitting along side it.

Since I think the JS tools design with CST will become as the above, so I think this PR makes sense (this PR is part of AST to CST module and it will work fine in the redesigned architectures).

@getify, maybe you have the different design. could you show me it?

@michaelficarra:
What do you think of? I'd like to hear your opinion too.

[michaelficarra]

Yes, that is exactly what I had understood from #133. Thanks for making the awesome diagram.

An additional note: depending on the structure we choose for CSTs, the CST->AST transformation may be the identity function.

[getify]

I think all this discussion is great, but we now have a dedicated place for it: https://github.com/getify/concrete-syntax-tree I would strongly suggest/request that you move these posts from here to an issue thread there.

I just started one there with my diagram of how I see the flow working (I think somewhat more simply). Feel free to hijack that thread, or create a new one, for what's being discussed here.

[Constellation]

Hi @getify and @michaelficarra, I've posted getify/concrete-syntax-tree#3 !

[bezoerb]

@Constellation I changed the condition from expr.leadingComments || expr.trailingComments to extra.comment. Do you add the additional 2 options?

[Constellation]

Hi @getify

Seeing your proposal, you also suggested that

If any tool (like escodegen or any other) wants to add default formatting like whitespace or comments or whatever, they take a CST (or an AST!) and add the "extras" annotations directly to it. A CST then is just an AST + extras.

In your proposal this code will become a part of default formatting. So it will attach extras annotations as the same to this PR adding a comment string. That's similar to my explanation,

This PR adds good default comment handling part to AST to CST

So I think this PR makes sense in both proposals, mine and yours. So I would like to merge it. Do you agree with this?

[Constellation]

@bezoerb

Ah, I'll add them later.

[Constellation]

@bezoerb

Code itself looks nice.

[getify]

So I would like to merge it. Do you agree with this?

This is your project, I didn't mean to butt in and try to control it. If you think it makes sense, sure. I just wasn't clear that this work wasn't duplcative of work that will eventually need to be done once we decide what the CST looks like.

If it's compatible, great. If there's any chance that this work "forks" in a different and non-compatible direction from the CST, and particularly if it could possibly set a precedent (legacy code) that makes it any harder for the eventual CST adoption, I would suggest you consider waiting. Sounds like you've already analyzed that, so I have no objections. :)

[Constellation]

I just wasn't clear that this work wasn't duplcative of work that will eventually need to be done once we decide what the CST looks like.

If so, maybe I don't still understand your proposal completely. I think it's important that this work makes sence in the future CST architecture, and so I'd like to get your agreement since I believe this PR works in the both your and my proposals.

So let's describe my understanding, if it isn't true, I'd appreciate if you would point it.

This PR in your CST = AST + extras architecture

If any tool (like escodegen or any other) wants to add default formatting like whitespace or comments or whatever, they take a CST (or an AST!) and add the "extras" annotations directly to it. A CST then is just an AST + extras.

Since you noted as the above, I imagine that the default formatting module will do

1. Takes CST (or an AST)
2. Attaches textual information (comment, whitespaces) to CST's extras property
3. Returns CST

And then, codegen module will do,

1. Takes CST
2. Generates code from CST

Is it correct?

And in your CST architecture, this PR will be the part of the default formatting module's step 2.
We'll implement attaching comment to CST's extras property in the default formatting module.
There's no duplicate code. Only the codegen module will generate actual string. This PR will only attach comment text to extras property of CST in your architecture.

So I think this makes sence. Do you agree with this? If you don't think so, please point which is incorrect.

This PR in my CST != AST architecture

In my proposal, this PR will become a part of CST to AST module. It is the same as your proposal's default formating module.

[bezoerb]

Hey guys,
sorry to interrupt the discussion. I'm totally in for CST. I'm currently working on a project (gruntfile-api) where i need to modify the syntax tree and write it back to file (at best non destructive). So CST would be exactly what i need.
But for now CST is just a proposal. This PR consists of 3 lines of code. It doesn't matter if this lines would be dropped or replaced when CST is there. Escodegen will need some rewrite at this time either. I Think this discussion has taken more time than just changing these lines when CST is available. So why not just merge this PR and make this library better?

[getify]

The theory sounds just fine here, IMO. My soft objection earlier was I didn't think it was a good idea if it end up being that a tree "standard" assumed as a result of this PR was something like:

{
    "type": "BinaryExpression",
    "operator": "+",
    "left": {
        "type": "Identifier",
        "name": "a",

        "comments": " ... "

    },
    "right": {
        "type": "Identifier",
        "name": "b"
    }
}

That tree structure could end up being incompatible with the eventual CST tree structure, so setting such a precedent now would make it harder to switch later. That's all I meant.

If that kind of precedent isn't being set, then it doesn't seem like a concern to hold up this PR.

[Constellation]

@getify:

The theory sounds just fine here, IMO. My soft objection earlier was I didn't think it was a good idea if it end up being that a tree "standard" assumed as a result of this PR was something like:

Ah, I see! Thank you for your clarification.

To clarify my understanding, I'll describe my thought.

Yes, current default formatting with enabled comments option assumes that leadingComments / trailingComments properties of CSTs/ASTs are set.

However, in my understanding, I think it doesn't become an issue. So let's describing why I think so. if it is not correct or there's missed points, please feel free to point it :)

This PR in your CST = AST + extras architecture

I think this leadingComments / trailingComments is some kind of options for default formatting module (like indent width, etc.). Since there' no reasonable way to provide default formatting parameters to an each node without attaching an properties to it, default formating module takes this way. But I think the most important thing is that default formating module will always produce a standard CST :) Because of this, I think this PR works fine in the future architectures.

I imagine that default formating module will do in your proposal's design (it is more detailed version than before),

1. Takes a CST (or an AST) and options about formatting
2. For each node
   1. Dropping CST's extras property (these will be overwritten by default formatting modules' information, (2.ii) )
   2. Attaches textual information (comment, whitespaces) to CST's extras property
3. Returns CST

leadingComments / trailginComments properties will be only recognized by default formatting module. default formatting module will take them and do some intelligent transformation (adjusting indentation in comments etc.), convert them to CST's standard representation (in this case, extras property) and attach them on CST. leadingComments / trailginComments properties works as default formatting module's optional parameter for an each node. These properties are only recognized by default formatting module and codegen module doesn't recognize them!

So once default formatting is done, we can always get an standard CST. This CST doesn't have (or doesn't consider about) leadingComments / traiingComments properties. They are already represented as CST's standard way by default formatting step (2.ii). Then codegen module only recognize CST's extras properties and codegen doesn't see leadingComments / trainlingComments.

This is important: default formatting module will always produce a standard CST from these options. And codegen doesn't recognize leadingComments / trailginComments. codegen only recognize the standard CST's information!

OK, so let's answer your questions. If my thought is incorrect, I'd appreciate if you would point it!

My soft objection earlier was I didn't think it was a good idea if it end up being that a tree "standard" assumed as a result of this PR was something like:

Fortunately, No :) leadingComments / trailginComments are just optional properties only for default formatting module. It is the same as the other options such as indent, base, etc. for default formatting module. I don't assume that leadingComments / trailginComments properties are part of a "standard". Actually, codegen module will not recognize them. default formatting module will represent them as an standard CST's way.

If that kind of precedent isn't being set, then it doesn't seem like a concern to hold up this PR.

default formatting module assumes that leadingComments / trailginComments are set. But they are options only for default formatting module. default formatting module will produce a standard CST and codegen module doesn't assume that these optional properties are set.

Is it a good answer for your concerns? If it isn't, please point it :) Let's continue to consider / reconstruct the design!

This PR in my CST != AST architecture

About this, my proposal works as the same. AST to CST module will convert leadingComments / trailginComments and represent them in the standard CST way!

[getify]

If I understand correctly, you will be adding some "non standard properties" (only so far as there is not a CST standard yet) to the AST, but since that's only internal use in your tool, it won't represent a de facto "standard tree format" that could hamper us from a future CST format. Am I correct on that?

[Constellation]

Oh, sorry, I failed to send my comment on GitHub...

If I understand correctly, you will be adding some "non standard properties" (only so far as there is not a CST standard yet) to the AST, but since that's only internal use in your tool, it won't represent a de facto "standard tree format" that could hamper us from a future CST format. Am I correct on that?

Yes! And the important thing is that, default formatting module will always produce the standard CST. These properties are only recognized by this default formatting module. codegen doesn't recognize it because it isn't the standard CST!

[Constellation]

ping? @getify

[getify]

No further objections from me. :)

[Constellation]

Thank you @getify! So I'll merge it.