Resolve sbt warnings during build

[witgo]

No description provided.

[AmplabJenkins]

Merged build triggered.

[AmplabJenkins]

Merged build started.

[AmplabJenkins]

Merged build finished. All automated tests passed.

[AmplabJenkins]

All automated tests passed.
Refer to this link for build results: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/15711/

[srowen]

The changes you are removing were put in place to resolve warnings from Scala 2.10. IIRC the code does not even compile without these in Scala 2.11. What is this resolving?

[pwendell]

@srowen@witgo - as far as I can tell, this adds rather than removes build warnings. I also noticed that your patch #1032 actually caused new warnings because you removed some of the scala imports. We should add those back.

[witgo]

@srowen If we use -language:postfixOps, This scala.language.postfixOps is not necessary. as proposed in the SIP-18 document.

@pwendell#1032 remove duplicate import scala.language.postfixOps

[huitseeker]

@witgo@pwendell@srowen Correct : adding -language:postfixOps to your scalac line (scalacOptions for sbt) will remove the warning.

[srowen]

Yes, but then what warnings is this resolving?

I understand one compiler flag is shorter than imports. I also understand that these language features are considered errors in Scala 2.11, instead of warnings. It seems like they are kind of discouraged or at least something that has to be explicitly enabled by the developer. Rather than globally reverse this, it seemed better (although more verbose) to only enable them where needed, and then perhaps consider later whether the code should be using these constructs. It also means you don't require special compiler flags to get the source to compile.

[huitseeker]

It resolves the following warning, which occurs in different (but easy-to-reach) contexts depending on whether you are running with the -language option and your version of Scala :

[WARNING] your/source/File.scala:42:warning: postfix operator foo should be enabled
[WARNING] by making the implicit value scala.language.postfixOps visible.
[WARNING] This can be achieved by adding the importclause 'importscala.language.postfixOps'
[WARNING] or by setting the compiler option -language:postfixOps.
[WARNING] See the Scala docs for value scala.language.postfixOps for a discussion
[WARNING] why the feature should be explicitly enabled.

(aforementioned scaladoc)

Like other features in SIP-18, postfix operators (spec 6.1.2) are modularized. This SO answer gives a hint at why : in some cases, it adds some parser ambiguity which is not always obvious to resolve without some experience with the issue.

The choice of whether to add the flag globally or piecewise is mostly a maintainability issue : if people editing the code base are expected to be able to handle the potential parser errors fired when editing with this option turned on, it may make more sense to leave it on globally.

[srowen]

Yes, I understand that, but these warnings are already suppressed with imports, and the advertised change here is to resolve warnings. It just exchanges mechanisms for suppressing the warning/error. I myself think it's best to not enable it globally.

[huitseeker]

Fair enough. I just meant to provide context.

Note that this commit has already added postfixOps to the pom.xml. Another datapoint : there's, as of b3736e3, 20 matches fo the postfixOps import in the codebase.

[srowen]

I'm more concerned with consistency than anything. We shouldn't do it both ways, and I don't think that compiler arg should have been added. A compiler flag is not a bad solution per se though. Whatever the majority opinion is on which way to handle this, let's make sure it's consistent, via this or another PR.

[rxin]

Can one of you give me a tl;dr on the verdict about this one? :)

[srowen]

@rxin It looks like Scala is requiring developers to be more explicit about intention to use these features; these warnings become errors in Scala 2.11 actually. So my opinion is that use of the feature should not be enabled globally with a compiler flag, but enabled per compilation unit with an import. The latter is the currently how the codebase works.

Since this PR just swaps the latter for former, I personally would argue that this not be merged. Actually, an earlier PR also add the compiler flag though. I think it should be removed and any extra warnings fixed. If there is any support for that, and no objections, I can raise a PR for that.

[rxin]

Thanks for summarizing that. I agree that we should not have a global flag that just disables certain warnings, since that could hide potential problems in the future.

[witgo]

@srowen If so, I close the PR and I will submit a new PR meets you said.

[srowen]

@witgo Go ahead. Right now I actually don't see any warnings appear when the compiler flag is removed, so it looks like all other warnings are suppressed locally already. So it can just be removed.