Thanks for the PR, @PSeitz!

typescript-eslint is a 100% community driven project, and we are incredibly grateful that you are contributing to that community.

The core maintainers work on this in their personal time, so please understand that it may not be possible for them to review your work immediately.

Thanks again!

🙏 Please, if you or your company is finding typescript-eslint valuable, help us sustain the project by sponsoring it transparently on https://opencollective.com/typescript-eslint. As a thank you, your profile/company logo will be added to our main README which receives thousands of unique visitors per day.

I added more tests for better coverage, but the voter is not updated? Trying to close and open

so I use the check in 252 with return; as a cast on the type and this line is expected to never hit

          if (
            !indexPosVar ||
            !isZeroInitialized(indexPosVar) ||
            indexPosVar.id.type !== AST_NODE_TYPES.Identifier ||
            arraLengthVar.id.type !== AST_NODE_TYPES.Identifier
          ) {
            return;
          }

Codecov Report

Merging #1617 into master will decrease coverage by 0.26%.
The diff coverage is 79.38%.

@@            Coverage Diff             @@
##           master    #1617      +/-   ##
==========================================
- Coverage   95.20%   94.94%   -0.27%     
==========================================
  Files         148      158      +10     
  Lines        6989     7100     +111     
  Branches     2017     2034      +17     
==========================================
+ Hits         6654     6741      +87     
- Misses        124      155      +31     
+ Partials      211      204       -7     

@bradzacher should I also create an issue for this enhancement?

Could you please explain the motivation behind this change?
why do you assign the length as a loop variable when it's not being incremented?

The examples you provided don't shed much light on what the programming pattern is exactly. Do you have some real world code which might illustrate the intention?

My concern is that this is use case is very, very rare, and in contrast the code required to support it makes it harder to maintain.

@bradzacher

why do you assign the length as a loop variable when it's not being incremented?

I don't think it makes sense, but I see this quite often in a codebase I'm working on. I think it's bad style, since it impairs readability. I guess some people do this for performance reasons. In this case the developers are coming from java.

http://allthingsjavascript.com/blog/index.php/2016/11/16/caching-the-length-property-in-a-for-loop/

https://stackoverflow.com/questions/5752906/is-reading-the-length-property-of-an-array-really-that-expensive-an-operation

Assuming the ts ast is rather stable, I don't think its hard to maintain, because the implementation is disjunct from the original, so you can imo just leave this case as is and not integrate changes.

I don't think its hard to maintain, because the implementation is disjunct from the original, so you can imo just leave this case as is and not integrate changes

Sorry, when I say maintenance, I don't just mean "keeping the code running as we upgrade versions", I also mean enhancing it with new features, and fixing bugs.

This PR adds ~100 lines of code, which adds ~50% to the rule, which is non-trivial. This adds maintenance overhead in two ways:

• more code is harder to understand for contributors, which raises the bar for contributions.
• if someone wants to add a new feature they will have to understand this code to decide if it needs to be enhanced as well.
  • if they do need to enhance it, that's more cases to think about.
• more code is more branches that need to be tested with those changes.

With all the above, if someone else wants to make an update or fix to the rule, then they can easily be scared off by the complexity. We already have too few contributors, so we don't want to scare anyone off.

Additionally, if no contributors want to fix bugs, then it falls on the core maintainers to fix bugs (which likely won't happen as we are pretty overloaded as is).

All of this is why I'm careful to validate that a change provide more use beyond a single codebase.

@bradzacher Yes, it's hard to understand. I don't think both cases can be merged into one block. I could offer to add comments explaining all the steps. As for the tests, I think the new code is quite well covered.

Closing as there isn't any drive from the community for such a feature, so IMO it's not worth the additional maintenance overhead.

Thank you for your contribution!