Optionally add nodes for whitespace

[fabpot]

I don't know if that's part of the scope of PHP-Parser, but it would be useful (at least for me as it would greatly simplify the code for my PHP CS fixer -- http://cs.sensiolabs.org/) to be able to have nodes for whitespace. This should probably be an option as most of the time, you don't care about whitespace, and also because it would add many nodes.

[nikic]

I think having whitespace information available would be awesome for a lot of applications.. But I can't add whitespaces nodes into the current AST structure. To preserve all whitespace, comments and formatting one would need a concrete syntax tree that would basically a tree that tags stuff on the token stream. For "<?php\n echo 1 + 2;" it would something like this:

PHP[
    T_OPEN_TAG(<?php\n)
    T_WHITESPACE(     )
    Echo[
        T_ECHO(echo)
        T_WHITESPACE( )
        Plus[
            LNumber[
                T_LNUMBER(1)
            ]
            T_WHITESPACE( )
            char(+)
            T_WHITESPACE( )
            LNumber[
                T_LNUMBER(2)
            ]
        ]
        char(;)
    ]
]

This is a completely different structure and would probably require a lot of work to create. (I wanted to look into this far a while already, but never got around to doing it :/ )

Another interesting idea is what @schmittjoh implemented in https://github.com/schmittjoh/php-manipulator: It has the token stream and the AST as separate structures with links between them. I'll look a bit more closer at that and see if it can be integrated with the parser.

[pscheit]

correct me if i'm totally wrong here, but isn't the main problem, that the information of whitespace just don't get incorporated into the AST structure?
This is nearly the same problem as with the comments, isn't it?

I think for example fabpot's usecase is to correct indentation etc. But isn't for this the only need that the whitespace information is attached to the current elements we have in the AST?

like:

T_OPEN_Tag { indentation: 0, newline: true }
Echo { indentation: 4, inline-whitespace: 0 }
  LNumber { inline-whitespace: 1}
  char(+) { inline-whitespace: 1}
  LNumber { inline-whitespace: 1}
  char(;} {}

the whitespace information could be attached in a way that makes "most sense". I admit that this is the big part of the work. I used the convention to attach the inline-whitespace to the node after before this is more consistent with identation counting), but last whitespace would be lost.
This additional code-identation-information would allow to write indentation fixers and code-re-compilers

[schmittjoh]

Why not storing whitespace information in the existing nodes as attributes?

[pscheit]

that is what i wanted to say :)

[nikic]

@pscheit @schmittjoh Storing whitespace in the node attributes would come with the same problems that currently exist for comments, just worse. For comments I currently simply store all comments that occur before the node. That works in most cases, but not always (e.g. issue #36 and #37).

For whitespace it's a lot more problematic, because unlike comments whitespace usually also occurs within the node and not just before it. So for echo 1 + 2;, where does the whitespace go? It isn't really helpful if I add a whitespace = [' ', ' ', ' '] attribute to the node.

[schmittjoh]

You would need to create a custom format per node, but a separate structure makes all code that is written for the current structure incompatible which would be a huge drawback from my pov at least.

[pscheit]

if whitespace before and whitespace after and line breaks would be captured for every node, the indentation could be computed. This would store the same whitespace twice in a node.
Otherwise just the whitespace before could be captured or the whitespace coming after every node. It just has to be consistent..

phptag: after-whitespace: eol
echo: before-whitespace eol + 4 after-whitespace: 1
LNumber(1): before-whitespace: 1 after-whitespace: 1

etc
of course this is hugely verbose, but thats the point with indentation

[jakoch]

Well, extending the lexer to handle tokenization of whitespace and comments isn't too hard.
This information must then be passed on to the parser. If you need to avoid getting the current structure incompatible, then you might add a step between lexer and parser, something like a ParserFilter or ParserPrefilter, just to handle a seperate AST with whitespace and comment tokens next to the current AST.
Storing them as attributes in one AST is not good for tree walking, because you would need to walk the whole tree with subtrees, to adjust the values. When you have two ASTs the next step would be merging them.
I would add a rule to the first AST, so that every token is followed by a whitespace and then ask the other tree for the rest of the informations (this saves lots of tree merges in inline stmts, think of the 1+2 example).

[nikic]

@schmittjoh @pscheit Having a custom format for every node would cause a lot of work both in the implementation and the use lateron, because every node would require special handling.

That's why I currently lean towards the approach where just the offset into the token stream is saved in the nodes. So every node would have a startOffset and endOffset attribute, which could be used to look up the tokens it is composed off in the token stream. This is something that can be done automatically and the mechanisms for it are already in place. I quickly tried this out and it basically seems to work, though it looks like I sometimes get an off-by-one error for the end token (probably some bug in the handling for the end attributes).

I think this approach is very nice because it cleanly separates the abstract syntax tree and it's concrete formatting.

What will be a bit tricky about this is to figure out how to properly pretty print a partially changed AST. One somehow has to figure out which parts were changed and pretty print only those. And there too one would probably try to keep as much of their content in the original form.

[pscheit]

sounds good, at least something i can live with :)

because my current use case would really be just a partly changed AST (and i know exactly the parts, i have changed). I'm looking forward to this!

[schmittjoh]

This sounds very similar to the approach that I follow in php-manipulator.

The usage there is basically to jump to an AST node that you are interested in re-writing, at which point you would which to the token stream, re-write the node, and then switch back to the AST stream. If you can provide some mechanisms to make the mapping of AST node to tokens that would help me to simplify that code.

[nikic]

@schmittjoh I just fixed a small bug that was getting in the way of properly doing the mapping (cdbad02), but it should be as easy as using a custom lexer that specifies the necessary attributes:

<?php

class LexerWithTokenOffsets extends PHPParser_Lexer {
    public function getNextToken(&$value = null, &$startAttributes = null, &$endAttributes = null) {
        $tokenId = parent::getNextToken($value, $startAttributes, $endAttributes);
        $startAttributes['startOffset'] = $endAttributes['endOffset'] = $this->pos;
        return $tokenId;
    }
}

From a few quick tests this works fairly well, though there will be some rare cases where this will be off (those can be fixed without much effort).

[lstrojny]

This would be indeed very helpful for all sorts of transformations that can’t use Generation Gap.

[nikic]

My initial attempt at a pretty printer that tries to preserve the formatting: https://gist.github.com/4365484 It turned out really hard to do this (the main issue being indentation) and the current version still doesn't do a particularly good job.

@lstrojny What is Generation Gap?