squizlabs / PHP_CodeSniffer

PHP_CodeSniffer tokenizes PHP files and detects violations of a defined set of coding standards.
BSD 3-Clause "New" or "Revised" License
10.67k stars 1.48k forks source link

Wrong column reported when using tabs #982

Closed UziTech closed 8 years ago

UziTech commented 8 years ago

The wrong column is reported when the --tab-width parameter is set and the error line uses tabs

gsherwood commented 8 years ago

This is because tabs are converted to n spaces before checking starts, and the column value must be set in terms of spaces so sniffs don't get confused. PHPCS would need to store an orig_column value (as it does with orig_content) when replacing tabs and use this for reporting.

gsherwood commented 8 years ago

I'm not actually sure this should be changed.

Some text editors count tabs as a single column and some count them as 1-n columns, based on tab width. Changing the way reports print the column number might actually break some editor and IDE plugins that count tabs properly.

UziTech commented 8 years ago

I noticed this issue in AtomLinter/linter-phpcs#146. The linter expects the character column.

From the other linters I tested all of them gave the character column (tabs = 1 character).

Is there a way to put this behind a flag for backwards compatibility?

gsherwood commented 8 years ago

Is there a way to put this behind a flag for backwards compatibility?

I'd prefer a single reliable column value, and I think Atom is probably doing the wrong thing here (both Sublime and PHPStorm correctly count tabs when determining column values).

Atom seems to be considering the column value as the "character number" of the line rather than the "column position". It's possible that an Atom plugin can correct this by counting tabs itself, but I have no idea.

UziTech commented 8 years ago

I don't believe a linter should be tied to a specific editor. Since the file being input uses tabs the number of spaces a tab uses should be irrelevant to the linter. I realize PHPCS needs that information for compatibility with some of the sniffs but no other linter requires that information in order to process a file.

All other linters (that i have checked) output the character column not taking into effect the tab width.

Atom's linter-phpcs could just calculate the correct character column but I believe it would be better for linters to have a standard and be consistent otherwise there can be issues like AtomLinter/linter-phpcs#146.

I don't believe it is unreasonable to assume that different linters have consistent line and column numbers.

gsherwood commented 8 years ago

I don't believe a linter should be tied to a specific editor.

I think that's what you're asking for given Atom is the odd one out. If the column numbers are changed to not count tabs as spaces, the other IDEs will show the wrong error position.

All other linters (that i have checked) output the character column not taking into effect the tab width.

PHPCS has always taken them into account, and is nearly 10 years old. So this doesn't seem like a good reason to change the behaviour.

UziTech commented 8 years ago

Creating a standard for linters doesn't seem like a good reason to change behavior?

I'm not saying outputting the character column is the right way, but I do believe there should be a standard and changing PHPCS's output seems easier than changing all other linters.

UziTech commented 8 years ago

Frankly I believe the best solution, although maybe impractical, would be to get rid of the option to change tabs to spaces and require sniffs that deal with tabs when in an environment that uses tabs.

gsherwood commented 8 years ago

Frankly I believe the best solution, although maybe impractical, would be to get rid of the option to change tabs to spaces and require sniffs that deal with tabs when in an environment that uses tabs.

That would require every sniff to do its own tab detection, or have a tab equivalent. The result of that would be a lack of available sniffs, and inconsistency between checks.

I strongly believe the conversion of tabs to spaces for checking and fixing is the best solution and it has worked just fine for many many years. If you believe different, you can decide to just not use that option and to code equivalent sniffs for tab checking. I'm not going to personally build and maintain a second set of sniffs for this, but it doesn't mean someone else can't.

UziTech commented 8 years ago

Every sniff wouldn't need to be rewritten just ones dealing with indentation. I have already rewritten a couple of the sniffs that I use since I use tabs.

This still does not change the fact that there should be consistency between the line and column numbers of linters.

gsherwood commented 8 years ago

Closing this as I'm not going to break BC with existing plugins by changing this. Clearly there is inconsistency in both linters and IDEs, but PHPCS has been reporting this way since late 2007, so it seems incorrect to change it now.

Arcanemagus commented 8 years ago

Just a note... the only inconsistency in linters with regards to tabs is PHPCS, of the dozens of linters I work with maintaining the AtomLinter repositories. Every other linter counts them as 1 character.

UziTech commented 8 years ago

Seems like v3.0 would be a perfect time to change this since there are already breaking changes