Added autofix function for imports-on-top rule + Tests for this feature

[GabrielAlacchi]

This is in regards to issue #94. There are still many other autofix functions left to write. I'm more than willing to help with them in the coming days after my exams are finished.

My implementation does the following:
When the linter encounters the first invalid import statement, it finds all proceeding import statements and combines them into one block. Then inserts that in one of three places:

1. Below the last valid import statement.
2. Below the pragma solidity ^0.4.0; statement.
3. The very top of the file.

Afterwards, any other import statement is just simply removed.

I had to combine everything and add it to the top of the file in one step so that there was no issue with two fixes overlapping. However one thing that concerns me is the case where another rule has a fix that sets the cursor beyond the top of the file. Then what will happen is the copying of import statements to the top of the file will not be run, but every import statement that follows will still be removed, thus completely defacing the user's code. Maybe I've misunderstood the fixing engine, but if you think that this is a valid concern then I can develop a use case that will execute this bug and open an issue surrounding it.

[duaraghav8]

Thanks @GabrielAlacchi, currently reviewing this PR.
We welcome more contributions from you, you can reach me on Gitter in case you have any doubts, would be happy to clarify!

[duaraghav8]

@GabrielAlacchi Don't worry about cursor issues. If you supply a fix and Solium ensures that this fix doesn't conflict with any other rule fixes, then the fix is guaranteed to be applied just the way you intended.

(Correct me if I'm wrong below)
I've understood your approach. Although very elegant, I'd be more inclined toward a different one. I see several drawbacks of this one:

• The code is much longer
• Flow is more complex and so maintaining it over time will be difficult
• // All future import statements' fixes will involve just simply removing the node - Generally in a rule implementation, relying on future iterations of the rule is a bad practice because although it achieves the objective, it introduces bugs in unexpected places that are extremely hard to detect. Believe me, I've already been through a nightmare!
• combinedImportFix - maintaining a global variable is also a bad idea, because we don't know how they might affect other iterations of the rule. So we'll avoid it bc here we can.

^ Given all this, I see the end result you created for the user - cleanly organised import statements.

I'd like to suggest a shorter fix - just follow the comments I put, ie, to

• remove the import node that was detected to be in wrong position, and
• add it 1 line above chidNode.start. This guarantees that nothing can go wrong although its not as clean a result as your current approach. this code will be max 4 lines and therefore much more maintainable. We can improve the user experience over time.

But I can be wrong, so I'm still open to hearing you in case you feel your approach is better

[GabrielAlacchi]

@duaraghav8 I agree with you on the fact that relying on other iterations of the rule is a bad idea in general. My original idea was to just insert the import below the last valid import node, or else below the pragma solidity line. The issue was that because the fixes were all centered around the same node, there were conflicts and only the first import fix was ever applied. Perhaps there's a better way to insert them all below the same node without any conflicts and without relying on any kind of global state. In that case the most ideal place to insert the statement can be calculated while looping through program.body and thus we'd eliminate the need for any global variables.

I think your solution is fine for temporarily solving this problem. One issue I can see is that the fix may conflict with the blank-lines rule if it's not implemented carefully, consider this case:

pragma solidity ^0.4.0;


contract Foo { }

import "file.sol";

The rule will see that when it reaches the node corresponding to contract Foo that the import invalid, and thus insert the import above it as such:

pragma solidity ^0.4.0;


import "file.sol";
contract Foo { }

However this contradicts the rule that contracts should be preceded by 2 blank lines, and thus the autofix would create code that fails the linting process. This can be solved by including some extra cases when inserting the import statement before childNode, but it's definitely something to be wary of in general. Are there better ways of dealing with potential conflicts between rules when autofixing?

[duaraghav8]

I fully agree with the modified approach you've described above (could you please make the necessary changes in PR?)
I knew that once enough rules supplied the fix(), there would be conflicts. Solium is written in a way that a rule implementer need not worry about these conflicts.

I'll make the changes so Solium so it runs multiple iterations over a file instead of single. So in first iteration, import statements are fixed as intended by you, but they cause a conflict and solium reports it. But in the next iter. Solium will fix the conflict arising from the first iter. (once the blank-lines rule supplies its own fix, otherwise solium will simply report the error).
5-10 such iterations will be enough to ensure that the linter has fixed as much as it could.
So you don't need to add extra newlines to ensure compliance with another rule, it becomes tedious when you have to fix while obeying every other linter rule.

Bottom line: you need not worry about conflicts your rule's fix might create with another rule's checks. That's Solium's problem

On a side note, please keep in mind that there's also an experimental pragma statement (eg- pragma experimental ABIEncoderV2;). The imports should be below both kinds of pragma statements.

[GabrielAlacchi]

I've implemented the insert after last valid node strategy that we've agreed on in my latest commit, and have modified the unit tests to reflect this change. I also simplified the loop over program.body to use slice(0, indexOfNode) for looping over all preceding nodes rather than doing:

if (childNode.start === node.start) {
    return;
}

[duaraghav8]

Thanks @GabrielAlacchi 🙌
happy new year!

[GabrielAlacchi]

Cheers @duaraghav8 and Happy New Year to you too! I have pushed some commits for autofixing the blank-lines rule on the dev branch of my fork
https://github.com/gabrielalacchi/Solium/tree/dev

Should I open a PR or wait to complete some of the others first?

[duaraghav8]

great! Yes, please open a PR. Its easier if we only have 1 thing to focus on in 1 PR