Skip to content

Incorrect file/line blamed for errors caused by new SomeClass #7771

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

Closed
dktapps opened this issue Dec 13, 2021 · 4 comments
Closed

Incorrect file/line blamed for errors caused by new SomeClass #7771

dktapps opened this issue Dec 13, 2021 · 4 comments

Comments

@dktapps
Copy link
Contributor

dktapps commented Dec 13, 2021

Description

https://3v4l.org/UtjRl

This code should blame line 4, not line 7, since that's where the bogus constant is actually declared.


An example of where this is more problematic:

SomeClass.php

<?php

class SomeClass{
	public const INGOT = IDontExist::INGOT;
}

test.php

<?php

require 'SomeClass.php';

new SomeClass();

Resulted in this output:

Fatal error: Uncaught Error: Class "IDontExist" not found in C:\stable\test4.php:5

But I expected this output instead:

Fatal error: Uncaught Error: Class "IDontExist" not found in C:\stable\SomeClass.php:4

This frequently causes issues with my telemetry because incorrect subsystems are getting blamed for errors when third-party plugin code comes into play.

PHP Version

8.0.13

Operating System

Windows (doubtful if relevant)

@nikic
Copy link
Member

nikic commented Dec 13, 2021

Reporting the line of the AST evaluation would indeed be preferable, and I believe the necessary information is already present, but not used.

@iluuu1994
Copy link
Member

Note that this is not a fatal error and thus can be caught. https://3v4l.org/ghG5d#v8.1.0 Not sure what the full stack trace should be if this were to be changed to the position of the constant itself. Just one more frame with the constant AST itself?

@cmb69
Copy link
Member

cmb69 commented Dec 14, 2021

iluuu1994 added a commit to iluuu1994/php-src that referenced this issue Feb 5, 2022

Verified

This commit was signed with the committer’s verified signature.
iluuu1994 Ilija Tovilo
Closes phpGH-7771
@iluuu1994
Copy link
Member

I created a small POC here. https://github.com/php/php-src/compare/master...iluuu1994:gh-7771?expand=1

The approach in the PR is to simply replace filename/lineno in the exception, similar to ErrorException. Since there are tons paths for throwing errors in constant expressions I avoided passing the name/lineno by storing it in the execution globals.

Unfortunately the lineno in the AST somewhere. I'm still trying to figure out why. Let me know if this approach is ok before I spend too much time on this.

iluuu1994 added a commit to iluuu1994/php-src that referenced this issue Mar 4, 2022

Verified

This commit was signed with the committer’s verified signature.
iluuu1994 Ilija Tovilo
Closes phpGH-7771
Closes phpGH-8124
iluuu1994 added a commit to iluuu1994/php-src that referenced this issue Mar 5, 2022

Verified

This commit was signed with the committer’s verified signature.
iluuu1994 Ilija Tovilo
Closes phpGH-7771
Closes phpGH-8124
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants