Rewrite SyntacticSugarRule using SwiftSyntax #3866
Conversation
PaulTaykalo
force-pushed
the
fix/critical-error-in-syntactic-sugar-rule
branch
2 times, most recently
from
f7b6423
to
70d8963
Compare
|
Looks good to me, thanks for the fix. Is there a reason this PR is marked as a draft? The Buildkite CI agent appears to be down, I’m looking into that now. I’d like to see the OSSCheck output before merging this. |
|
@PaulTaykalo OSSCheck found some regressions, can you please audit and fix them? |
PaulTaykalo
force-pushed
the
fix/critical-error-in-syntactic-sugar-rule
branch
2 times, most recently
from
c49a844
to
19fd0b6
Compare
Codecov Report
@@ Coverage Diff @@
## master #3866 +/- ##
==========================================
- Coverage 92.61% 92.55% -0.07%
==========================================
Files 436 437 +1
Lines 21964 22099 +135
==========================================
+ Hits 20343 20453 +110
- Misses 1621 1646 +25
📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more |
PaulTaykalo
force-pushed
the
fix/critical-error-in-syntactic-sugar-rule
branch
from
19fd0b6
to
df3dde4
Compare
Thanks, there's fewer regressions now but still 4 remaining. |
PaulTaykalo
force-pushed
the
fix/critical-error-in-syntactic-sugar-rule
branch
from
df3dde4
to
19b2b76
Compare
|
I'm wondering if this would be easier to write using SwiftSyntax 🤔 |
PaulTaykalo
force-pushed
the
fix/critical-error-in-syntactic-sugar-rule
branch
2 times, most recently
from
f14a46c
to
5be51c6
Compare
|
@jpsim 🤔 |
|
I recently re-added support for SwiftSyntax-based rules (e.g. #3867) and I'm wondering if it would be easier to write this rule if we used a Swift Syntax visitor rather than these challenging regular expressions. |
|
@jpsim yeah, I understood, what do you mean. I would like to try, but, first will fix this PR and then will try to use Swiftsyntax :) |
|
ok I don't want to cause more work for you, I was just suggesting an alternative if making this work with regular expressions is too hard. |
|
Well, in the general case, this kind of task is unresolvable using regular expressions. so this should fix most of the cases. but I'll check whether it is possible to use SwiftSyntax here |
|
Oh, this is slow :) |
PaulTaykalo
changed the title
|
@jpsim after few hours of debugging, decided, that it is probably, will be a better idea to use |
PaulTaykalo
force-pushed
the
fix/critical-error-in-syntactic-sugar-rule
branch
2 times, most recently
from
9150974
to
3c79fa7
Compare
|
Great progress on this. Let me know if you'd like a hand at any point. |
|
@jpsim committing between air alerts :) |
omg please stay safe ❤️ |
👍🏼 It's relatively safe here. @jpsim have a question regarding triggering/non-triggering examples. I added these as "non-triggering". are these examples fine? |
Those look perfectly fine to me. |
|
Ok then, will move on to corrections :) |
|
Sorry for the trouble, but you may need to rebase to get #3893 for Danger/OSSCheck to run on your next commit. |
PaulTaykalo
force-pushed
the
fix/critical-error-in-syntactic-sugar-rule
branch
from
d953b59
to
1cd8847
Compare
PaulTaykalo
force-pushed
the
fix/critical-error-in-syntactic-sugar-rule
branch
3 times, most recently
from
b816f92
to
ecbfd23
Compare
|
@jpsim can you look at it? In general there are two cases which changed:
|
PaulTaykalo
force-pushed
the
fix/critical-error-in-syntactic-sugar-rule
branch
3 times, most recently
from
ba4e1bf
to
4f3ed70
Compare
PaulTaykalo
force-pushed
the
fix/critical-error-in-syntactic-sugar-rule
branch
from
4f3ed70
to
65093fe
Compare
PaulTaykalo
changed the title
|
Great job here! Sorry I didn't review faster, but I'd like to suggest a few minor changes in #3915. Please review if you get a chance. |
Rewrite SyntacticSugarRule with SwiftSyntax
This adds additional checks for
syntactic_sugarruleThis fixes #3156
Changes
.selfsuffix won't be triggered, like <Optional>.self (Previously, some simple cases were handled)