Fix compiler warnings #258

wlk · 2017-03-05T21:29:30Z

I noticed that there are a lot of compiler warnings during build, this PR attempts to fix (a large) portion of them:

unused imports
unused private methods
unused vals

jvican · 2017-03-06T09:36:13Z

This is so great @wlk 😄!

Let us know when this is ready, you probably want to squash all the commits since the previous ones were failing. That way, we make sure that commits pass build 👍

dwijnand

Thank you for spending time on this @wlk!

I have a few questions / requested changes, but other than that this looks good.

Welcome to Zinc.

dwijnand · 2017-03-06T10:10:53Z

.gitignore

@@ -1,2 +1,3 @@
 target/
 zinc/src/test/resources/bin
+.idea


If you are an IntelliJ IDEA user I would suggest you configure your global gitignore file, instead of expecting every project to include these ignores or adding them while submitting a pull request.

GitHub has a nice little help article about it: https://help.github.com/articles/ignoring-files/.

The general principle is that .gitignore should only include paths for files that are generated by running the build, and not include a random assortment of paths generated by other tools.

dwijnand · 2017-03-06T10:11:51Z

internal/compiler-bridge/src/main/scala/xsbt/ExtractUsedNames.scala

@@ -162,6 +162,7 @@ class ExtractUsedNames[GlobalType <: CallbackGlobal](val global: GlobalType) ext
        def usedNameInImportSelector(name: Name): Unit = {
          if (!isEmptyName(name) && (name != nme.WILDCARD) && !names.contains(name)) {
            names += name
+            ()


Just out of my own curiosity, could you explain this change?

There is a warning discarded non-Unit value I followed the suggestion from here: http://stackoverflow.com/questions/13415307/suppress-discarded-non-unit-value-warning (also saw this pattern in few other places in code)

Oh discarded inside the if block, of course. Thanks.

TBH in most PRs where () is added sombody is asking about it.

@eed3si9n @jvican @dwijnand Maybe we should remove that compiler setting?

No, value discarding is a real source of error, particularly with side-effecting (such as mutating) code.

One way to deal though with this is to explicitly discard the unused value. Either as a method or as an extension method (e.g .toUnit), and then the method could document its reason for existence.

The real solution is to change the compiler so -Ywarn-value-discard doesn't warn about this.type. Then you'll fix it for all codebases :-)

I vote in favor of removing that warning. IMO it's unnecessary. I generally think that this codebases abuses warnings. The same goes for unused imports.

I don't see why this is a real source of error. I have never experienced it.

I disagree with unused imports since later on we are left with tons on unused imports. However we don't need to apply it to all compilations - IMO we can apply it only for CIs builds (since hacking with fatal unused imports is not pleasant).

I think the compiler warnings do help with keeping the code more maintainable (assuming you change code to prevent warnings), so IMO both warnings could be left in as they are, maybe -Ywarn-value-discard is a little to verbose but that's not a big deal.

dwijnand · 2017-03-06T10:13:17Z

internal/zinc-apiinfo/src/main/scala/xsbt/api/SameAPI.scala

-    }
+  def apply(a: ClassLike, b: ClassLike): Boolean = {
+    new SameAPI(false, true).check(a, b)
+  }


Could you drop the brackets and one-line this please? Looks short enough.

dwijnand · 2017-03-06T10:14:28Z

internal/zinc-classfile/src/main/scala/sbt/internal/inc/classfile/Analyze.scala

      source <- guessSourcePath(sourceMap, classFile, log);
      binaryClassName = classFile.className;
      loadedClass <- load(binaryClassName, Some("Error reading API from class file"))
    ) {
      binaryClassNameToLoadedClass.update(binaryClassName, loadedClass)
-      val srcClassName = binaryToSourceName(loadedClass)
+      val maybeSrcClassName = binaryToSourceName(loadedClass)


I don't see the need for the Hungarian notation. I prefer the former.

No problem, I did this because few lines below we are reusing name srcClassName inside match -> I renamed the value inside the match to avoid confustion

dwijnand · 2017-03-06T10:58:59Z

internal/zinc-classfile/src/main/scala/sbt/internal/inc/classfile/Analyze.scala

-          classNames += srcClassName
+        case Some(className) =>
+          analysis.generatedNonLocalClass(source, newClass, binaryClassName, className)
+          classNames += className


One of the wonderful things about pattern matching (in my opinion) is the ability to shadow.

Was this change triggered by a warning? If so, what was the warning about? And who emitted it?

Good observation, this was warning generated by intellij "Suspicious shadowing by a Variable Pattern". I don't have strong opinion how to approach this but usually I try to avoid shadowing by using different names

Ah right, yeah I have that inspection turned off. This is its motivation:

Detects a Variable Pattern that shadows a stable identifier defined in the enclosing scope. To perform an equality test against that value, use backticks.

Before:

val foo = 0 0 match { case foo => }

After:

val foo = 0 0 match { case `foo` => }

Food for thought :-)

dwijnand · 2017-03-06T11:20:05Z

Approved, pending Travis CI building it successfully obviously.

jvican

LGTM!

One minor suggestion: squash all the commits 😄. Let's merge when CI passes.

dwijnand · 2017-03-06T12:16:44Z

You're getting

[info] ! Parser.able to parse all relevant classes: Exception raised on property evaluation.
[info] > ARG_0: class sbt.internal.inc.classfile.ParserSpecification$
[info] > Exception: java.lang.RuntimeException: Unknown constant: 97

in the 2.11 build. And a whole lot of errors in the 2.12 build.

wlk · 2017-03-06T12:41:52Z

@dwijnand I'll have more time to look at this tomorrow, most likely I have removed import that is reported as unused, but is in fact used (I have already stumbled on few cases like this here)

jvican · 2017-03-06T12:52:27Z

I'll have more time to look at this tomorrow, most likely I have removed import that is reported as unused, but is in fact used (I have already stumbled on few cases like this here)

Indeed, this happens because Intellij knows that you're using a concrete Scala version. The issue is that we use implicits for compatibility reasons, so that if a method is not defined in a class the compiler automatically tries to get an implicit class with that method. We use this to keep track of changes in the Scala compiler API.

That, mixed with some false positives from Intellij sides, may be the culprit of this. Double check that the removed imports are indeed used and you're good to go. Looking forward to merge this and remove all this clutter from my screen 👍

romanowski

Good stuff! Finally someone was brave enough to clean up the code :)

Except to minor rewrites (that are consequence of cleanup) LGTM

romanowski · 2017-03-06T18:21:50Z

internal/zinc-apiinfo/src/main/scala/sbt/internal/inc/ClassToAPI.scala

@@ -24,9 +24,9 @@ object ClassToAPI {
  // (api, public inherited classes)
  def process(classes: Seq[Class[_]]): (Seq[api.ClassLike], Set[(Class[_], Class[_])]) =
    {
-      val pkgs = packages(classes).map(p => new api.Package(p))
+      packages(classes).map(p => new api.Package(p))


If we really don't use return values we should replace map and flatMap with foreach (and remove overhead for creating unused collections).

Agreed 😄

romanowski · 2017-03-06T18:25:44Z

internal/zinc-compile-core/src/main/scala/sbt/internal/inc/javac/LocalJava.scala

@@ -73,14 +73,14 @@ final class LocalJavadoc() extends XJavadoc {
  override def run(sources: Array[File], options: Array[String], incToolOptions: IncToolOptions,
    reporter: Reporter, log: XLogger): Boolean = {
    val cwd = new File(new File(".").getAbsolutePath).getCanonicalFile
-    val (jArgs, nonJArgs) = options.partition(_.startsWith("-J"))
+    val (_, nonJArgs) = options.partition(_.startsWith("-J"))


If we care only about 'false' part of partition we should use filterNot

Agreed 👍

wlk · 2017-03-07T08:26:58Z

Commits squashed and travis should report green status in a moment.
There are still outstanding warnings, but I think this PR big enough for now.

The warnings I didn't change are related to scaladoc and few deprecation warnings

…used vals to _

Avoid empty generators when possible (take 2)

eed3si9n added the waiting for review label Mar 5, 2017

wlk force-pushed the wlk-fix-compiler-warnings branch from a4c8f6d to b86033e Compare March 6, 2017 10:10

jvican added enhancement spree labels Mar 6, 2017

dwijnand requested changes Mar 6, 2017

View reviewed changes

dwijnand reviewed Mar 6, 2017

View reviewed changes

dwijnand approved these changes Mar 6, 2017

View reviewed changes

jvican approved these changes Mar 6, 2017

View reviewed changes

romanowski approved these changes Mar 6, 2017

View reviewed changes

wlk force-pushed the wlk-fix-compiler-warnings branch from 292e27e to 5460ff4 Compare March 6, 2017 20:43

romanowski approved these changes Mar 6, 2017

View reviewed changes

wlk force-pushed the wlk-fix-compiler-warnings branch 2 times, most recently from 2a19103 to b09d79f Compare March 7, 2017 08:19

wlk changed the title ~~[WIP] Fix compiler warnings~~ Fix compiler warnings Mar 7, 2017

Fixed compiler warnings, mostly removed unused imports and renamed un…

63695f4

…used vals to _

wlk force-pushed the wlk-fix-compiler-warnings branch from b09d79f to 63695f4 Compare March 7, 2017 08:31

dwijnand merged commit da413f3 into sbt:1.0 Mar 7, 2017

dwijnand removed the waiting for review label Mar 7, 2017

wlk deleted the wlk-fix-compiler-warnings branch March 7, 2017 09:21

cunei pushed a commit to cunei/zinc that referenced this pull request Oct 25, 2017

Merge pull request sbt#258 from non/bugfix/minimize-empty-gen2

9c3e6a8

Avoid empty generators when possible (take 2)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix compiler warnings #258

Fix compiler warnings #258

wlk commented Mar 5, 2017 •

edited

Loading

jvican commented Mar 6, 2017

dwijnand left a comment •

edited

Loading

dwijnand Mar 6, 2017 •

edited

Loading

wlk Mar 6, 2017

dwijnand Mar 6, 2017

wlk Mar 6, 2017

dwijnand Mar 6, 2017

romanowski Mar 6, 2017

dwijnand Mar 6, 2017 •

edited

Loading

jvican Mar 6, 2017

romanowski Mar 6, 2017

wlk Mar 7, 2017

dwijnand Mar 6, 2017

wlk Mar 6, 2017

dwijnand Mar 6, 2017

wlk Mar 6, 2017

dwijnand Mar 6, 2017

wlk Mar 6, 2017

dwijnand Mar 6, 2017

dwijnand commented Mar 6, 2017

jvican left a comment

dwijnand commented Mar 6, 2017

wlk commented Mar 6, 2017

jvican commented Mar 6, 2017

romanowski left a comment

romanowski Mar 6, 2017

jvican Mar 6, 2017

romanowski Mar 6, 2017

jvican Mar 6, 2017

wlk commented Mar 7, 2017

Fix compiler warnings #258

Fix compiler warnings #258

Conversation

wlk commented Mar 5, 2017 • edited Loading

jvican commented Mar 6, 2017

dwijnand left a comment • edited Loading

Choose a reason for hiding this comment

dwijnand Mar 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dwijnand Mar 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dwijnand commented Mar 6, 2017

jvican left a comment

Choose a reason for hiding this comment

dwijnand commented Mar 6, 2017

wlk commented Mar 6, 2017

jvican commented Mar 6, 2017

romanowski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wlk commented Mar 7, 2017

wlk commented Mar 5, 2017 •

edited

Loading

dwijnand left a comment •

edited

Loading

dwijnand Mar 6, 2017 •

edited

Loading

dwijnand Mar 6, 2017 •

edited

Loading