Skip to content

Add IndexingOnANonIndexedSeq rule to Linter#1

Closed
JoshRosen wants to merge 1 commit intomasterfrom
indexing-on-a-non-indexed-seq
Closed

Add IndexingOnANonIndexedSeq rule to Linter#1
JoshRosen wants to merge 1 commit intomasterfrom
indexing-on-a-non-indexed-seq

Conversation

@JoshRosen
Copy link
Copy Markdown
Owner

@JoshRosen JoshRosen commented May 20, 2017

Inspired by https://twitter.com/jshrsn/status/865677863646658560, this patch adds a new IndexingOnANonIndexedSeq rule which raises a warning when we see code which indexes on a Seq which isn't known to be an IndexedSeq subtype. The goal here is to make it easier to prevent cases where someone writes an O(n^2) loop by accident.

I'm opening this PR against my own fork rather than upstream because this is still an early draft which needs self-review and testing (plus I don't have time now to get it ready for full upstream standards, such as README documentation and examples, etc.; plus it might not be as generally applicable / useful outside of Spark's needs).

This rule should have prevented the following Spark bugs:

For comparision, here is the same rule implemented in scapegoat: JoshRosen/scapegoat#1

TODOs include README updates, plus more thinking about interaction
with other existing rules.

case Apply(Select(seq, _apply), List(index))
if methodImplements(tree.symbol, SeqLikeApply)
&& !isLiteral(index)
Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

The idea behind ignoring Seq.apply(<literal>) was to prevent this rule from masking the UseHeadNotApply rule for things like x(1).

@JoshRosen
Copy link
Copy Markdown
Owner Author

JoshRosen commented May 20, 2017

Running this on Spark turned up a lot of cases. After manual inspection it looks like most are non-issues, but I may have spotted some bad ones:

Those are just the first two that jump out to me from skimming the results; there may be others which are less obvious.

@JoshRosen JoshRosen changed the title Add IndexingOnANonIndexedSeq rule Add IndexingOnANonIndexedSeq rule to Linter May 20, 2017
@JoshRosen JoshRosen closed this May 21, 2017
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.

1 participant