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

Add return shape for Throwable::getTrace #7798

Merged
merged 2 commits into from Mar 18, 2022

Conversation

ciaranmcnulty
Copy link
Contributor

This is based on the documentation for debug-backtrace:
https://php.net/debug_backtrace

Experimentation that shows the object field is not populated for exceptions:
https://3v4l.org/iQoni

And validation on psalm.dev:
https://psalm.dev/r/b35a8df2f3

Should I also update the stubs?
What tests should I add?

@psalm-github-bot
Copy link

I found these snippets:

https://psalm.dev/r/b35a8df2f3
<?php

/**
 * @psalm-type Backtrace list<array{file:string,line:int,function:string,class?:string,type?:'::'|'->',args:array<mixed>}>
 */

/**
 * From:
 *
 * class cls
 * {
 *     public static function meth(int $a, string $b) : void
 *     {
 *         throw new Exception;   
 *     }
 * }
 *
 * @return Backtrace
 */
function traceWhenInsideStaticMethod()
{
    return array (
  		0 => 
  			array (
    			'file' => '/in/9n6qW',
    			'line' => 12,
    			'function' => 'meth',
    			'class' => 'cls',
    			'type' => '::',
    			'args' => 
    			array (
      				0 => 1,
      				1 => 'hello',
    				),
  			),
		);
}


/**
 * From:
 *
 * class cls
 * {
 *     public function meth2(int $a, string $b) : void
 *     {
 *         throw new Exception;   
 *     }
 * }
 *
 * @return Backtrace
 */
function traceWhenInsideObjectMethod()
{
    return array (
      0 => 
      array (
        'file' => '/in/iQoni',
        'line' => 24,
        'function' => 'meth2',
        'class' => 'cls',
        'type' => '->',
        'args' => 
        array (
          0 => 1,
          1 => 'hello',
        ),
      ),
    );
}

/**
 * From:
 *
 * function func()
 * {
 *   throw new Exception;   
 * }
 *
 * @return Backtrace
 */
function traceWhenInsideFunction()
{
    return array (
  		0 => 
  			array (
    			'file' => '/in/2cEkb',
    			'line' => 22,
    			'function' => 'func',
    			'args' => 
    		array (
    		),
  		),
	);
}


/**
 * From:
 *
 * (function() { throw new Exception;})();
 *
 * @return Backtrace
 */
/**
 * From:
 *
 * (function() { throw new Exception;})();
 *
 * @return Backtrace
 */
function traceWhenInsideAnonymousFuncion()
{
    return array (
      0 => 
      array (
        'file' => '/in/QlGeB',
        'line' => 38,
        'function' => '{closure}',
        'args' => 
        array (
        ),
      ),
    );
}

/**
 * From:
 *
 * throw new Exception;
 *
 * @return Backtrace
 */
function traceWhenInGlobalScope()
{
    return array (
    );
}
Psalm output (using commit a9f4148):

No issues!

@ciaranmcnulty
Copy link
Contributor Author

Updated to allow optional args (in the case of an included file)

https://psalm.dev/r/11247acbc9

@psalm-github-bot
Copy link

I found these snippets:

https://psalm.dev/r/11247acbc9
<?php

/**
 * @psalm-type Backtrace list<array{file:string,line:int,function?:string,class?:string,type?:'::'|'->',args?:array<mixed>}>
 */

/**
 * From:
 *
 * class cls
 * {
 *     public static function meth(int $a, string $b) : void
 *     {
 *         throw new Exception;   
 *     }
 * }
 *
 * @return Backtrace
 */
function traceWhenInsideStaticMethod()
{
    return array (
  		0 => 
  			array (
    			'file' => '/in/9n6qW',
    			'line' => 12,
    			'function' => 'meth',
    			'class' => 'cls',
    			'type' => '::',
    			'args' => 
    			array (
      				0 => 1,
      				1 => 'hello',
    				),
  			),
		);
}


/**
 * From:
 *
 * class cls
 * {
 *     public function meth2(int $a, string $b) : void
 *     {
 *         throw new Exception;   
 *     }
 * }
 *
 * @return Backtrace
 */
function traceWhenInsideObjectMethod()
{
    return array (
      0 => 
      array (
        'file' => '/in/iQoni',
        'line' => 24,
        'function' => 'meth2',
        'class' => 'cls',
        'type' => '->',
        'args' => 
        array (
          0 => 1,
          1 => 'hello',
        ),
      ),
    );
}

/**
 * From:
 *
 * function func()
 * {
 *   throw new Exception;   
 * }
 *
 * @return Backtrace
 */
function traceWhenInsideFunction()
{
    return array (
  		0 => 
  			array (
    			'file' => '/in/2cEkb',
    			'line' => 22,
    			'function' => 'func',
    			'args' => 
    		array (
    		),
  		),
	);
}


/**
 * From:
 *
 * (function() { throw new Exception;})();
 *
 * @return Backtrace
 */
/**
 * From:
 *
 * (function() { throw new Exception;})();
 *
 * @return Backtrace
 */
function traceWhenInsideAnonymousFuncion()
{
    return array (
      0 => 
      array (
        'file' => '/in/QlGeB',
        'line' => 38,
        'function' => '{closure}',
        'args' => 
        array (
        ),
      ),
    );
}

/**
 * From:
 *
 * throw new Exception;
 *
 * @return Backtrace
 */
function traceWhenInGlobalScope()
{
    return array (
    );
}

/**
 * From:
 *
 * throw new Exception;
 *
 * @return Backtrace
 */
function traceWhenInInclude()
{
    return array (
      0 => array (
        'file' => '/Users/ciaranmcnulty/Code/composer-scratch-test/test.php',
        'line' => 3,
        'function' => 'include',
      ),
    );
}
Psalm output (using commit a9f4148):

No issues!

@orklah
Copy link
Collaborator

orklah commented Mar 17, 2022

Seems great!

You'll have to change Callmap_Historical too for tests to pass

@ciaranmcnulty
Copy link
Contributor Author

@orklah should I update /stubs too? I'm not sure the connection between the two things

@AndrolGenhald
Copy link
Collaborator

AndrolGenhald commented Mar 17, 2022

Yes, stubs should be updated too. CallMap is there because it's much faster to parse for simple functions, but stubs exist to add annotations that don't work in the CallMap, so the stubs override the CallMap. It looks like CoreImmutableClasses.phpstub is the only stub that needs changed?

@orklah orklah added the release:fix The PR will be included in 'Fixes' section of the release notes label Mar 17, 2022
@orklah
Copy link
Collaborator

orklah commented Mar 17, 2022

Note: you made a PR on master, it means it will be released as a part of Psalm 5 in the near future. If it's important to you to get this quickly, you may want to rebase your PR onto 4.x so it can be released faster

This is based on the documentation for debug-backtrace:
   https://php.net/debug_backtrace

Experimentation that shows the `object` field is not populated for exceptions:
   https://3v4l.org/iQoni

And validation on psalm.dev:
   https://psalm.dev/r/b35a8df2f3
@ciaranmcnulty ciaranmcnulty changed the base branch from master to 4.x March 18, 2022 09:41
@ciaranmcnulty
Copy link
Contributor Author

I rebased onto 4.x

I'm not sure what's happened there with Appveyor?

@ciaranmcnulty
Copy link
Contributor Author

Closed/reopened to try and nudge appveyor :/

@orklah
Copy link
Collaborator

orklah commented Mar 18, 2022

Appveyor was removed from our CI ages ago, but sometimes on rebase it pops again, not sure why. As long as the rest of the CI runs, it's good

@orklah orklah merged commit b4ae3a9 into vimeo:4.x Mar 18, 2022
@orklah
Copy link
Collaborator

orklah commented Mar 18, 2022

Thanks!

@@ -346,7 +346,7 @@
'ArgumentCountError::getLine' => ['int'],
'ArgumentCountError::getMessage' => ['string'],
'ArgumentCountError::getPrevious' => ['?Throwable'],
'ArgumentCountError::getTrace' => ['list<array<string,mixed>>'],
'ArgumentCountError::getTrace' => ['list<array{file:string,line:int,function:string,class?:string,type?:\'::\'|\'->\',args?:array<mixed>}>'],
Copy link
Contributor

@staabm staabm Mar 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

per phpstan/phpstan-src#914 (comment) file is optional

Copy link
Contributor

@staabm staabm Mar 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and class can be narrowed to class-string.

let me steal this more precise type-union-type into phpstan ;)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

line is optional as well apparently.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release:fix The PR will be included in 'Fixes' section of the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants