Skip to content
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

Handle assignment to shared object in Lint/UselessSetterCall #7309

Conversation

jonas054
Copy link
Collaborator

When a local variable is assigned to a shared object, the referenced object is effectively no longer local, so calling a setter method on it might very well be useful and should not be reported as an offense.

hmarr and others added 2 commits August 24, 2019 09:38
There are several cases that will trigger Lint/UselessSetterCall when
mutating a non-local object.
When a local variable is assigned to a shared object, the referenced object is
effectively no longer local, so calling a setter method on it might very well
be useful and should not be reported as an offense.
@bbatsov
Copy link
Collaborator

bbatsov commented Aug 28, 2019

What's the high level idea to discern a non-local object from a local one? Looking at the tests - those examples could have just as easily been non-shared objects as well.

@jonas054
Copy link
Collaborator Author

jonas054 commented Aug 29, 2019

The idea is that setting something inside a method-local object is useless but if the object can be accessed from outside the method, then the setter call might be useful and shouldn't be reported.

those examples could have just as easily been non-shared objects as well

We can only detect that some_lvar refers to an object that's accessible from outside the method. Whether the object is actually being accessed is unknown, so there could be false negatives. I'm not sure if that's what you meant, @bbatsov.

@jonas054
Copy link
Collaborator Author

jonas054 commented Sep 7, 2019

@bbatsov Did that answer your question? I tried to clarify, but maybe I'm missing something.

@bbatsov
Copy link
Collaborator

bbatsov commented Sep 7, 2019

We can only detect that some_lvar refers to an object that's accessible from outside the method.

I don't think we can actually detect this - we can only assume that it's an object that's accessible outside the method. After all you can't know if a method returns some shared object or something else, right?

@jonas054
Copy link
Collaborator Author

jonas054 commented Sep 8, 2019

I see what you mean now, and you're right. We're can't know if it's a shared object. But it could be and we want to err on the side of caution and avoid false positives at all costs.

So I want to keep the logic as it is but maybe clarify in the code about the possibly shared objects?

@bbatsov
Copy link
Collaborator

bbatsov commented Dec 30, 2019

I honestly don't know. I'm leaning towards just ignoring that problem and leaving the code as it is. I don't think that many people ran into that particular issue anyways. Perhaps we should just document the shortcomings of the cop better?

jonas054 added a commit to jonas054/rubocop that referenced this pull request Feb 9, 2020
This closes rubocop#7309, which was an attempt to fix false positives with code. It
was deemed too complex, so we're adding documentation for a, hopefully, very
rare problem.
jonas054 added a commit to jonas054/rubocop that referenced this pull request Feb 10, 2020
This closes rubocop#7309, which was an attempt to fix false positives with code. It
was deemed too complex, so we're adding documentation for a, hopefully, very
rare problem.

The cop configuration now marks the cop as "not safe".
bbatsov pushed a commit that referenced this pull request Feb 12, 2020
This closes #7309, which was an attempt to fix false positives with code. It
was deemed too complex, so we're adding documentation for a, hopefully, very
rare problem.

The cop configuration now marks the cop as "not safe".
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants