decaf icon indicating copy to clipboard operation
decaf copied to clipboard

Multi and single line comments

Open juliankrispel opened this issue 9 years ago • 15 comments

The coffeescript compiler unfortunately completely ditches single line comments and currently there's no easy way to insert multiline comments into the ast (ast-types doesn't make that easy).

juliankrispel avatar Jan 25 '16 16:01 juliankrispel

why? am i missing something?

flying-sheep avatar Jan 26 '16 09:01 flying-sheep

@flying-sheep feel free to give it a try it'd be pretty awesome to get some help on it.

Comment nodes differ from any other nodes (this is not obvious from reading the source) in that comments need to have a location object to be usable by the recast printer and they need to be attached to a block (this can be the block of a program or a function body).

The problem with having to have a location object is that the transpiler needs to know where in relation to other nodes the comment should be positioned, currently the transpiler doesn't know or care about location.

juliankrispel avatar Jan 26 '16 09:01 juliankrispel

ok, so what i understood (or thought to understand) from your comment:

  1. nodes are organized in blocks. a block is a sequence of expression nodes or so
  2. blocks have info about line numbers and so on where nodes reside
  3. comments are attached to blocks
  4. apart from comments, the structure of code is completely independent from that location information, so you ignoring comments you can convert to JS using only the node structure

is that all right?

flying-sheep avatar Jan 26 '16 10:01 flying-sheep

  1. nodes are organized in blocks. a block is a sequence of expression nodes or so

Simplified but yes.

In answer to 2,3,4

Generally yes, here's some more explaining:

Blocks are generally nodes themselves. If you parse a bunch of javascript code with recast, recast attaches a location object to every node that tells you about line numbers etc. Every node inside that block will also get a location object.

However, we're not parsing javascript code here. We're building an AST manually (with the information we get from the coffeescript compiler). Generally, the part of recast responsible for printing code doesn't need location objects except when it comes to comments and here's my theory why:

All block expressions live in block.body, but comments live in block.comments. This is very awkward, because comment nodes are stored in a different place, the printer now needs to know about where to place these comments in relation to other nodes, at least that's my theory as to why recast will not print comments that don't have a location object.

juliankrispel avatar Jan 26 '16 10:01 juliankrispel

and how do the location objects look? line/column coordinates? or node indices? or what?

because if it’s the former, the AST also has to know about how exactly the code will be rendered (e.g. regarding whitespace), which really makes it all awkward. suddenly it’s all no longer semantics with attached presentation, but instead semantics tied to presentational information. (once you change semantic information, you also have to change presentational information)

is this the case?

flying-sheep avatar Jan 26 '16 13:01 flying-sheep

line/column coordinates, which makes the whole thing rather difficult.

juliankrispel avatar Jan 26 '16 13:01 juliankrispel

I wonder what the reason is to treat comment nodes differently than any other nodes

juliankrispel avatar Jan 26 '16 13:01 juliankrispel

hmm, looking at recast’s printComments function, it works if we’re OK with simply adding a comment node without loc.

then this will be skipped but this will be executed

else we need those parts to work (flow-like type annotations)

Node {
    comment: Comment {
        loc: Location {
            lines: Lines {
                skipSpaces: (start: number, backward: boolean) => number,
                firstPos: () => number,
                slice: (start: number, end: number) => Lines,
                ...
            },
            start: number,
            end: number,
            ...
        },
        leading: boolean,
        trailing: boolean,
        type: "Block" | "Line",
        ...
    },
    ...
}

flying-sheep avatar Jan 27 '16 12:01 flying-sheep

Hi @flying-sheep. I really appreciate the company :heart:

I don't get what you're saying here:

hmm, looking at recast’s printComments function, it works if

It works if what?

else we need those parts to work (flow-like type annotations)

It's entirely possible that I'm missing something glaringly obvious, but how are type annotations related to this issue?

juliankrispel avatar Jan 27 '16 13:01 juliankrispel

eh, sorry, edited the second part, forgot to finish the first.

it works if we’re OK with simply adding a comment node without loc.

then this will be skipped but this will be executed

flying-sheep avatar Jan 27 '16 13:01 flying-sheep

@flying-sheep how would that work? Where would the comment be positioned? I thought we just established that it wouldn't work and benjamin sorta confirmed it right?

juliankrispel avatar Jan 27 '16 13:01 juliankrispel

based on your explanation. but look at the code: isn’t it so that if you simply don’t have .loc, it will just be inserted before/after that node?

/e: yes it will, but it’s attached to the function body block, as you said (so it can be inserted before/after it)

as said in https://github.com/benjamn/recast/issues/159#issuecomment-76054246, there are methods that reattach comments. maybe we can use one to reattach comments from the body itself to the body nodes (e.g. statements in a function body)?

flying-sheep avatar Jan 27 '16 14:01 flying-sheep

hmm, maybe like that:

for each comment:

  1. identify tightest enclosing node (i.e. whose location info completely wraps the comment’s loc info tightest)
  2. among the enclosing node’s children, identify node on the same line as the comment (if any)
  3. attach as leading or trailing to that node

flying-sheep avatar Jan 28 '16 09:01 flying-sheep

Hmm

juliankrispel avatar Jan 28 '16 14:01 juliankrispel

@flying-sheep that sounds like a reasonable way of doing it :+1:

juliankrispel avatar Jan 29 '16 10:01 juliankrispel