Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable scalac linting / fatal warnings and fix all warnings. #1373

Merged
merged 1 commit into from Oct 28, 2017

Conversation

@jbgi
Copy link
Member

@jbgi jbgi commented Apr 27, 2017

We may not want to include/fix all warnings, but there it is, for further discussion.

@@ -228,7 +228,7 @@ object EitherT extends EitherTInstances {
}

def eitherTU[FAB, AB, A0, B0](fab: FAB)(
implicit u1: Unapply[Functor, FAB]{type A = AB}, u2: Unapply2[Bifunctor, AB]{type A = A0; type B = B0}, l: Leibniz.===[AB, A0 \/ B0])
Copy link
Member Author

@jbgi jbgi Apr 27, 2017

Choose a reason for hiding this comment

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

The unused Unapply2 was there to guide type inference but it is probably no more necessary... ?

Loading

Copy link
Member Author

@jbgi jbgi Apr 27, 2017

Choose a reason for hiding this comment

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

oups... looks like it is still necessary.

Loading

@jbgi jbgi force-pushed the scalac-options branch 3 times, most recently from 34e28ec to 2e6ad75 Apr 27, 2017
Copy link
Member

@S11001001 S11001001 left a comment

Main thing I suppose is that I don't think we should have all the extra statement lists + () implied by -Ywarn-value-discard.

Loading

@@ -105,7 +105,7 @@ object LazyEitherT extends LazyEitherTInstances {
u1: Unapply[Functor, FAB]{type A = AB},
u2: Unapply2[Bifunctor, AB]{type A = A0; type B = B0},
l: Leibniz.===[AB, LazyEither[A0, B0]]
): LazyEitherT[u1.M, A0, B0] = LazyEitherT(l.subst[u1.M](u1(fab)))
): LazyEitherT[u1.M, u2.A, u2.B] = LazyEitherT(l.subst[u1.M](u1(fab)))
Copy link
Member

@S11001001 S11001001 Apr 28, 2017

Choose a reason for hiding this comment

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

Ehh.....this is tricky in the face of expected-type inference (the former propagates while the latter doesn't), best leave it as it was.

Loading

Copy link
Member Author

@jbgi jbgi Apr 28, 2017

Choose a reason for hiding this comment

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

yeah, this was to use the u2 parameter, otherwise I get an "unused parameter" warning...
But not sure to understand why this could be a problem for u2.A / u2.B while it apparently is not for u1.M?

Loading

Copy link
Member

@S11001001 S11001001 Apr 28, 2017

Choose a reason for hiding this comment

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

It is a problem for u1.M, too, but we can't fix it due to scala/bug#5075. (This is not scala/bug#2712, despite some references therein and surface similarities.)

We don't have that problem for A0 and B0, so we can Do The Right Thing for those.

Loading

Copy link
Member

@S11001001 S11001001 Apr 28, 2017

Choose a reason for hiding this comment

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

That's also why 1110a5e didn't include an Aux variant including the hk member; scala/bug#5075 makes it so it never works.

Loading

Copy link
Member Author

@jbgi jbgi Apr 29, 2017

Choose a reason for hiding this comment

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

@S11001001 used deprecated annotation to avoid the warning.

Loading

@@ -88,7 +88,7 @@ sealed abstract class AdjunctionInstances {

implicit def curryUncurryAdjunction[S]: (S, ?) -| (S => ?) =
new Adjunction[(S, ?), (S => ?)] {
override def leftAdjunct[A, B](a: => A)(f: ((S, A)) => B): S => B = s => f(s, a)
override def leftAdjunct[A, B](a: => A)(f: ((S, A)) => B): S => B = s => f((s, a))
Copy link
Member

@S11001001 S11001001 Apr 28, 2017

Choose a reason for hiding this comment

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

Huh, could have sworn we had -Yno-adapted-args a long time ago. Oh well 😎

Loading

@@ -807,18 +805,6 @@ sealed abstract class FingerTree[V, A](implicit measurer: Reducer[A, V]) {
)
}

private def traverseFinger[F[_], A, B, V2](digit: Finger[V, A])(f: A => F[B])(implicit ms: Reducer[B, V2], F: Applicative[F]): F[Finger[V2, B]] = {
Copy link
Member

@S11001001 S11001001 Apr 28, 2017

Choose a reason for hiding this comment

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

Ah, my second favorite reason for using private all over the place. (My first is that incremental compiles are faster 🏎 )

Loading

@@ -268,14 +268,14 @@ private trait StateTMonadState[S, F[_]] extends MonadState[StateT[F, S, ?], S] w

private trait StateTHoist[S] extends Hoist[λ[(g[_], a) => StateT[g, S, a]]] {

type StateTF[G[_], S] = {
type StateTF[G[_]] = {
Copy link
Member

@S11001001 S11001001 Apr 28, 2017

Choose a reason for hiding this comment

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

Nice.

Loading

@@ -110,7 +110,7 @@ object TracedT extends TracedTInstances {
U1: Unapply[Functor, WAB]{type A = AB},
U2: Unapply2[Profunctor, AB]{type A = A0; type B = B0},
L: Leibniz.===[AB, A0 => B0]
): TracedT[U1.M, A0, B0] = TracedT(L.subst[U1.M](U1(wab)))
): TracedT[U1.M, U2.A, U2.B] = TracedT(L.subst[U1.M](U1(wab)))
Copy link
Member

@S11001001 S11001001 Apr 28, 2017

Choose a reason for hiding this comment

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

As with other A0/B0 change.

Loading

"-Ywarn-dead-code",
"-Ywarn-unused-import",
"-Ywarn-numeric-widen",
"-Ywarn-value-discard",
Copy link
Member

@S11001001 S11001001 Apr 28, 2017

Choose a reason for hiding this comment

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

Let's turn off -Ywarn-value-discard and not have all the extra () noise. That warning's more suited to a heavily imperative codebase.

Loading

Copy link
Member

@TomasMikula TomasMikula Apr 28, 2017

Choose a reason for hiding this comment

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

Or maybe only turn it on for core?

Loading

Copy link
Member Author

@jbgi jbgi Apr 28, 2017

Choose a reason for hiding this comment

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

@puffnfresh what is your opinion on this option? (as you wrote NonUnitStatements)

Loading

@@ -34,7 +34,7 @@ import scala.reflect.ClassTag
sealed abstract class Validation[+E, +A] extends Product with Serializable {

final class SwitchingValidation[X](s: => X){
def <<?:(fail: => X): X =
def <<?:(fail: X): X =
Copy link
Member

@S11001001 S11001001 Apr 28, 2017

Choose a reason for hiding this comment

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

Whoops, thanks.

Loading

@@ -284,7 +284,7 @@ trait WriterTFunctions {
u1: Unapply[Functor, FAB]{type A = AB},
u2: Unapply2[Bifunctor, AB]{type A = A0; type B = B0},
l: Leibniz.===[AB, (A0, B0)]
): WriterT[u1.M, A0, B0] = WriterT(l.subst[u1.M](u1(fab)))
): WriterT[u1.M, u2.A, u2.B] = WriterT(l.subst[u1.M](u1(fab)))
Copy link
Member

@S11001001 S11001001 Apr 28, 2017

Choose a reason for hiding this comment

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

A0/B0 etc etc

Loading

"-Ywarn-unused-import",
"-Ywarn-numeric-widen",
"-Ywarn-value-discard",
"-Xfatal-warnings",
Copy link
Member

@xuwei-k xuwei-k Apr 28, 2017

Choose a reason for hiding this comment

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

remove -Xfatal-warnings when generating scaladoc ?

[error] /home/travis/build/scalaz/scalaz/core/src/main/scala/scalaz/Validation.scala:6: Could not find any member to link for "a]Validation[E,".
[error] /**
[error] ^
[error] /home/travis/build/scalaz/scalaz/core/src/main/scala/scalaz/Validation.scala:341: Variable i undefined in comment for method excepting in class Validation
[error]    *   strings map (_.parseInt excepting { case i if i < 0 => new Exception(s"Int must be positive: $i") })
[error]                                                                                                      ^
model contains 44 documentable templates
[error] /home/travis/build/scalaz/scalaz/scalacheck-binding/src/main/scala/scalaz/scalacheck/ScalazArbitrary.scala:9: Could not find any member to link for "scalacheck.Arbitrary".
[error] /**
[error] ^
[error] /home/travis/build/scalaz/scalaz/scalacheck-binding/js/src/main/scala/scalaz/scalacheck/ScalazArbitraryPlatform.scala:3: Could not find any member to link for "scalacheck.Arbitrary".
[error] /**
[error] ^
[error] two errors found
[error] /home/travis/build/scalaz/scalaz/core/src/main/scala/scalaz/Order.scala:38: unbalanced or unclosed heading
[error]   /** @note `Order.fromScalaOrdering(toScalaOrdering).order(x, y)`
[error]   ^
[error] /home/travis/build/scalaz/scalaz/core/src/main/scala/scalaz/Free.scala:77: Could not find any member to link for "scalaz.iteratee.Iteratee".
[error]   /** A computation that accepts values of type `A`, eventually resulting in a value of type `B`.
[error]   ^
[error] /home/travis/build/scalaz/scalaz/core/src/main/scala/scalaz/FingerTree.scala:768: Could not find any member to link for "if".
[error]   /**
[error]   ^
[error] /home/travis/build/scalaz/scalaz/core/src/main/scala/scalaz/FingerTree.scala:761: Could not find any member to link for "if".
[error]   /**
[error]   ^
[error] /home/travis/build/scalaz/scalaz/core/src/main/scala/scalaz/FingerTree.scala:754: Could not find any member to link for "if".
[error]   /**
[error]   ^
[error] /home/travis/build/scalaz/scalaz/core/src/main/scala/scalaz/FingerTree.scala:747: Could not find any member to link for "if".
[error]   /**
[error]   ^
[error] /home/travis/build/scalaz/scalaz/core/src/main/scala/scalaz/FingerTree.scala:696: Could not find any member to link for "if".
[error]   /**
[error]   ^
[error] /home/travis/build/scalaz/scalaz/core/src/main/scala/scalaz/FingerTree.scala:38: Could not find any member to link for "if".
[error]   /**
[error]   ^
[error] /home/travis/build/scalaz/scalaz/core/src/main/scala/scalaz/FingerTree.scala:31: Could not find any member to link for "if".
[error]   /**
[error]   ^
[error] 11 errors found
[error] (scalaz/scalaunidoc:doc) Scaladoc generation failed
[error] (iterateeJS/compile:doc) Scaladoc generation failed
[error] (iterateeJVM/compile:doc) Scaladoc generation failed
[error] (coreJVM/compile:doc) Scaladoc generation failed
[error] (scalacheckBindingJVM/compile:doc) Scaladoc generation failed
[error] (scalacheckBindingJS/compile:doc) Scaladoc generation failed
[error] (coreJS/compile:doc) Scaladoc generation failed

https://travis-ci.org/scalaz/scalaz/jobs/226610713#L7169

Loading

@jbgi jbgi force-pushed the scalac-options branch 3 times, most recently from b5d111c to 4ce041a Apr 28, 2017
@xuwei-k xuwei-k dismissed their stale review Apr 28, 2017

test passed

@jbgi jbgi force-pushed the scalac-options branch 2 times, most recently from 0967887 to eb314c6 Apr 29, 2017
@jbgi
Copy link
Member Author

@jbgi jbgi commented Apr 29, 2017

@S11001001 I disabled -Ywarn-value-discard for scalacheck-bindings where it causes much noise, and use val _ = ... to avoid the extra statement elsewhere. Is that ok?

Loading

@jbgi jbgi force-pushed the scalac-options branch 2 times, most recently from d2a53f3 to bf8ea28 Apr 29, 2017
@jbgi jbgi force-pushed the scalac-options branch from bf8ea28 to fbae192 Oct 28, 2017
@jbgi
Copy link
Member Author

@jbgi jbgi commented Oct 28, 2017

Loading

@S11001001 S11001001 merged commit f127e47 into scalaz:series/7.3.x Oct 28, 2017
1 check passed
Loading
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants