-
Notifications
You must be signed in to change notification settings - Fork 131
Improve linting of LHS expressions #2070
base: master
Are you sure you want to change the base?
Conversation
This reverts commit 6109ac7.
(cherry picked from commit a4e2608)
@@ -53,11 +52,9 @@ public UndefinedVariablesWalker(IDocumentAnalysis analysis, IServiceContainer se | |||
augs.Right?.Walk(new ExpressionWalker(this)); | |||
break; | |||
case AssignmentStatement asst: | |||
_suppressDiagnostics = true; | |||
foreach (var lhs in asst.Left ?? Enumerable.Empty<Expression>()) { | |||
foreach (var lhs in asst.Left.MaybeEnumerate().Where(x => !(x is NameExpression))) { |
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 affect does this have for an assignment like:
def foo():
return 1, 2
x = 1234
x, y = foo()
Where y
has not been defined but is being walked as a name expression?
I'm thinking that the original solution was "more correct" in that it's trying to avoid lvars being linted, but needed an extra case where it stored _suppressDiagnostics
and made it false before doing the first member access (to check the first thing), and then restored it.
_suppressDiagnostics = true; | ||
foreach (var lhs in asst.Left ?? Enumerable.Empty<Expression>()) { | ||
lhs?.Walk(new ExpressionWalker(this)); | ||
foreach (var l in lhs.Skip(1)) { |
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.
Not sure this is what I described, it's not about it being the first thing in a tuple, it's about it being the first part of a member or array access, like foo.bar
or foo[bar]
appearing somewhere on the left.
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.
Where foo
is undefined, but bar
is defined.
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.
Unfortunate typo, meant foo
not defined but bar
defined.
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.
That parts works OK i.e. x[y]
detects undefined x
or y
. The remaining part of LHS is trickier since we don't know if RHS returns proper number of items.
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.
I'll make a test for the case I'm thinking of and see if it works.
Seems to fail the NoRightSideCheck test. My test was something like: y = {}
invalid.access, another.access, (y.okay, fake.variable.access) = [1, 2, (3, 4)] But the current code appears to correctly only squiggle |
Fixes #2068
Original condition was too harsh. Intent was to avoid reporting of variables that are actually being defined, but that can be done by skipping over name expressions in LHS.