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

Switch to bison location tracking #3948

Closed
wants to merge 8 commits into from

Conversation

4 participants
@nikic
Copy link
Member

commented Mar 15, 2019

This switches the location tracking for AST nodes from some custom code based on CG(zend_lineno) to using bison location tracking.

What we were doing before is to take the zend_lineno (end line of a token) of the first available AST child node, or the current zend_lineno if there is none. This mostly works out okay, but is not fully precise, rather convoluted and not extensible. If we ever want to have additional information (such as precise offsets), the current approach will no longer work.

This patch instead uses the native location tracking provided by bison. Similar to AST nodes, locations are kept on a separate stack. Right now the location is just the start line number, but this can now be easily extended.

All the AST constructors now take zend_ast_loc* as the first argument, which is passed as &@$ (in most cases) on the bison side.

nikic added some commits Mar 15, 2019

Basic use of bison location tracking
Enable use of bison location tracking, keeping only information on the
starting line number. Use this in a few places in the parser which
currently perform manual zend_lineno manipulations.
Fixup some locations
If multiple nodes are created in a single action, either split it
up or correct the used location info.
Add a zend_ast_get_loc() helper
Create a zend_ast_loc from an ast node.
Pass loc to create_decl as well
The end_lineno is still passed separately.
Pass location to lex_scan and handle it there
Also use the location of the lookahead token instead of the
pre-reduction location in YYLLOC_DEFAULT for empty reductions.
Also pass loc to ast_create_znode
For consistency.
@krakjoe

This comment has been minimized.

Copy link
Member

commented Mar 15, 2019

@nikic

This comment has been minimized.

Copy link
Member Author

commented Mar 15, 2019

@krakjoe It may impact, but should only be positive.

@krakjoe

This comment has been minimized.

Copy link
Member

commented Mar 15, 2019

That's what I thought, but I was more looking for examples of where the current method is known to be inaccurate, so that we may document the reason for any possible subtle changes when this is merged.

@nikic

This comment has been minimized.

Copy link
Member Author

commented Mar 15, 2019

@krakjoe I'm not sure if anything would even change right now for coverage. I wanted to give something like

<?php
echo
"foobar";

as an example, but this will still put the ZEND_ECHO on line 3 rather than 2. While the information in the AST node is now correct, the way the compiler works it's still prone to pick up the last rather than first lineno inside a node.

I think the main thing this will impact is start line numbers in php-ast (which will now be 2 rather than 3 for the above example).

@krakjoe krakjoe added the Enhancement label Mar 15, 2019

@krakjoe

This comment has been minimized.

Copy link
Member

commented Mar 15, 2019

ok, good

@nikic

This comment has been minimized.

Copy link
Member Author

commented Mar 21, 2019

Merged as e528762.

@nikic nikic closed this Mar 21, 2019

@nuxodin

This comment has been minimized.

Copy link

commented May 2, 2019

Newbie question:
This would mean that someone could implement column numbers for error objects?

set_error_handler(function( $errno , $errstr, $errfile, $errline, $errcontext, $new_errColumn ){
    ...
});

That would be great!

@nuxodin

This comment has been minimized.

Copy link

commented May 15, 2019

I have a comprehensive error-reporting portal where I group the error messages by file, line and column.
With PHP error messages the errors are grouped incorrectly because there is no column-nr.

@staabm

This comment has been minimized.

Copy link
Contributor

commented May 15, 2019

@nuxodin this is not the right channel for your question. I guess you should report a issue on bugs-net or open a discussion on the mailing list

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.