Close instances of dart.core.Sink #241
Conversation
@alexeieleusis : are you still blocked here? |
|
@pq yes I am,, I think the problem is that the code test has an import and that code is not available/parsed when the test is running, hopefully is just a matter of running the test the right way or changing the test type. I uploaded a new commit rebased on the PR merged this morning. |
|
Thank you! |
|
I should also say, I sent #242 which refactors code in this PR just to provide visibility in the upcoming changes. |
|
Could you paste the rest of the stack trace? I'm curious where the exception is originating. I'll try and repro later but in the meantime maybe something will jump out at a quick glance. |
|
|
Thanks! This is interesting:
On a quick glance, I see |
|
Curious on why it does not happen when running the linter itself? I thought it was because dart.io is available there. Do you think this is something the rule should guard against? |
|
OK, so that is odd. Really not sure what's happening... I'll try and look later. |
|
Pushed a new commit with a similar approach to the one in 8fa9ddd. I think this is now in a state where we can start discussing the rule and tests. |
|
|
||
| const _details = r''' | ||
|
|
||
| **DO** Invoke `close` on instances of `dart.core.Sink` to avoid memory leaks and |
|
I have concerns about false positives from this lint. If a class has a Sink and uses a local variable (of type Sink) in a method that creates the sink, then the local variable will be flagged, even though the sink might well be closed in some other method. If a class takes a Sink in the constructor because the Sink is owned by some other object and only used by that class, then the field will be flagged even though the class shouldn't close the sink. You can reduce the number of false positives with some heuristics. For example, if a Sink is created in and returned from a method then it clearly shouldn't be closed in that method. Of if a field is initialized to an instance that might have come from outside the class then it probably shouldn't be closed in that class. Of course, the only way to be really accurate with a rule like this is to do some data flow analysis, but that's more work that I would recommend at this point. As a fun challenge, try thinking of some valid code patterns that this rule would currently flag and see whether you can think of heuristics that would avoid the false positives. |
|
I think most of the false positives are discarded now, still need to address some of the previous comments, but I think this is looking better now. |
|
I think all comments are addressed now :) |
|
lgtm |
First attempt, I am facing a problem I don't know how to solve, which is I can't run the test, I get the following:
If I instead run the linter directly, I get the expected lints
dart-lang/sdk#57281