Add "Xlint:implicit-no-annotation" #5915

edmundnoble · 2017-05-20T04:44:08Z

Wondering if this needs a more self-explanatory name. Warns on implicits without type annotations.

Edit: Also please let me know if I am doing this in the wrong phase; I went as early as possible just because this info is available right away.

sjrd · 2017-05-20T06:47:03Z

An implicit val in a local scope (e.g., within a def) should be allowed not to have a type annotation. Only field vals require a type annotation.

However, all defs require a type annotation.

edmundnoble · 2017-05-20T18:19:26Z

Why exactly? I am working off of the comment in isValid in Implicits.scala, which seems to state that implicits without type annotations will not be considered for implicit resolution unless they occur before the search location in the source.

sjrd · 2017-05-20T18:49:06Z

Because scala/scala3#1784 and scala/scala3#1785.

Local implicit vals are only visible in the scope that follows them, so it is safe to let their type be inferred. Moreover, it is extremely annoying to write explicit types for local implicit vals. For example the Scala.js codebase is full of

def someTransformation(tree: Tree): Tree = {
  implicit val pos = tree.pos
  ...
}

edmundnoble · 2017-05-21T19:21:19Z

Makes sense to me. I'll try to make the change.

edmundnoble · 2017-05-23T06:46:46Z

Change made and tested.

sjrd · 2017-05-23T08:56:51Z

Thanks. I guess someone from the core team has to take over the technical review at this point.

retronym · 2017-05-23T09:21:13Z

Here's the corresponding check in dotty: https://github.com/lampepfl/dotty/blob/d524e28ebc4a5dc445a11df2548d7c9ea586b745/compiler/src/dotty/tools/dotc/typer/Namer.scala#L1062

retronym

It is also worth looking at the history of the corresponding rule in the wartremover lint tool: https://github.com/wartremover/wartremover/commits/master/core/src/main/scala/wartremover/warts/ExplicitImplicitTypes.scala

It evolved to allow { impicit x => ... } lambda params without an result type on the ValDef representing the lamdba param x; added an some sort of exception for implicit classes.

retronym · 2017-05-23T09:09:35Z

src/compiler/scala/tools/nsc/typechecker/Namers.scala

@@ -710,6 +710,9 @@ trait Namers extends MethodSynthesis {
      if (isScala) {
        if (nme.isSetterName(tree.name)) ValOrVarWithSetterSuffixError(tree)
        if (tree.mods.isPrivateLocal && tree.mods.isCaseAccessor) PrivateThisCaseClassParameterError(tree)
+        if (tree.mods.isImplicit && !owner.isTerm && settings.warnImplicitNoAnnotation && tree.tpt.isEmpty) {
+          reporter.warning(tree.pos, s"implicit value ${tree.name} has no type annotation")


Should be .name.decoded to avoid displaying encoded names like '$colon$colon`.

The term "type annotation" is widely understood but the spec talks about "result types" in this context (and about "type ascriptions" after expressions).

A precedent for wording of this sort of message is:

scala> def foo = { return 1; 1 } <console>:11: error: method foo has return statement; needs result type def foo = { return 1; 1 } ^

s/needs/should have a and I think it fits here.

(I guess this is to avoid overloading the term "annotation" with @annotations.)

Not that further motivation is needed, but I expected it to warn about lacking an implicit not found annotation.

retronym · 2017-05-23T09:22:00Z

project/ScalaOptionParser.scala

@@ -108,7 +108,7 @@ object ScalaOptionParser {
    "-g" -> List("line", "none", "notailcails", "source", "vars"),
    "-target" -> List("jvm-1.5", "jvm-1.6", "jvm-1.7", "jvm-1.8"))
  private def multiChoiceSettingNames = Map[String, List[String]](
-    "-Xlint" -> List("adapted-args", "nullary-unit", "inaccessible", "nullary-override", "infer-any", "missing-interpolator", "doc-detached", "private-shadow", "type-parameter-shadow", "poly-implicit-overload", "option-implicit", "delayedinit-select", "by-name-right-associative", "package-object-classes", "unsound-match", "stars-align"),
+    "-Xlint" -> List("adapted-args", "nullary-unit", "inaccessible", "nullary-override", "infer-any", "missing-interpolator", "doc-detached", "private-shadow", "type-parameter-shadow", "poly-implicit-overload", "option-implicit", "delayedinit-select", "by-name-right-associative", "package-object-classes", "unsound-match", "stars-align", "no-implicit-annotation"),


Perhaps: implicit-result-type

Without the rename, there is a typo here:

s/no-implicit-annotation/implicit-no-annotation

(The fact that you need to repeat yourself here is my fault, sorry...)

I'm curious; so my motivation was that it seems to be that all of these lint options are named for what they warn for. Nullary-unit, inaccessible, adapted args, these are all the "bad" that the lint option actually warns for itself. "implicit-result-type" seems inconsistent, because it would be named for the condition needed to not warn.

I thought about it and I agree the rename is the most consistent option.

This still looks unfixed?

I can attest that naming these options is hard. I suspect no one will ever request -Xlint:poly-implicit-overload. You warn for implicit-lacks-type, or -Xlint:infer-X, since the warning is triggered by inference. -Xlint:infer-implicit-type.

retronym · 2017-05-23T09:52:16Z

When adding a new warning, it's a can be helpful to run it across a few projects and review the output. That often turns up false positives. You can also post a gist of the output to help us review.

Here's how that looks for "re-starring" (using your modified compiler as the new reference compiler):

% sbt setupPublishCore publishLocal | tail -n 20

% git diff -U1
diff --git a/build.sbt b/build.sbt
index 8557e1280d..1e70b7d9ca 100644
--- a/build.sbt
+++ b/build.sbt
@@ -145,2 +145,3 @@ lazy val commonSettings = clearSourceAndResourceDirectories ++ publishSettings +
   fork in run := true,
+  scalacOptions in Compile ++= Seq("-Xlint:implicit-no-annotation"),
   scalacOptions in Compile in doc ++= Seq(

% sbt -Dstarr.version=2.12.3-bin-f092e93-SNAPSHOT test:compile

https://gist.github.com/53444a774c7675d7f115bd58b65ab498

Jasper-M · 2017-05-31T12:20:45Z

Does this also warn for implicit objects? Because they have the same problem.
Dotty doesn't seem to have a problem with implicit objects though, so maybe they can be fixed.

adriaanm · 2017-09-21T22:33:04Z

@edmundnoble, what's your plan regarding getting this across the finish line in time for 2.12.4? This would be a nice lint option and it looks like there isn't much more polish needed. To be included in 2.12.4, PRs must be mergeable (approved by reviewer) by Wed Sep 27.

edmundnoble · 2017-09-24T23:17:52Z

@retronym I've rebased, and added tests for the special cases you've mentioned: they didn't require any more changes to the warning check. I also changed "type annotation" to "result type" in the lint option and the warning itself.

I'm considering maybe relaxing the implicit def condition to allow no result type if the RHS is just a new call, but I think this is worth merging into 2.12.4 as it is now.
EDIT: I'm considering this exception based mostly on implicit conversions into AnyVal classes which usually take this form as well as the cases I encountered after re-starring.

adriaanm · 2017-09-28T22:01:42Z

We'll allow PRs currently scheduled for 2.12.4 to be merged until Friday, run benchmarks and community builds over the weekend and Monday, and cut the release on Tuesday. Any PRs that regress performance / break the community build will be reverted and rescheduled for 2.12.5.

adriaanm · 2017-10-01T02:56:24Z

We ran out of time here for 2.12.4, but let's get this into a nightly as soon as all review comments are implemented.

martin-g · 2018-01-10T19:43:02Z

Ping! Is it a good time to merge this PR for 2.12.5 ?

SethTisue · 2018-02-22T21:50:23Z

should I do a community build run so we can grep the log for the new warning and see what turns up?

lrytz · 2018-02-23T11:04:47Z

@edmundnoble as far as I can see there are still a few minor changes to be done:

rename the option from no-implicit-annotation to implicit-result-type across the PR (including test file names)
use tree.name.decoded instead of tree.name in the warning messages, thest for such names
update the warning messages from has no type annotation to should have a result type

@SethTisue the logic looks correct, so you can run it through the community build already now.

SethTisue · 2018-02-28T00:43:00Z

I'm sorry this PR sat so long — we are really trying to do better on this in 2018

gah, and actually I can't run (not easily, anyway) the community build until this is rebased, the validate-publish-core job was deleted. @edmundnoble would you mind rebasing onto current 2.12.x...?

Add local test Remove warning for local implicit vals More tests, inspired by WartRemover's Implicit object test

lrytz · 2018-03-01T12:51:42Z

@edmundnoble / @SethTisue I rebased and pushed -f to edmundnoble/implicit-no-annotation (there were no conflicts)

SethTisue · 2018-03-02T00:05:28Z

community build run queued: https://scala-ci.typesafe.com/view/scala-2.12.x/job/scala-2.12.x-integrate-community-build/2635/ ~~(404 til Jenkins finishes shaving his toes)~~

lrytz · 2018-03-02T11:29:43Z

That didn't go very far:

ERROR: Couldn't find any revision to build. Verify the repository and branch configuration for this job.

SethTisue · 2018-03-06T02:04:54Z

doh. trying again: https://scala-ci.typesafe.com/view/scala-2.12.x/job/scala-2.12.x-integrate-community-build/2643/consoleFull

SethTisue · 2018-03-06T20:03:36Z

gah, that timed out. queued run 2645. builds from 2643 should still be cached, so when it comes time to grep the logs, we'll need to grep both of them together.

SethTisue · 2018-03-06T20:08:00Z

(note that these runs don't explicitly enable the new flag everywhere, so we're only seeing the warnings for projects that have -Xlint enabled. this is probably sufficient to evaluate the quality.)

lrytz · 2018-03-08T13:02:38Z

The build timed out; there's 125 lines with "has no type annotation" in the log, checking if those are all intended would give a good estimate.

SethTisue · 2018-03-08T16:28:39Z

in run 2645 which is a continuation of 2643, there are a bunch of new compiler errors like:

[kind-projector] [error] /home/jenkins/workspace/scala-2.12.x-integrate-community-build/target-0.9.11/project-builds/kind-projector-cad88983fe673ab2ad72b4a5f1d7ca5de5e38d0e/src/main/scala/KindProjector.scala:93:14: not enough arguments for method require: (requirement: Boolean, message: => Any)Unit.
[kind-projector] [error] Unspecified value parameter message.
[kind-projector] [error]       require(i >= 0)
[kind-projector] [error]              ^

SethTisue · 2018-03-13T22:07:41Z

this needs another rebase, now, in order to get better community build results

adriaanm · 2018-05-30T07:45:31Z

Closing due to inactivity. Happy to accept a resubmission, though that should go to 2.13.x now.

som-snytt · 2018-05-30T15:52:38Z

Maybe the community build should be peer-to-peer, with "volunteer" computers all over the world verifying every scala project for every commit.

adriaanm · 2018-05-30T20:43:22Z

We promise we won't mine bitcoin in macros.

som-snytt · 2024-03-12T07:13:25Z

This noble effort landed not as a lint but under -Xsource:3.

scala-jenkins added this to the 2.12.3 milestone May 20, 2017

edmundnoble force-pushed the implicit-no-annotation branch from 36ea4c8 to f092e93 Compare May 22, 2017 05:39

retronym reviewed May 23, 2017

View reviewed changes

SethTisue modified the milestones: 2.12.4, 2.12.3 Jun 27, 2017

adriaanm added the release-notes worth highlighting in next release notes label Sep 21, 2017

edmundnoble force-pushed the implicit-no-annotation branch 2 times, most recently from 876630d to b688a4f Compare September 24, 2017 21:24

adriaanm modified the milestones: 2.12.4, 2.12.5 Oct 1, 2017

lrytz self-assigned this Feb 23, 2018

Add "Xlint:implicit-no-annotation"

ce8ec43

Add local test Remove warning for local implicit vals More tests, inspired by WartRemover's Implicit object test

lrytz force-pushed the implicit-no-annotation branch from b688a4f to ce8ec43 Compare March 1, 2018 12:51

lrytz modified the milestones: 2.12.5, 2.12.6 Mar 12, 2018

SethTisue modified the milestones: 2.12.6, 2.12.7 Mar 23, 2018

adriaanm closed this May 30, 2018

SethTisue removed this from the 2.12.7 milestone Sep 10, 2018

Add "Xlint:implicit-no-annotation" #5915

Add "Xlint:implicit-no-annotation" #5915

Uh oh!

Conversation

edmundnoble commented May 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sjrd commented May 20, 2017

Uh oh!

edmundnoble commented May 20, 2017

Uh oh!

sjrd commented May 20, 2017

Uh oh!

edmundnoble commented May 21, 2017

Uh oh!

edmundnoble commented May 23, 2017

Uh oh!

sjrd commented May 23, 2017

Uh oh!

retronym commented May 23, 2017

Uh oh!

retronym left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

retronym commented May 23, 2017

Uh oh!

Jasper-M commented May 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adriaanm commented Sep 21, 2017

Uh oh!

edmundnoble commented Sep 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adriaanm commented Sep 28, 2017

Uh oh!

adriaanm commented Oct 1, 2017

Uh oh!

martin-g commented Jan 10, 2018

Uh oh!

SethTisue commented Feb 22, 2018

Uh oh!

lrytz commented Feb 23, 2018

Uh oh!

SethTisue commented Feb 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lrytz commented Mar 1, 2018

Uh oh!

SethTisue commented Mar 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lrytz commented Mar 2, 2018

Uh oh!

SethTisue commented Mar 6, 2018

Uh oh!

SethTisue commented Mar 6, 2018

Uh oh!

SethTisue commented Mar 6, 2018

Uh oh!

edmundnoble commented May 20, 2017 •

edited

Loading

Jasper-M commented May 31, 2017 •

edited

Loading

edmundnoble commented Sep 24, 2017 •

edited

Loading

SethTisue commented Feb 28, 2018 •

edited

Loading

SethTisue commented Mar 2, 2018 •

edited

Loading