I-176: add wartremover checks OptionPartial wart

[EzequielPostan]

This is part of #176

[coveralls]

Coverage remained the same at 46.617% when pulling 785dce9 on EzequielPostan:I-176-add-wartremover-checks-OptionPartial-wart into 8f08c75 on ScorexFoundation:master.

[ceilican]

If this is still a WIP, please add "WIP: " to the title.

I think OptionPartial is an important wart to remove.
We should really solve the problems, instead of just suppressing them.

I would focus on the core project first, and leave "testkit" and "example" aside at first.

[ceilican]

I suspect that this change breaks the semantics. When selfPeer == None, the old code would clearly result in Failure(e), whereas it is not clear what happens in the new code. Will parseBytes throw an exception when selfPeer is None?

[EzequielPostan]

I actually think that the semantics before the change would raise the exception and not Failure(e). The expression Some(selfPeer.get) is a by-value parameter in parseBytes

def parseBytes(bytes: ByteBuffer, sourceOpt: Option[ConnectedPeer]): Try[Message[_]]

and hence the exception should be thtown before calling the method. This exception is not being caught.
The only use of sourceOpt inside parseBytes method is actually at the very return value and it is

Message(spec, Left(msgData), sourceOpt)

Hence, it is not even checked.
This is still a different semantics than the one before the change, but I am not sure about the original intention. I will make a general comment about this PR to go further on this.

[ceilican]

Ok. Considering that parseBytes does not even check sourceOpt, I agree that your change is good. If we do want to throw an exception when selfPeer is None, we should do it in a more explicit way, and not by calling get on it.

@kushti: could you have a look on this, and tell us whether an exception should be thrown when selfPeer is None?

[ceilican]

Good!

[EzequielPostan]

First of all, thanks for the review @ceilican :)

I agree that this is a really important Wart, same as TraversableOps.
As my current understanding of the codebase is not that deep, I thought it would be better for me to just mark the places that should be reviewed and enable the wart to avoid new uses of unsafe get methods. Of course, in trivial cases I did apply changes (this was the way we worked on TraversableOps).

As a general comment after applying this wart, I would say that we could add more care to the way we handle certain corner case scenarios. In many places I could see that we use Option and Try parameters and just use get methods over them (sometimes with no success checks).
A general approach I could suggest is

1. If we expect the parameter not to be optional, then we should try not to use an Option/Try as parameter type. This would lead to the client to handle the lack of necessary information before calling the method.
2. If a method should receive this kind of optional values and depends on them to be successful (Some/Success) and can't provide a value of type A to handle failure cases, then one should return to the client an Option[A]/Try[A] instead of A. It should then be the client the one to either again solve the issue or forward the returned Option/Try.

Sometimes, this could be inconvenient due to the intended semantics.

I didn't mark this as WIP because, as mentioned, this was my intended PR to be merged. At least to enable the Wart. Should we use a different approach?

[ceilican]

I thought it would be better for me to just mark the places that should be reviewed

Yes. That was a good idea.

[ceilican]

A general approach I could suggest is [...]

I agree with your suggestion.

I didn't mark this as WIP because, as mentioned, this was my intended PR to be merged. At least to enable the Wart. Should we use a different approach?

I see. I was thinking that this would be a WIP until the execution of your suggestion. But I understand that you may not have the necessary knowledge of the intended semantics to execute your suggrstion completely on your own. In this case, let's approve and accept this PR, and create an issue with your suggestion. Then someone with a better knowledge of the semantics could remove the wart supressions in the future. Could you create the issue and link it to this PR?

[EzequielPostan]

Thanks @ceilican
I just created the issue #243

[kushti]

@EzequielPostan thanks for the contributions!