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

Generic.Formatting.MultipleStatementAlignment error with const DEFAULT #3326

Closed
caseyjhol opened this issue Apr 27, 2021 · 3 comments · Fixed by #3351
Closed

Generic.Formatting.MultipleStatementAlignment error with const DEFAULT #3326

caseyjhol opened this issue Apr 27, 2021 · 3 comments · Fixed by #3351
Milestone

Comments

@caseyjhol
Copy link

caseyjhol commented Apr 27, 2021

Describe the bug
I'm getting an error when attempting to align the assignment tokens when defining constants, if one of the constants is named DEFAULT. It's trying to align DEFAULT with the array assignment token below, rather than the other statements directly below it.

Code sample

<?php

declare(strict_types=1);

// phpcs:ignore
final class Test
{
    public const DEFAULT = 'default';
    public const SOS     = 'sos';
    public const HELP    = 'help';

    // phpcs:ignore
    protected static $thisIsAReallyLongVariableName = [
        self::DEFAULT  => 'DEFAULT',
        self::SOS => 'SOS',
        self::HELP => 'Help',
    ];
}

To reproduce
Steps to reproduce the behavior:

  1. Create a file called test.php with the code sample above...
  2. Run phpcs test.php ...
  3. See error message displayed
  8 | ERROR | [x] Equals sign not aligned with surrounding
    |       |     assignments; expected 28 spaces but found 1
    |       |     space
    |       |     (Generic.Formatting.MultipleStatementAlignment.NotSame)
  9 | ERROR | [x] Equals sign not aligned with surrounding
    |       |     assignments; expected 2 spaces but found 5
    |       |     spaces
    |       |     (Generic.Formatting.MultipleStatementAlignment.NotSame)
 10 | ERROR | [x] Equals sign not aligned with surrounding
    |       |     assignments; expected 1 space but found 4
    |       |     spaces
    |       |     (Generic.Formatting.MultipleStatementAlignment.NotSame)

Expected behavior
No reported errors.

Versions (please complete the following information):

  • OS: Ubuntu 20.10
  • PHP: 7.4
  • PHPCS: 3.6.0
  • Standard: PSR2

Additional context
To clarify the issue, this throws no errors:

<?php

declare(strict_types=1);

// phpcs:ignore
final class Test
{
    public const DEFAULT                            = 'default';
    public const SOS  = 'sos';
    public const HELP = 'help';

    // phpcs:ignore
    protected static $thisIsAReallyLongVariableName = [
        self::DEFAULT  => 'DEFAULT',
        self::SOS => 'SOS',
        self::HELP => 'Help',
    ];
}

This also throws no errors:

<?php

declare(strict_types=1);

// phpcs:ignore
final class Test
{
    public const DEFAULX = 'default';
    public const SOS     = 'sos';
    public const HELP    = 'help';

    // phpcs:ignore
    protected static $thisIsAReallyLongVariableName = [
        self::DEFAULX  => 'DEFAULT',
        self::SOS => 'SOS',
        self::HELP => 'Help',
    ];
}
@caseyjhol caseyjhol changed the title Generic.Formatting.MultipleStatementAlignment: assignment token aligning with wrong surrounding assignments Generic.Formatting.MultipleStatementAlignment: const DEFAULT assignment token aligning with wrong surrounding assignments Apr 27, 2021
@jrfnl
Copy link
Contributor

jrfnl commented May 12, 2021

I've been looking into this issue.

A git bisect learns that it first surfaces with commit 2ba5393 in response to #3219.

That commit, however, did not cause the issue. Rather, it exposes an underlying issue related to #3336.

After PHP::tokenize(), the DEFAULT is still tokenized as T_DEFAULT. This causes the Tokenizer::recurseScopeMap() to assign it as the scope_opener to the ; semi-colon at the end of the constant declaration, with the class close curly brace being set as the scope_closer.
In the PHP::processAdditional() method, the DEFAULT is subsequently retokenized to T_STRING as it is preceded by a const keyword, but that is too late.

The scope_opener being set on the semi-colon is what is causing the errors to be displayed for the above code sample.

So, like #3336, this is a problem with reserved keywords being used as names in valid contexts, but still being tokenized as the reserved keyword by PHP. As discussed in #3020, the TOKEN_PARSE flag would solve this, but using it is not an option due to PHPCS 3.x still supporting PHP < 7.0, as well as that flag making the tokenizer parse error intolerant, so this will need to be solved differently at a PHPCS tokenizer level and needs to be solved in PHP::tokenize(), not PHP::processAdditional() as solving it in the latter still allows for the scope map to be incorrectly set.

jrfnl added a commit to jrfnl/PHP_CodeSniffer that referenced this issue May 12, 2021
As per: squizlabs#3326 (comment)

> After `PHP::tokenize()`, the `DEFAULT` is still tokenized as `T_DEFAULT`. This causes the `Tokenizer::recurseScopeMap()` to assign it as the `scope_opener` to the `;` semi-colon at the end of the constant declaration, with the class close curly brace being set as the `scope_closer`.
> In the `PHP::processAdditional()` method, the `DEFAULT` is subsequently retokenized to `T_STRING` as it is preceded by a `const` keyword, but that is too late.
>
> The `scope_opener` being set on the semi-colon is what is causing the errors to be displayed for the above code sample.

The commit fixes this by:
1. Abstracting the list of `T_STRING` contexts out to a class property.
2. Using the list from the property in all places in the `Tokenizer\PHP` class where keyword tokens are (potentially) being re-tokenized to `T_STRING`, including in the `T_DEFAULT` tokenization code which was added to address the PHP 8.0 `match` expressions.
    Note: the issue was not introduced by `match` related code, however, that code being there does make it relatively easy now to fix this particular case.

While this doesn't address 3336 yes, it is a step towards addressing it and will sort out one of the most common causes for bugs.
@jrfnl
Copy link
Contributor

jrfnl commented May 12, 2021

@caseyjhol Thanks for reporting this. PR #3351 should fix this. Testing appreciated.

@gsherwood gsherwood added this to the 3.6.1 milestone May 27, 2021
jrfnl added a commit to jrfnl/PHP_CodeSniffer that referenced this issue May 27, 2021
As per: squizlabs#3326 (comment)

> After `PHP::tokenize()`, the `DEFAULT` is still tokenized as `T_DEFAULT`. This causes the `Tokenizer::recurseScopeMap()` to assign it as the `scope_opener` to the `;` semi-colon at the end of the constant declaration, with the class close curly brace being set as the `scope_closer`.
> In the `PHP::processAdditional()` method, the `DEFAULT` is subsequently retokenized to `T_STRING` as it is preceded by a `const` keyword, but that is too late.
>
> The `scope_opener` being set on the semi-colon is what is causing the errors to be displayed for the above code sample.

The commit fixes this by:
1. Abstracting the list of `T_STRING` contexts out to a class property.
2. Using the list from the property in all places in the `Tokenizer\PHP` class where keyword tokens are (potentially) being re-tokenized to `T_STRING`, including in the `T_DEFAULT` tokenization code which was added to address the PHP 8.0 `match` expressions.
    Note: the issue was not introduced by `match` related code, however, that code being there does make it relatively easy now to fix this particular case.

While this doesn't address 3336 yes, it is a step towards addressing it and will sort out one of the most common causes for bugs.
@gsherwood gsherwood changed the title Generic.Formatting.MultipleStatementAlignment: const DEFAULT assignment token aligning with wrong surrounding assignments Generic.Formatting.MultipleStatementAlignment error with const DEFAULT Jun 30, 2021
gsherwood pushed a commit that referenced this issue Jun 30, 2021
As per: #3326 (comment)

> After `PHP::tokenize()`, the `DEFAULT` is still tokenized as `T_DEFAULT`. This causes the `Tokenizer::recurseScopeMap()` to assign it as the `scope_opener` to the `;` semi-colon at the end of the constant declaration, with the class close curly brace being set as the `scope_closer`.
> In the `PHP::processAdditional()` method, the `DEFAULT` is subsequently retokenized to `T_STRING` as it is preceded by a `const` keyword, but that is too late.
>
> The `scope_opener` being set on the semi-colon is what is causing the errors to be displayed for the above code sample.

The commit fixes this by:
1. Abstracting the list of `T_STRING` contexts out to a class property.
2. Using the list from the property in all places in the `Tokenizer\PHP` class where keyword tokens are (potentially) being re-tokenized to `T_STRING`, including in the `T_DEFAULT` tokenization code which was added to address the PHP 8.0 `match` expressions.
    Note: the issue was not introduced by `match` related code, however, that code being there does make it relatively easy now to fix this particular case.

While this doesn't address 3336 yes, it is a step towards addressing it and will sort out one of the most common causes for bugs.
gsherwood added a commit that referenced this issue Jun 30, 2021
@gsherwood
Copy link
Member

I confirmed PR #3351 does fix this bug (thanks @jrfnl). Thanks a lot for the bug report. The fix will be in 3.6.1.

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.

3 participants