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

Fixed #75426 - "Cannot use empty array elements" reports wrong position #2933

Closed
wants to merge 1 commit into from

Conversation

7 participants
@andrewnester
Copy link
Contributor

commented Nov 23, 2017

Fix for https://bugs.php.net/bug.php?id=75426

It introduces new ZEND_AST_ARRAY_ELEM_EMPTY which used instead of NULL in array/list AST and helps keep track of position of such empty element.

@andrewnester

This comment has been minimized.

Copy link
Contributor Author

commented Nov 29, 2017

@nikic @krakjoe any feedback on it? :)

@krakjoe krakjoe added the Bugfix label Dec 11, 2017

@krakjoe krakjoe requested a review from nikic Feb 8, 2018

@krakjoe

This comment has been minimized.

Copy link
Member

commented Feb 8, 2018

@andrewnester sorry about delay ... requested review from @nikic

@kohenkatz

This comment has been minimized.

Copy link

commented May 11, 2018

@nikic Any update on request from @krakjoe for reviewing this?

@kohenkatz

This comment has been minimized.

Copy link

commented Jul 30, 2018

@staabm Did you perhaps review while scrolled sightly to the right such that the first column of characters was missing? Both of your comments are not correct.

@staabm

This comment has been minimized.

Copy link
Contributor

commented Jul 31, 2018

Hmm I guess it might have been like you said, or a FFNightly or Github Bug.

Looks ok now

@mfn

This comment has been minimized.

Copy link

commented Nov 15, 2018

Just hit this problem, found https://stackoverflow.com/questions/42058233/php-compile-error-cannot-use-empty-array-elements-in-arrays , followed a link here.

Came to say: would be great if that PR could make it in 😄

@nikic

This comment has been minimized.

Copy link
Member

commented Nov 15, 2018

I'm not a fan of adding a new AST node just for this purpose. I think it would be "good enough" to just load CG(zend_lineno) for each array element. This would put the error message at the last valid element, i.e. on the line where the first rather than the second of the two commas is.

@krakjoe

This comment has been minimized.

Copy link
Member

commented Jan 30, 2019

@nikic you seem to have a better approach to solving this, so ... I'm going to add it to your never ending list-o-things ...

@nikic

This comment has been minimized.

Copy link
Member

commented Feb 1, 2019

@krakjoe I've implemented the alternative that I mentioned in 73596c5 some time ago. I think this is "good enough". We could additionally land this change in PHP-7.4 (we cannot change AST structure in older versions) to fix a potential final off-by-one error in the line number, but I don't think this is worthwhile. I will close this for now, but if someone feels strongly about it, we can also land it.

@nikic nikic closed this Feb 1, 2019

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.