Rector does not properly detect usage of whitespaces or tabs

[alexanderschnitzler]

Bug Report

Subject	Details
Rector version	e.g. v0.7.2 (invoke vendor/bin/rector --version)
Installed as	global binary via composer

Hi,
I am using version 0.7.2 (issue exists since 0.7.1) and rector messes with my doc block indents.
It replaces whitespace with tabs btw.

[EDIT] I tracked it down using git bisect and it tells me that the following commit introduced that behavior: bf97e87

The file used in this report can be found here:
https://github.com/TYPO3/TYPO3.CMS/blob/4188bca895e0009d7571e034c2ee4ca908dc59c8/typo3/sysext/backend/Classes/Controller/LoginController.php

rector process typo3/sysext/backend/Classes/Controller/LoginController.php --set=php53 --debug
File "/home/alexander/Development/TYPO3-development/10/TYPO3.CMS/vendor/autoload.php" is about to be loaded in "AutoloadIncluder::includeCwdVendorAutoloadIfExists()" on line 63"
File "/home/alexander/composer-packages/rector/rector/vendor/rector/rector/../../autoload.php" is about to be loaded in "AutoloadIncluder::autoloadProjectAutoloaderFile()" on line 95"
File "/home/alexander/Development/TYPO3-development/10/TYPO3.CMS/vendor/autoload.php" is about to be loaded in "AutoloadIncluder::includeCwdVendorAutoloadIfExists()" on line 63"
File "/home/alexander/composer-packages/rector/rector/vendor/rector/rector/../../autoload.php" is about to be loaded in "AutoloadIncluder::autoloadProjectAutoloaderFile()" on line 95"
Rector v0.7.2
Config file: rector.yml

[parsing] typo3/sysext/backend/Classes/Controller/LoginController.php
[refactoring] typo3/sysext/backend/Classes/Controller/LoginController.php
    [applying] Rector\Php53\Rector\FuncCall\DirNameFileConstantToDirConstantRector
    [applying] Rector\Php53\Rector\FuncCall\DirNameFileConstantToDirConstantRector
    [applying] Rector\Php53\Rector\FuncCall\DirNameFileConstantToDirConstantRector
    [applying] Rector\Php53\Rector\FuncCall\DirNameFileConstantToDirConstantRector
    [applying] Rector\Php53\Rector\FuncCall\DirNameFileConstantToDirConstantRector
    [applying] Rector\Php53\Rector\Ternary\TernaryToElvisRector
    [applying] Rector\Php53\Rector\FuncCall\DirNameFileConstantToDirConstantRector
    [applying] Rector\Php53\Rector\FuncCall\DirNameFileConstantToDirConstantRector
    [applying] Rector\Php53\Rector\FuncCall\DirNameFileConstantToDirConstantRector
    [applying] Rector\Php53\Rector\FuncCall\DirNameFileConstantToDirConstantRector
    [applying] Rector\Php53\Rector\FuncCall\DirNameFileConstantToDirConstantRector
    [applying] Rector\Php53\Rector\FuncCall\DirNameFileConstantToDirConstantRector
    [applying] Rector\Php53\Rector\FuncCall\DirNameFileConstantToDirConstantRector
    [applying] Rector\Php53\Rector\FuncCall\DirNameFileConstantToDirConstantRector
    [applying] Rector\Php53\Rector\FuncCall\DirNameFileConstantToDirConstantRector
    [applying] Rector\Php53\Rector\FuncCall\DirNameFileConstantToDirConstantRector
    [applying] Rector\Php53\Rector\FuncCall\DirNameFileConstantToDirConstantRector
    [applying] Rector\Php53\Rector\FuncCall\DirNameFileConstantToDirConstantRector
    [applying] Rector\Php53\Rector\FuncCall\DirNameFileConstantToDirConstantRector
    [applying] Rector\Php53\Rector\FuncCall\DirNameFileConstantToDirConstantRector
[printing] typo3/sysext/backend/Classes/Controller/LoginController.php


1 file with changes
===================

1) typo3/sysext/backend/Classes/Controller/LoginController.php

    ---------- begin diff ----------
--- Original
+++ New
@@ -78,13 +78,13 @@
     protected $loginProviders = [];

     /**
-     * Login-refresh bool; The backend will call this script
-     * with this value set when the login is close to being expired
-     * and the form needs to be redrawn.
-     *
-     * @var bool
-     */
-    protected $loginRefresh;
+				 * Login-refresh bool; The backend will call this script
+				 * with this value set when the login is close to being expired
+				 * and the form needs to be redrawn.
+				 *
+				 * @var bool
+				 */
+				protected $loginRefresh;

     /**
      * Value of forms submit button for login.
@@ -131,13 +131,13 @@
     }

     /**
-     * Injects the request and response objects for the current request or subrequest
-     * As this controller goes only through the main() method, it is rather simple for now
-     *
-     * @param ServerRequestInterface $request the current request
-     * @return ResponseInterface the finished response with the content
-     */
-    public function formAction(ServerRequestInterface $request): ResponseInterface
+				 * Injects the request and response objects for the current request or subrequest
+				 * As this controller goes only through the main() method, it is rather simple for now
+				 *
+				 * @param ServerRequestInterface $request the current request
+				 * @return ResponseInterface the finished response with the content
+				 */
+				public function formAction(ServerRequestInterface $request): ResponseInterface
     {
         $this->init($request);
         return new HtmlResponse($this->createLoginLogoutForm($request));
@@ -448,12 +448,12 @@
     }

     /**
-     * Gets news from sys_news and converts them into a format suitable for
-     * showing them at the login screen.
-     *
-     * @return array An array of login news.
-     */
-    protected function getSystemNews(): array
+				 * Gets news from sys_news and converts them into a format suitable for
+				 * showing them at the login screen.
+				 *
+				 * @return array An array of login news.
+				 */
+				protected function getSystemNews(): array
     {
         $systemNewsTable = 'sys_news';
         $queryBuilder = GeneralUtility::makeInstance(ConnectionPool::class)
@@ -476,15 +476,15 @@
     }

     /**
-     * Returns the uri of a relative reference, resolves the "EXT:" prefix
-     * (way of referring to files inside extensions) and checks that the file is inside
-     * the project root of the TYPO3 installation
-     *
-     * @param string $filename The input filename/filepath to evaluate
-     * @return string Returns the filename of $filename if valid, otherwise blank string.
-     * @internal
-     */
-    private function getUriForFileName($filename): string
+				 * Returns the uri of a relative reference, resolves the "EXT:" prefix
+				 * (way of referring to files inside extensions) and checks that the file is inside
+				 * the project root of the TYPO3 installation
+				 *
+				 * @param string $filename The input filename/filepath to evaluate
+				 * @return string Returns the filename of $filename if valid, otherwise blank string.
+				 * @internal
+				 */
+				private function getUriForFileName($filename): string
     {
         // Check if it's already a URL
         if (preg_match('/^(https?:)?\/\//', $filename)) {
@@ -556,13 +556,13 @@
     }

     /**
-     * Detect the login provider, get from request or choose the
-     * first one as default
-     *
-     * @param ServerRequestInterface $request
-     * @return string
-     */
-    protected function detectLoginProvider(ServerRequestInterface $request): string
+				 * Detect the login provider, get from request or choose the
+				 * first one as default
+				 *
+				 * @param ServerRequestInterface $request
+				 * @return string
+				 */
+				protected function detectLoginProvider(ServerRequestInterface $request): string
     {
         $parsedBody = $request->getParsedBody();
         $queryParams = $request->getQueryParams();
    ----------- end diff -----------

Expected Behaviour

I'd expect rector to not touch the indents at all.

[alexanderschnitzler]

I just checked the behavior of versions 0.7.0, 0.7.1 and 0.7.2 and this issue occurs since 0.7.1.

[alexanderschnitzler]

The behavior changed with commit bf97e87

[TomasVotruba]

If you've digged so deeply, could you send PR with fix?

[alexanderschnitzler]

I figured out what causes this. The mentioned file (of the TYPO3 core) contains HTML-Snippets which use tabs instead of whitespaces. With that change I mentioned in my last comment, you prefer tabs over whitespaces as soon as you detect a single tab in the whole file.

If you've digged so deeply, could you send PR with fix?

I honestly don't know what a right fix is in this situation. Maybe introduce a configuration to set tabs vs. whitespaces and don't guess which is used?

[TomasVotruba]

I honestly don't know what a right fix is in this situation.

What would you expect as default as a user?

[alexanderschnitzler]

Given that we follow PSR's and given there is PSR-2 which defines the usage of 4 spaces over tabs, my personal opinion is that spaces should be the default.

[TomasVotruba]

Maybe introduce a configuration to set tabs vs. whitespaces and don't guess which is used?

How would that work with files with different formatting? Or with the files you've provided (tabs and spaces in one file)

[TomasVotruba]

Given that we follow PSR's and given there is PSR-2 which defines the usage of 4 spaces over tabs, my personal opinion is that spaces should be the default.

Can be. But how would you detect tabs in file?

[alexanderschnitzler]

How would that work with files with different formatting? Or with the files you've provided (tabs and spaces in one file)

Well, I think there is no way to properly deal with mixed tabs and whitespaces. In my case I'd think that rector either ignores my code at this point or uses whitespaces all the way.

It's maybe a stupid idea but what about this idea:
We compare the amount of ^\t vs ^\ {4} and the higher usage wins. wdyt?

[TomasVotruba]

...uses whitespaces all the way.

That's the only way. Rector uses php-parser printer + one for doc, so spaces will be always printed.

We compare the amount of ^\t vs ^\ {4} and the higher usage wins. wdyt?

How does it work this use case?

[alexanderschnitzler]

$whitespaces = (int)preg_match_all('/^ {4}/m', $fileInfo->getContents());
$tabs = (int)preg_match_all('/^\t/m', $fileInfo->getContents());

// tab vs space
$this->tabOrSpaceIndentCharacter = ($whitespaces <=> $tabs) >= 0 ? ' ' : "\t";

This is a quick approach.