Linter: Implement erb-no-trailing-whitespace rule

[markokajzer]

closes #547

Implements the TrailingWhitespace rule from ERBLint.

This rule disallows whitespace (spaces, tabs, carriage return) at the end of a line.

Unfamiliar with the project, I took inspiration from #557, which implemented a similar rule.

[marcoroth]

Hey @markokajzer, thanks for taking a stab at this!

This looks great already and is a really accurate port of the linter rule as it was at play in erb_lint.

However, I have a slight concern with autofix for this rule. I feel like we need to be a bit more careful and cannot just strip whitespace at the end of each line, as some of the whitespace might be relevant.

So I feel like we might have to change this linter rule to be a ParserRule (opposed to the SourceRule we have now) and skip contexts like <pre> and <textarea> . Maybe even <script> and <style> too.

But I also realize that the implementation of this would become a lot trickier when we have to operate on AST nodes for a rule like this.

Do you have any thoughts on this? Thank you!

[markokajzer]

I actually fear that might exceed my skills lol

How would be go about this? Traverse all nodes, and if we're at the end of the line, and not within one of the mentioned nodes, delete the whitespace? (Haven't looked at ParserRule yet: does a node know where we are in a line?)

On the other hand, does erb_lint handle those correctly? If not, people using erb_lint should already be familiar with such intricacies?

[marcoroth]

How would be go about this? Traverse all nodes, and if we're at the end of the line, and not within one of the mentioned nodes, delete the whitespace? (Haven't looked at ParserRule yet: does a node know where we are in a line?)

Yeah, sadly that's the super tricky part here. When we are operating on AST Nodes we don't really think in terms of lines anymore. But, the AST Nodes would have all required information on them to be able to figure that out again and map it back. It probably would just mean a lot of new rule helpers to make it easier to work with lines in a ParserRule. But yeah, this might be a bit overkill for now.

That said, you are right that this is already a problem in erb_lint today. But given that we also support an autofix with this rule now, this will automatically be fixed in the Language Server if you open a file with trailing whitespace and save it.

What I would recommend is to tell the rule that it's not auto-correctable set static autocorrectable = false and leave the current implementation as-is.

[markokajzer]

static autocorrectable = false

lower severity to warning in the same vein?

[marcoroth]

Hey @markokajzer, sorry for pushing up directly to your branch.

I addressed the concerns by converting the rule from a SourceRule to a ParserRule with "smart skip zones".

The detection now works in multiple steps: First, we split the source into lines and scan each line for trailing whitespace using a regex, this is pretty much what we had before, but this gives us a list of trailing whitespace candidates with their line and column positions.

Next, we traverse the AST using a visitor once to collect the "skip zones". They are regions where trailing whitespace should be preserved. This includes the <pre>, <textarea>, <script>, <style> elements, and all ERB tags. ERB tags are skipped because trailing whitespace inside Ruby code could be significant (heredocs, string literals, etc.). Maybe we can be smarter about this in the future and also enable it for ERB tags.

Finally, we filter the candidates against the skip zones. Only candidates that fall outside all skip zones become offenses. This approach gives us the simplicity of the source-based detection we had before, but with the accurate context-awareness of AST-based filtering.

Since we now skip sensitive contexts, autofix is safe to enable again. I added some more tests to make sure we cover all of the tricky edge cases.

Thanks for starting this and opening a pull request! 🙏🏼

[markokajzer]

Glad we’ll be able to merge something, even if you had to do all the work after all :^)

I see you have a high standard for this project haha

Might try again on another rule 👍

[marcoroth]

Sorry to have hijacked it 🙈

But please don't be discouraged to tackle another rule, I'm happy to help steer you the right way. For example both #1157 and #1158 should be a lot more straightforward, as they are about detecting the presence of a certain node type.