Branch: refs/heads/main
Revision: e66b8f53552382e34ce432668ec38d4166eb5d5a
Author: DanilaFe
Link: chplcheck: integrate misleading indentation + incorrect indentation, improve fixits. by DanilaFe · Pull Request #28634 · chapel-lang/chapel · GitHub
Log Message:
chplcheck: integrate misleading indentation + incorrect indentation, improve fixits. (#28634)
This PR seeks to make the MisleadingIndentation and
IncorrectIndentation rules play nicer together, in addition to
improving how @chplcheck.ignore works.
Previously, in a code block like this:
for 1..10 do
a();
b();
c();
Only b(); would be flagged as MisleadingIndentation, and c() would
be flagged as IncorrectIndentation. This means that fixits for
MisleadingIndentation would only appear anchored to b(), and that
@chplcheck.ignore("MIsleadingIndentation") would not prevent a warning
for c();, since it has a different source.
The general problem is that these two rules are closely related. In an
ideal world, they need to be aware of each other, which suggests
performing both computations in one pass, and specially handling nodes
that are eligible for both warnings (specifically, by disabling the more
vague IncorrectIndentation warning for those nodes). To make this
work, a secondary problem is to make MisleadingIndentation flag more
than just the first node in a misleadingly-aligned sequence. This helps
to "speak over" IncorrectIndentation properly (silencing it for all
nodes in a batch), as well as to better show the extent of the error
(and the corresponding fix) to the user.
Fusing the Traversals
In this PR, I extracted the logic for IncorrectIndentation and fused
it with the logic for MisleadingIndentation. While there, I noticed
that we don't properly handle cases where indentation goes up and down:
for 1..10 do
a();
for 1..10 do
b();
c();
d();
To fix this, I adjusted the MisleadingIndentation logic to use a
stack.
Technically, the fusion isn't necessary, but the traversals do work in a
very similar way, so this ought to save some work.
Silencing Conflicting Rules
By performing the MisleadingIndentation and IncorrectIndentation
computations together before emitting warnings, I was able to adjust the
rules to avoid firing when the sibling rule had a more precise warning
to report. This ended up needing to work both ways:
- In the original case,
for 1..10 do a(); b(); // Misleading c(); // Misleading
c() will no longer report IncorrectIndentation because it would be
properly flagged MisleadingIndentation, and because
MisleadingIndentation is more specific here.
- In an existing case,
for 1..10 do a(); // Incorrect b(); c();
MisleadingIndentation will not fire for b() or c(). This is
because a() itself is subject to IncorrectIndentation.
I'm quite happy with the results.
Exposing chplcheck state to Rules
One problem with thinking "I shouldn't warn about X because the warning
about Y will get it!" is that the rule for "Y" might be enabled. So, the
net result is that we can stop warning for "X" because the warning also
qualifies for "Y", and, if "Y" is not running, not emit any warning at
all.
I think this is undesirable, and I wanted the cross-silencing to only
occur if the sibling rule is actually enabled by the user (whether it's
ignore'd is a separate concern).
To do this, I needed the rules themselves to be aware of what other
rules are enabled. I wanted this to work by adding an extra
ChplcheckBla argument, similarly to how things work for rule-specific
settings. To do this, I adjusted the rule infrastructure to detect these
new special formals (so far, ChplcheckSilencedRules), and add
arguments corresponding to them. The final result is as follows:
MisleadingIndentation and IncorrectIndentation both enabled
for 1..10 do
a();
b(); // Misleading
c(); // Misleading
for 1..10 do
a(); // Incorrect
b();
MisleadingIndentation enabled, IncorrectIndentation disabled
for 1..10 do
a();
b(); // Misleading
c(); // Misleading
for 1..10 do
a();
b(); // Misleading
MisleadingIndentation disabled, IncorrectIndentation enabled
for 1..10 do
a();
b(); // Incorrect
c(); // Incorrect
for 1..10 do
a(); // Incorrect
b();
Fixing Auto-Fixing
My previous PR had an incorrect ordering of conditions before applying
auto-fixits: it first filtered for only semantics-preserving fixes,
then decided whether to apply them depending on how many were
non-default. However, this means that if there are two auto-fixes, one
of which preserves semantics and the other does not, the linter will
choose to pick the semantic-preserving one. I don't think this is
reasonable, since there are still two choices available, and one should
be made explicitly. I have therefore reordered the decision making. This
significantly reduced the number of places in which
MisleadingIndentation is auto-fixed, but I think this is a good thing.
Reviewed by @jabraham17 -- thanks!
Testing
-
test/chplcheck
Diff:
M test/chplcheck/MisleadingIndentation.chpl
M test/chplcheck/MisleadingIndentation.good
M test/chplcheck/MisleadingIndentation.good-fixit
M tools/chpl-language-server/src/chpl-language-server.py
M tools/chpl-language-server/src/lsp_util.py
M tools/chplcheck/src/chplcheck.py
A tools/chplcheck/src/indentation.py
M tools/chplcheck/src/lsp.py
M tools/chplcheck/src/rule_types.py
M tools/chplcheck/src/rules.py
https://github.com/chapel-lang/chapel/pull/28634.diff