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

Minor inconsistency with endFilePos of error nodes (in ClassConstFetch) #359

Closed
Gert-dev opened this issue Feb 23, 2017 · 3 comments
Closed

Comments

@Gert-dev
Copy link

Hello

I received the following example code from a user:

<?php

namespace Test;

define('abc', \Test::);

As you might have spotted, this file contains syntax errors. I'm parsing it with php-parse -r -P. This reports that the ClassConstFetch runs from offset 15 through to 22. When I change it into:

<?php

namespace Test;

define('abc', \Test::t);

... the parser correctly reports the name node with name t, but then also goes on to report that the ClassConstFetch runs from offset 15 to 22. In other words, the t does not seem to have counted for any increase in offset with respect to its omission. When you change t to ts, the end offset does increase to 23.

I was wondering if this is intentional or maybe a bug? I'm selecting text at the location of the value of the define statement and always used a length of endFilePos - startFilePos + 1. This works correctly, but when there is an error node, the string then suddenly also includes the closing brace of the define statement, even though there is no actual name node present.

Thanks in advance

@nikic
Copy link
Owner

nikic commented Feb 23, 2017

It's not really clear what attributes should be assigned to an Error node. Right now the end position of the Error node is the last shifted token, which is also the token at which the error initially occurred. In your example that would be the ), which is why it is part of the range. An alternative interpretation would be to make Error an empty node, so that the start position would be at ( and the end at :.

The ClassConstFetch case has an additional problem, because the start offset is not correct. The Error should start at 22, but it starts at 15 (the start of the ClassConstFetch).

@Gert-dev
Copy link
Author

I'm not familiar with the impact this has on the rest of the code or how hard this is to accomplish, of course, but it seems as if the error node should never be including a token that is not part of it at all, even if it were valid. Is it perhaps an option to make this error node take up no space whatsoever or, reduce its endFilePos by one?

nikic added a commit that referenced this issue Feb 26, 2017
@nikic
Copy link
Owner

nikic commented Feb 26, 2017

Empty error nodes implemented with 48ec654. Fingers crossed this doesn't regress anything else...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants