Add WartRemover checks

[EzequielPostan]

Would it be a good idea to add checks using wartremover?

https://github.com/wartremover/wartremover

I have seen some good checks like

• Remove unsafe calls like head, last, get, etc.
• Remove vars and mutable state
• Detect Product type inferences
• Remove the use of Any

and many others.
We could define a full list of desired checks from here: http://www.wartremover.org/doc/warts.html

[ceilican]

I think this is generally a good idea, although I don't agree with a few built-in warts or with the suggested ways to remove some warts.

[greenhat]

I found wartremover super useful. @EzequielPostan nice list, Recursion wart deserves a mention too. It forces you to think about stack safety in every recursion.

[EzequielPostan]

Thanks for the comment @greenhat , I will go back to work on this issue soon.

@ceilican
We can select warts we would like to adopt and ignore the rest. I also don't like some of the suggested ways to remove some warts, but we can apply our own style.
The good thing is that breaking the wart rules won't let you compile, so it enforces good practices.

I will enable some warts in a new branch and check how many issues it detects.

[EzequielPostan]

@ceilican
I enabled TraversableOps in a local branch and the plugin found the following error cases:

sbt:scorex-core> test:compile
···
[error] .../src/main/scala/scorex/core/api/http/CompositeHttpService.scala:21:44: [wartremover:TraversableOps] reduce is disabled - use reduceOption or fold instead
[error]   val compositeRoute = routes.map(_.route).reduce(_ ~ _) ~ corsHandler(swaggerService.route) ~
[error]                                            ^
[error] .../src/main/scala/scorex/core/consensus/BlockChain.scala:39:36: [wartremover:TraversableOps] head is disabled - use headOption instead
[error]   def lastBlock: B = lastBlocks(1).head
[error]                                    ^
[error] .../src/main/scala/scorex/core/consensus/BlockChain.scala:50:29: [wartremover:TraversableOps] head is disabled - use headOption instead
[error]     val modId = openSurface.head._1
[error]                             ^
[error] .../src/main/scala/scorex/core/consensus/BlockChain.scala:51:37: [wartremover:TraversableOps] head is disabled - use headOption instead
[error]     val s = lookForward(openSurface.head._2, size)
[error]                                     ^
[error] .../src/main/scala/scorex/core/network/NodeViewSynchronizer.scala:305:33: [wartremover:TraversableOps] head is disabled - use headOption instead
[error]         val modType = modifiers.head.modifierTypeId
[error]                                 ^
[error] .../src/main/scala/scorex/core/network/message/BasicMessagesRepo.scala:114:7: [wartremover:TraversableOps] reduce is disabled - use reduceOption or fold instead
[error]     }.reduce(_ ++ _)
[error]       ^
[error] .../src/main/scala/scorex/core/utils/utils.scala:59:38: [wartremover:TraversableOps] head is disabled - use headOption instead
[error]   else concatFixLengthBytes(seq, seq.head.length)
[error]                                      ^
[error] 7 errors found

which reveals that some unsafe calls are made and probably should be reviewed.
Would you like me to enable some Warts and correct them as I can? It may be good to create a separate issues for each Wart we decide to enable.
Should I proceed in this path? If yes, which Warts would you suggest to enforce?

[EzequielPostan]

Let me correct myself, there are a lot more of unsafe calls. The ones above are just from scorex-core

[EzequielPostan]

@ceilican @greenhat
As pointed above, I created a PR to discuss the application of TraversableOps

[ceilican]

Thanks, @EzequielPostan ! I left some comments there. My comments clarify what I meant when I wrote above that:

I think this is generally a good idea, although I don't agree with a few built-in warts or with the suggested ways to remove some warts.

In my opinion, we should only avoid using head, tail and other methods that may throw an exception if we are using them in a buggy way (e.g. when the case that may trigger the exception can actually occur and is not being handled in some way). It is better to have exceptions than to have possibly wrong behaviours for those unexpected cases.

[EzequielPostan]

Thanks for the review @ceilican :)

I understand and share the objection when the error pointed by the Wart is:

1. Not real based on code logic, such us the case of using tail after checking
   the length of a sequence is bigger than 1 for example.
2. Just adds verbosity, such as the case of solving a possible exception inside
   an ensuring clause that would throw an exception anyway.

I neither like the use of drop(1) instead of tail. I also prefer to leave those cases as tail and see an explicit exception over an unexpected behaviour. However, I still prefer a more solid code logic, i.e. consider the empty case in the code over letting the exception arise.

I am not sure how should I proceed with that PR, any suggestion is welcome.
I can try to apply another wart.

[ceilican]

I can try to apply another wart.

I think we should have a separate PR for each wart type.

[ceilican]

I am not sure how should I proceed with that PR, any suggestion is welcome.

I propose that you modify the PR. For each wart occurrence found by the wart remover, do one of the following:

1. if the wart is a bug (e.g. the code is not handling the case of an empty traversable as it should) and it is clear how to fix the bug, fix the bug.

2. if the wart is not a bug (e.g. the code doesn't handle the case of an empty traversable, but this case never happens, or the exceptional case is already handled in ways that WartRemover doesn't understand (such as surrounding the code with Try, ensuring, catch...)), leave the code as it is and switch off WartRemover on that piece of code. Optionally, do one of the following:

   • add a comment explaining why the wart is not a bug.
   • refactor the code removing the wart, but preserving the behaviour, if the refactored code without the wart is clearly more concise and easier to understand (for programmers in general) than with the wart.

3. if you are unsure whether the wart is a bug, switch off WartRemover around that wart and add a // fixme comment explaining why it might be a bug.

This is already almost what you did.

What do you think of this proposal?

[ceilican]

@kushti @greenhat , what do you think about the proposal above?

[EzequielPostan]

I agree with the suggestions of creating different PRs for each Wart and also on how to proceed.
As I am still not that familiar with the code base, I guess I will "fix" few of the warts, but I can leave a trace of the problematic cases as you mention.
Other warts may be "easier" to apply.

[greenhat]

@ceilican I agree. And it might be a good idea to create ticket(s) for "warts to enable" list.

[EzequielPostan]

I created a PR to enable the wart Recursion as it was really simple to apply #194 .
I will go back to the TraversableOps during the weekend.
Is it ok if I try to apply other Warts? Creating a list of Warts we agree on applying would be good as suggested above.

[ceilican]

Is it ok if I try to apply other Warts?

I think so. Which other warts would you like to try next?

[EzequielPostan]

Var and Null tend to be good Warts.
Null is easy to apply (there are only two uses of null in the whole project)
I haven't checked Var (eliminates the use of var) but it may be hard to apply.

I once found a tricky bug thanks to Product.
Any is also a good wart, but haven't checked its application either.
OptionPartial: replace get for fold in type Option

I think PublicInference could be good as I see a lack of type declarations in some places (but not sure if this happens with public members).

Then, other possible warts could be:

• DefaultArguments: To eliminate default arguments
• ToString: Scala creates a toString method automatically for all classes. Since toString is based on the class name, any rename can potentially introduce bugs. This is especially pernicious for case objects. toString should be explicitly overridden wherever used.
• FinalCaseClass: Scala’s case classes provide a useful implementation of logicless data types. Extending a case class can break this functionality in surprising ways. This can be avoided by always making them final or sealed.
• FinalVal: Value of a final val is inlined and can cause inconsistency during incremental compilation (see Name hashing: Members with constant types are not tracked sbt/sbt#1543)
• ImplicitConversion: Implicit conversions weaken type safety and always can be replaced by explicit conversions.
• IsInstanceOf: isInstanceOf violates parametricity. Refactor so that the type is established statically.
• JavaConversions: The standard library provides implicits conversions to and from Java types in scala.collection.JavaConversions. This can make code difficult to understand and read about. The explicit conversions provided by scala.collection.JavaConverters should be used instead.
• JavaSerializable: java.io.Serializable is a common subtype to many structures, especially those imported from Java. For example, String is a subtype of java.io.Serializable but not scala.Serializable. The Scala compiler loves to infer java.io.Serializable as a common supertype, but that is almost always incorrect. Explicit type arguments should be used instead.
• Serializable: Serializable is a type common to many structures. The Scala compiler loves to infer Serializable as a generic type, but that is almost always incorrect. Explicit type arguments should be used instead.
• LeakingSealed: Descendants of a sealed type must be final or sealed. Otherwise this type can be extended in another file through its descendant.
• MutableDataStructures: The standard library provides mutable collections. Mutation breaks equational reasoning.

As you see, there are many warts to examine.
I think that most of them can be applied in a single PR because they may not involve many changes (for example: Null, Product, FinalVal, JavaConversions, JavaSerializable and Serializable could probably be applied in a single PR).
Other warts would need special care in separate PRs (Any, Var, MutableDataStructures).

MutableDataStructures could be related to #78

Enabling a Wart also imposes a sort of style standard. Hence, I wouldn't like to add warts without a certain level of consensus. Please let me know which ones you prefer me to work on (if any) and which to ignore (if any).

Thank you all for your time to read/review this.

[ceilican]

Here are my comments:

Null is easy to apply (there are only two uses of null in the whole project)

Yes, I totally agree with removing null.

I haven't checked Var (eliminates the use of var) but it may be hard to apply.

Partially agree. We definitely should avoid global var. Locally, if var can be removed without compromising code readability, I'm in favour of it. Occasionally, though, a local use of var allows things to be achieved more easily and more clearly. In such cases, I think we should keep using var instead of masochistically doing some immutable contortions... :-)

I once found a tricky bug thanks to Product.

I think it is a good one.

Any is also a good wart, but haven't checked its application either.

I think it is a good one too.

OptionPartial: replace get for fold in type Option

I partially agree. As with head and tail (discussed above), sometimes it is ok to use get.

I think PublicInference could be good as I see a lack of type declarations in some places (but not sure if this happens with public members).

IntelliJ already warns to declare types of public members. Therefore, at least for parts of the code that I have touched, this won't make a difference. But I think it is a good one.

• DefaultArguments: To eliminate default arguments

I completely disagree with this one. Default arguments are a very nice feature.

• ToString: Scala creates a toString method automatically for all classes. Since toString is based on the class name, any rename can potentially introduce bugs. This is especially pernicious for case objects. toString should be explicitly overridden wherever used.

I'm against this one. I never had a problem with "toString", and being forced to override "toString" to use it would make the code unnecessarily more verbose.

• FinalCaseClass: Scala’s case classes provide a useful implementation of logicless data types. Extending a case class can break this functionality in surprising ways. This can be avoided by always making them final or sealed.

Isn't "case class inheritance" already prohibited in recent versions of Scala anyway?

• FinalVal: Value of a final val is inlined and can cause inconsistency during incremental compilation (see Name hashing: Members with constant types are not tracked sbt/sbt#1543)

Not sure. I think it wouldn't make a difference anyway.

• ImplicitConversion: Implicit conversions weaken type safety and always can be replaced by explicit conversions.

I'm against this one. Implicit conversions are a powerful feature that must be used with responsibility, not forbidden.

• IsInstanceOf: isInstanceOf violates parametricity. Refactor so that the type is established statically.

This is a good one.

• JavaConversions: The standard library provides implicits conversions to and from Java types in scala.collection.JavaConversions. This can make code difficult to understand and read about. The explicit conversions provided by scala.collection.JavaConverters should be used instead.

This seems ok to me.

• JavaSerializable: java.io.Serializable is a common subtype to many structures, especially those imported from Java. For example, String is a subtype of java.io.Serializable but not scala.Serializable. The Scala compiler loves to infer java.io.Serializable as a common supertype, but that is almost always incorrect. Explicit type arguments should be used instead.

Seems ok too.

• Serializable: Serializable is a type common to many structures. The Scala compiler loves to infer Serializable as a generic type, but that is almost always incorrect. Explicit type arguments should be used instead.

Seems ok.

• LeakingSealed: Descendants of a sealed type must be final or sealed. Otherwise this type can be extended in another file through its descendant.

Good.

• MutableDataStructures: The standard library provides mutable collections. Mutation breaks equational reasoning.

Theis one would require very extensive refactoring, because Scorex relies a lot on mutable collections.
Therefore, I think we shouldn't even discuss whether this one is good or not. We should leave it aside for now.

I think that most of them can be applied in a single PR because they may not involve many changes (for example: Null, Product, FinalVal, JavaConversions, JavaSerializable and Serializable could probably be applied in a single PR).

I agree.

MutableDataStructures could be related to #78

Yes, it is. And #78 is a hard issue. #78 alone would already require several PRs, one per actor.

Enabling a Wart also imposes a sort of style standard. Hence, I wouldn't like to add warts without a certain level of consensus. Please let me know which ones you prefer me to work on (if any) and which to ignore (if any).

I think the ones for which I said either "good" or "agree" are quite uncontroversial. I think everyone would agree with them. I would start with those.

[EzequielPostan]

Good, thanks @ceilican for the comments.
I will experiment with the agreed ones and have a deeper look to the others.
I can treat the replacement of get for fold in a separate PR in a similar way we are doing with List's unsafe methods.
For the ones you stated a partial or total disagreement or are tricky to apply:

• Var: I would leave this one for later as it would need a separate PR in any case.
• DefaultArguments: I kinda agree with you on this one (also disagree with the Wart).
• ToString: I never applied this one before, I guess it could be useful in some contexts. I will leave it aside.
• FinalCaseClass: I also think this one is currently forbidden by the compiler, but should check. I will leave it aside too.
• FinalVal: I also think there are probably no cases for this one in the code base. I will enable it.
• ImplicitConversion: I kinda don't like this feature from the language, but won't deny that it is powerful when used with care. I won't apply this Wart.
• MutableDataStructures: As mentioned, this will be hard to apply if we decide to do so. I will leave it aside for now as suggested.

I will then apply (in more than one PR):

• TraversableOps (currently in progress)
• Null, Product, PublicInference, FinalVal, IsInstanceOf, JavaConversions, JavaSerializable, Serializable and LeakingSealed (probable in a single PR)
• OptionPartial (in a separate PR)
• Any (in a separate PR)
• Var (to be defined, but would be in a separate PR)
• MutableDataStructures (won't be applied now, to be evaluated in the future)

[EzequielPostan]

These are the pending Warts from the above list:

• LeakingSealed (I should review the impact of this one)
• OptionPartial (in a separate PR)
• Any (in a separate PR)
• Var (to be defined, but would be in a separate PR)
• MutableDataStructures (won't be applied now, to be evaluated in the future)

[EzequielPostan]

Just to clean up this issue.

• OptionPartial was enabled and an issue was opened to improve the general use of Option and Try constructors (Improve the use of Option and Try #243).
• LeakingSealed warns some cases of traits that extend other traits which are then extended in other files. Example:
  sealed trait NodeViewModifier is defined in src/main/scala/scorex/core/NodeViewModifier.scala
  In the same file we can see the definition
  trait EphemerealNodeViewModifier extends NodeViewModifier
  but then we have class Transaction extending EphemerealNodeViewModifier
abstract class Transaction extends EphemerealNodeViewModifier
  in the file src/main/scala/scorex/core/transaction/Transaction.scala
  The keyword sealed is usually used to let the compiler do an exhaustive search on pattern matchings, I guess we don't pattern match over instances of NodeViewModifier. Would it be okay to remove sealed from this case and similar ones (if any)?
• Any, Val and MutableDataStructures will demand a big effort and I may suggest to open separate issues for each of them if we intend to apply them.

[ceilican]

Would it be okay to remove sealed from this case and similar ones (if any)?

I think so.

Any, Val and MutableDataStructures will demand a big effort and I may suggest to open separate issues for each of them if we intend to apply them.

I suggest opening issues for Any and Val, but not for MutableDataStrucutures... Scorex's actors rely so much on mutable data structures that it would be impossible (and probable even not desirable) to get rid of them...