-
Notifications
You must be signed in to change notification settings - Fork 39
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
NPE in NewlinesRule: #1050
NPE in NewlinesRule: #1050
Conversation
### What's done: * fixed bug in NewlinesRule Closes #1047
2dfeaab
to
c16fbfe
Compare
Codecov Report
@@ Coverage Diff @@
## master #1050 +/- ##
============================================
+ Coverage 83.40% 83.43% +0.02%
Complexity 2447 2447
============================================
Files 102 102
Lines 6122 6120 -2
Branches 1833 1832 -1
============================================
Hits 5106 5106
+ Misses 273 272 -1
+ Partials 743 742 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
lintMethod( | ||
""" | ||
|fun bar(): String { | ||
| val a = it.responseBody!!.name |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code basically didn't trigger an NPE before
| val a = it.responseBody!!.name | |
| val a = it.responseBody!![0].name |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. I removed this for a test and forgot to return
@@ -461,7 +462,7 @@ class NewlinesRule(configRules: List<RulesConfig>) : DiktatRule( | |||
|
|||
if (psi.children.isNotEmpty() && !psi.isFirstChildElementType(DOT_QUALIFIED_EXPRESSION) && | |||
!psi.isFirstChildElementType(SAFE_ACCESS_EXPRESSION)) { | |||
val firstChild = psi.firstChild | |||
val firstChild = if (psi.isFirstChildElementType(ARRAY_ACCESS_EXPRESSION)) psi.firstChild.firstChild else psi.firstChild |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is that just workaround?
I looked at this bug before and think, that the main problem is in these lines:
For now I didn't catch the logic of this if
statement - in which cases do we need this, even more it doesn't covered by any test.
Probably, we should modify it or get rid of it at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I deleted this piece and. everything continued to work
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it should work, however did you understand the reason, why did we need this before, for which cases?
I can't imagine any other examples for now, which will satisfy this condition, instead of it.responseBody!![0].name
, for which it doesn't work
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't get how it should fix anything?
If we had an NPE on the further manipulations val firstChild
or psi.firstChild
, you should do null-checks and create special logic for it. Not simply hardcode ARRAY_ACCESS_EXPRESSION
### What's done: * fixed bug in NewlinesRule Closes #1047
@@ -527,6 +527,18 @@ class NewlinesRuleWarnTest : LintTestBase(::NewlinesRule) { | |||
) | |||
} | |||
|
|||
@Test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
create fixer tests (where WRONG_NEWLINES
is fixing the code in such case) to prove that you are right
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Cheshiriks where are tests for fixers?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add more tests, make the logic at least look less hardcoded (create null-checks and describe your workarounds)
""" | ||
|fun bar(): String { | ||
| val a = it.responseBody!![0].name | ||
|} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please add positive scenarios (that will trigger warnings)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
### What's done: * fixed bug in NewlinesRule Closes #1047
What's done:
Closes NPE in NewlinesRule #1047