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

Add a new unnecessary_late lint #3052

Merged
merged 4 commits into from
Dec 8, 2021

Conversation

parlough
Copy link
Member

@parlough parlough commented Nov 2, 2021

Adds a new unnecessary_late lint which will detect usages of the late modifier late on static and top-level variables which are evaluated as if they are marked with late already.

I believe there are other situations where late may be unnecessary. The first that comes to mind is in a local variable declaration where the initializer will have no side effects. I have opted to not account for this and potential other more complicated situations currently(unless I'm missing something obvious). I was thinking other situations could be incorporated into this lint in the future, but if you think this should have a more specific name such unnecessary_static_late, let me know :)

@google-cla google-cla bot added the cla: yes label Nov 2, 2021
@coveralls
Copy link

coveralls commented Nov 2, 2021

Coverage Status

Coverage increased (+0.01%) to 94.162% when pulling 9e07e8e on parlough:feature/unnecessary-late-lint into 2e1800e on dart-lang:master.

@pq
Copy link
Member

pq commented Nov 2, 2021

/fyi @jcollins-g @stereotype441

lib/src/rules/unnecessary_late.dart Outdated Show resolved Hide resolved
test_data/rules/unnecessary_late.dart Outdated Show resolved Hide resolved
Copy link
Member

@pq pq left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💎

@pq
Copy link
Member

pq commented Nov 11, 2021

I believe there are other situations where late may be unnecessary.

Right. Many seem to be harder to reliably check though making them not good candidates for a general lint. My thinking there is that if there are such cases we may want to lint specifically for them. Anyway, this is just a long way around to saying I think that unnecessary_late is a reasonable name and I say we go for it.

I'll add a few more folks to get their 2 cents, especially since I think this is a good candidate for inclusion in the core or recommended lint sets.

/fyi 📟 @jakemac53 @natebosch @munificent @lrhn @eernstg @leafpetersen

Thanks as always @parlough for the contribution!

@lrhn
Copy link
Member

lrhn commented Nov 12, 2021

I think detecting a local late variable with an initializer not having a side effect, is going to be impossible. Also, I'd imagine using late to delay allocation, which is usually not considered a side-effect.

One other unnecessary late use is a local variable which is definitely assigned at all reads, and either not final or definitely unassigned at all writes. Not everybody is aware that you can declare an final int x; without an initializer, as long as you definitely initialize it once before any read.

@parlough
Copy link
Member Author

@bwilkerson Is this above currently feasible within the linter? If I'm understanding correctly, I believe this would require some sort of access to flow analysis, but I'm not sure what is exposed there.

@bwilkerson
Copy link
Member

My current understanding, though I'm not very familiar with that code, is that it would be possible, but a fair amount of work, to use the flow analysis code to test these conditions. The flow analysis support isn't part of the public API, so we'd probably want to add an API to LinterContext, and we'd have to be careful about how we designed the API so that the checks would be performant.

It would be interesting to know how often local variables are marked late unnecessarily as a measure of whether it would be worth the effort to implement such support.

@pq
Copy link
Member

pq commented Dec 8, 2021

Sorry for the lag on this.

@parlough: any objection to landing as-is?

@parlough
Copy link
Member Author

parlough commented Dec 8, 2021

@pq Yeah I'm happy with this lint in the current state. I won't have time to investigate exposing and accessing the flow analysis for now.

@pq
Copy link
Member

pq commented Dec 8, 2021

Good deal. Thanks!

@pq pq merged commit a93740e into dart-lang:master Dec 8, 2021
mockturtl added a commit to mockturtl/tidy that referenced this pull request Dec 19, 2021
copybara-service bot pushed a commit to dart-lang/sdk that referenced this pull request Aug 23, 2023
* Add a new unnecessary_late lint

* Sort lint implementation Dart file

* Test non-late static and top-level declarations

* Provide better matching examples in details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants