Feedback on first use about scapegoat HOT 10 CLOSED

Great thanks for the feedback. It's good to get this running on a large codebase.

In general, the lack of "explanation behind a feature is because the project is new". So yes I need to update the descriptions with the rationale behind it, for things like var, use of null, use of return.

• asInstanceOf is bad you shouldn't be using it. Do this in your actor.

system match {
  case impl: ActorSystemImpl => walk(impl.lookupRoot)
  case _ =>
}

Now its safe for if the type changes from ActorSystemImpl, eg they introduced ActorSystemRemoteImpl. asInstanceOf will explode if the type changes.

• Actor Receive methods are because of partial functions. That's fixable.Issue todo.
• XML var use - fixable. Issue todo.
• JavaConversion - bad, they're not recommended for use anymore. You should be using JavaConvertors. Otherwise you can get implicit conversions when you don't want it, and it creates multiple intermediate lists. I think I updated the scala style guidelines to say this, if not then there's a PR pending still.
• Option.get - you should be pattern matching

Option(something) match {
  case Some(something) =>
  case None =>
}

• We have some false positives where the param is required to implement an interface, but not used by all implementor. Sounds like it should be ignored in interfaces/traits. It should be ignoring abstract methods already.
• Override warnings - yes, I want to do this. But code comments are nuked out at parser. Could do an annotation perhaps. Issue todo.
• Report filter - great idea. Issue todo.

from scapegoat.

asInstanceOf is bad you shouldn't be using it. Do [match] in your actor. ... Now its safe for if the type changes from ActorSystemImpl, eg they introduced ActorSystemRemoteImpl. asInstanceOf will explode if the type changes.

Yes, in most cases I agree.
In this case, where I want a cast operation, and I want to throw "unexpected type, needed X but found Y", then

x.asInstanceOf[X]

is much more concise than

x match {
  case x: X => x
  case y => throw new ArgumentException(s"Expected 'X', but found ${y.getClass}")
}

Keeping this as a warning, and allowing exceptions would address the above concern. Something like:

@scapegoatIgnore("asInstanceOf", "We need access to the underlying type X, and it is an error if we find a Y")
x.asInstanceOf[X]

Option.get - you should be pattern matching

Agreed, with the same caveats as above

from scapegoat.

JavaConversion - bad, they're not recommended for use anymore. You should be using JavaConvertors. Otherwise you can get implicit conversions when you don't want it, and it creates multiple intermediate lists. I think I updated the scala style guidelines to say this, if not then there's a PR pending still.

Thanks, I wasn't aware of that. I'll look into it.

I hope they add some deprecation warnings. "'Conversion' bad, 'Convertors' good" seems like it will be hard to remember.

from scapegoat.

So the inspection is correct, it's telling you you're doing something
non-idomatic and "bad". So your suggestion to turn it off is what I was
thinking. Perhaps there's an @ignore annotation anyway we can use rather
than creating one. @SupressWarnings, doesn't Java use that.

On 28 July 2014 21:35, Richard Bradley [email protected] wrote:

asInstanceOf is bad you shouldn't be using it. Do [match] in your actor.
... Now its safe for if the type changes from ActorSystemImpl, eg they
introduced ActorSystemRemoteImpl. asInstanceOf will explode if the type
changes.

Yes, in most cases I agree.
In this case, where I want a cast operation, and I want to throw
"unexpected type, needed X but found Y", then

x.asInstanceOf[X]

is much more concise than

x match {
case x: X => x
case y => throw new ArgumentException(s"Expected 'X', but found ${y.getClass}")
}

Keeping this as a warning, and allowing exceptions would address the above
concern. Something like:

@scapegoatIgnore("asInstanceOf", "We need access to the underlying type X, and it is an error if we find a Y")
x.asInstanceOf[X]

—
Reply to this email directly or view it on GitHub
#4 (comment).

from scapegoat.

I've just been looking down this findbugs list again. It's very
comprehensive but you must get a massive amount of false positives. The
inspections for statics, collections usage, fields (especially fields) are
all irrelevant. I think you could cherry pick out the ones that are
applicable to Scala, create a deployment just for that, and then you've got
basically a findbugs-scala.

On 28 July 2014 21:40, Stephen Samuel (Sam) [email protected] wrote:

So the inspection is correct, it's telling you you're doing something
non-idomatic and "bad". So your suggestion to turn it off is what I was
thinking. Perhaps there's an @ignore annotation anyway we can use rather
than creating one. @SupressWarnings, doesn't Java use that.

On 28 July 2014 21:35, Richard Bradley [email protected] wrote:

asInstanceOf is bad you shouldn't be using it. Do [match] in your actor.
... Now its safe for if the type changes from ActorSystemImpl, eg they
introduced ActorSystemRemoteImpl. asInstanceOf will explode if the type
changes.

Yes, in most cases I agree.
In this case, where I want a cast operation, and I want to throw
"unexpected type, needed X but found Y", then

x.asInstanceOf[X]

is much more concise than

x match {
case x: X => x
case y => throw new ArgumentException(s"Expected 'X', but found ${y.getClass}")
}

Keeping this as a warning, and allowing exceptions would address the
above concern. Something like:

@scapegoatIgnore("asInstanceOf", "We need access to the underlying type X, and it is an error if we find a Y")
x.asInstanceOf[X]

—
Reply to this email directly or view it on GitHub
#4 (comment).

from scapegoat.

Yes, I've turned off whole categories of stuff, but still there are loads of false-positives.

The auto-generated methods on case classes seem to confuse Findbugs a lot (warnings like "NP: Null pointer dereference of ? in new MyClass" on each case class), as well as the code generated from "match" statements.

I haven't had time to go through it properly. Looking over our current report, there are some good ones in there: "EQ_DOESNT_OVERRIDE_EQUALS" "This class extends a class that defines an equals method and adds fields, but doesn't define an equals method itself.", etc. etc., but they're buried in the noise.

If you build on Findbugs, you'd get a sophisticated "ignore" framework, as well as this back-catalogue of rules. (XML ignore-by-file, as well as attribute based ignore-by-method and ignore-by-statement).

from scapegoat.

So it sounds like the best part of find bugs is the tooling rather than the
actual bug reports?

On 28 July 2014 23:06, Richard Bradley [email protected] wrote:

Yes, I've turned off whole categories of stuff, but still there are loads
of false-positives.

The auto-generated methods on case classes seem to confuse Findbugs a lot
(warnings like "NP: Null pointer dereference of ? in new MyClass" on each
case class), as well as the code generated from "match" statements.

I haven't had time to go through it properly. Looking over our current
report, there are some good ones in there: "EQ_DOESNT_OVERRIDE_EQUALS"
"This class extends a class that defines an equals method and adds fields,
but doesn't define an equals method itself.", etc. etc., but they're buried
in the noise.

If you build on Findbugs, you'd get a sophisticated "ignore" framework, as
well as this back-catalogue of rules. (XML ignore-by-file, as well as
attribute based ignore-by-method and ignore-by-statement).

—
Reply to this email directly or view it on GitHub
#4 (comment).

from scapegoat.

So it sounds like the best part of find bugs is the tooling rather than the
actual bug reports?

lol, not entirely ;-)

The bug detectors represent a gold mine of painfully acquired experience and Java expertise, accumulated over many years (especially if you include findbugs-contrib).

The tooling is slightly old-fashioned in places, but very functional, and it would be expensive to re-create it from scratch.

from scapegoat.

I look forward to being able to use it on my projects!

On 29 July 2014 10:21, Richard Bradley [email protected] wrote:

So it sounds like the best part of find bugs is the tooling rather than the
actual bug reports?

lol, not entirely ;-)

The bug detectors represent a gold mine of painfully acquired experience
and Java expertise, accumulated over many years (especially if you include
findbugs-contrib).

The tooling is slightly old-fashioned in places, but very functional, and
it would be expensive to re-create it from scratch.

—
Reply to this email directly or view it on GitHub
#4 (comment).

from scapegoat.

Issues have been created for the feedback here, so this can be closed now.

from scapegoat.