Fix nested disable commands improved #5797
Conversation
Generated by 🚫 Danger |
mildm8nnered
force-pushed
the
mildm8nnered-fix-nested-disable-commands-improved
branch
2 times, most recently
from
593b3ad
to
2f6c930
Compare
mildm8nnered
force-pushed
the
mildm8nnered-fix-nested-disable-commands-improved
branch
from
4b50fd5
to
cba6a63
Compare
mildm8nnered
force-pushed
the
mildm8nnered-fix-nested-disable-commands-improved
branch
2 times, most recently
from
d44b112
to
15a4304
Compare
mildm8nnered
force-pushed
the
mildm8nnered-fix-nested-disable-commands-improved
branch
from
15a4304
to
723c11a
Compare
| @@ -145,6 +147,54 @@ private extension Rule { | |||
| ruleTime: ruleTime, | |||
| deprecatedToValidIDPairs: deprecatedToValidIDPairs) | |||
| } | |||
|
|
|||
| // Produces one region for each disabled rule identifier | |||
There was a problem hiding this comment.
Perhaps you can add some more documentation and examples here, because I'm having a hard time understanding what's happening and why.
There was a problem hiding this comment.
Let me have think about what to add. The long-winded explanation is this:
Given these rules:
"rule1": [
"regex": "pattern1"
],
"rule2": [
"regex": "pattern2"
],
and this input:
// swiftlint:disable rule1
// swiftlint:disable rule2
let pattern2 = ""
// swiftlint:enable rule2
let pattern1 = ""
// swiftlint:enable rule1
The regions of the file will be (index: start - end disabled rules)
0: 1,27 - 2,26 rule1
1: 2,27 - 4,25 rule1, rule2 rule2 is violated in this region
2: 4,26 - 6,25 rule1 rule1 is violated in this region
3: 6,26 - EOF no rules disabled
These regions are disjoint/non-overlapping. Our algorithm for detecting superfluous disable violations works fine when the disabled areas are non-overlapping, or non-nested, but in this particular nested case, we will get a false superfluous disable command for rule1 in region 0 and region 1.
decompose is perhaps not quite the right word - what we do here is rewrite the regions, so that there is a region for each rule. These regions can be overlapping. In this above case, we get:
1,27 - 6,25 rule1 rule1 is violated in this region
2,27 - 4,25 rule2 rule2 is violated in this region
6,26 - EOF no rules disabled
So we correctly get no superfluous disable command violations in this case. I didn't want to change how we represent regions elsewhere, so we just convert them here for this special case.
There was a problem hiding this comment.
Okay. Got that. I wonder if we should then generally treat regions like that right from the beginning.
I didn't want to change how we represent regions elsewhere, so we just convert them here for this special case.
Understandable. We should keep that in mind, however. Looks like we can simplify a few things in this area.
mildm8nnered
deleted the
mildm8nnered-fix-nested-disable-commands-improved
branch
|
@SimplyDanny can we create a new (bugfix) release now that this is merged? We use https://github.com/lukepistrol/SwiftLintPlugin, which bundles a precompiled version of Swiftlint via SPM to speed up builds. The plugin repo mirrors the releases from this repo and automatically creates a precompiled version via GitHub actions. This would give everyone the benefits of this PR with fast builds. |
Addresses #5788
The problem is that Regions are created with all of the rule identifiers that are disabled in them, with a new region being created for each
swiftlint:command, more or less.So an inner region may have multiple rules disabled, when disabled commands are nested, but not all of those rules might be violated within that specific region.
I guess people don't actually nest commands much in practice, at least for custom rules, or this would have created more noise.
This was not as hard to fix as I was afraid it might have been.
The basic idea is to remap the regions inside
superfluousDisableCommandViolations, so that the new regions describe the specific region that an individual rule is disabled within.