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

Multiple variables in @psalm-trace #6920

Merged
merged 7 commits into from
Nov 15, 2021
Merged

Conversation

rarila
Copy link
Contributor

@rarila rarila commented Nov 15, 2021

Psalm did allow multiple variables in @psalm-trace but failed to add multiple trace issues to the same place.

That way, both cases in this example fail:

https://psalm.dev/r/31262a113f

Additional to fixing this bug, I’ve added the ability to use a comma as separator, so this line is valid also:

/** @psalm-trace $a, $b */

This is probably more natural for some people.

Caveat:

  • I don’t know if my fix is the best way to allow multiple traces or if changing the format of $emitted_key for that case breaks something elsewhere. But no included test fails (at least none related to my changes) and using this codebase in another project worked as expected.
  • The additional tests test only half of the addition. I don’t know if is possible to expect multiple errors in the test framework. At least the test asserts that adding two variables doesn’t break getting the first.

@psalm-github-bot
Copy link

I found these snippets:

https://psalm.dev/r/31262a113f
<?php

$a = 123;
$b = 456;

/** @psalm-trace $a $b */

$c = 123;
$d = 456;

/** @psalm-trace $c */
/** @psalm-trace $d */
Psalm output (using commit 45e1a1c):

INFO: Trace - 8:1 - $a: 123

INFO: Trace - 12:23 - $d: 456

INFO: UnusedVariable - 3:1 - $a is never referenced or the value is not used

INFO: UnusedVariable - 4:1 - $b is never referenced or the value is not used

INFO: UnusedVariable - 8:1 - $c is never referenced or the value is not used

INFO: UnusedVariable - 9:1 - $d is never referenced or the value is not used

Copy link
Collaborator

@weirdan weirdan left a comment

Choose a reason for hiding this comment

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

👍

@weirdan weirdan added the release:feature The PR will be included in 'Features' section of the release notes label Nov 15, 2021
@weirdan weirdan changed the title Fix multiple variables in @psalm-trace Multiple variables in @psalm-trace Nov 15, 2021
@weirdan
Copy link
Collaborator

weirdan commented Nov 15, 2021

if changing the format of $emitted_key for that case breaks something elsewhere

I think that's unlikely.

I don’t know if is possible to expect multiple errors in the test framework

Not with Valid/InvalidCodeTrait, as those tests rely on Psalm throwing exceptions instead of storing all issues reported. It's possible to arrange a test emitting several issues like it's done in, e.g., ReportOutputTest.

@weirdan weirdan merged commit bd583ae into vimeo:master Nov 15, 2021
@weirdan
Copy link
Collaborator

weirdan commented Nov 15, 2021

Thanks!

@pilif
Copy link
Contributor

pilif commented Nov 15, 2021

For checking for multiple errors, see also #6899 where I had to do that last week

@rarila rarila deleted the fix-psalm-trace branch November 15, 2021 13:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release:feature The PR will be included in 'Features' section of the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants