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

warn on implicit def without explicit result type #5265

Closed
scabug opened this issue Dec 3, 2011 · 9 comments · Fixed by scala/scala#10083
Closed

warn on implicit def without explicit result type #5265

scabug opened this issue Dec 3, 2011 · 9 comments · Fixed by scala/scala#10083

Comments

@scabug
Copy link

scabug commented Dec 3, 2011

The following code snipet : https://gist.github.com/1427587
does not compile.

import java.util.Date

trait TDate 

trait TT[A1,T1]

trait TTFactory[F,G] {
  def create(f: F) : TT[F,G]
  def sample: F
}

object Impls {

  // If the c1 is declared before c2, it compiles fine
  // or if the implicit's result type is specified explicitly
  implicit def c2(s: Date)/* : TT[Date, TDate] */ = c1.create(s)  

  implicit val c1 = new TTFactory[Date,TDate] {
    def create(v: Date): TT[Date,TDate] = sys.error("")
    def sample = new Date
  }
}
@scabug
Copy link
Author

scabug commented Dec 3, 2011

Imported From: https://issues.scala-lang.org/browse/SI-5265?orig=1
Reporter: maxime.levesque
Affected Versions: 2.9.1
See #5348, #2206, #801

@scabug
Copy link
Author

scabug commented Jun 11, 2012

@adriaanm said:
see #2206, #801 and a plethora of duplicates

@scabug
Copy link
Author

scabug commented Jul 3, 2012

@adriaanm said:
warn unless enabled by feature flag

@scabug
Copy link
Author

scabug commented Oct 10, 2014

@retronym said:
I have a feeling there actually isn't a cycle in this example. Even if there is, we should report it.

Instead, I think the problem here is that typedBlock can trigger a type completion which causes the typedBlock for the same block to be reentered. Crucially, this happens after the DefTrees in the block have the symbols assigned. In the second run through typedBlock, the namer sees the attributed trees and decides not to enter them in scope.

The presentation compiler has a more robust namer to account for the sort of symptom, as caused by cancellation. I've moved that in to the regular namer in this WIP branch.

https://github.com/retronym/scala/tree/ticket/5265

That said, I definetely agree we should also have a warning / migration for unannotated implicits.

@scabug
Copy link
Author

scabug commented Oct 10, 2014

@retronym said:
That branch fails on (at least)

// t4716.scala
trait Bug2[ +A] extends TraversableOnce[A] {
  def ++[B >: A](that: TraversableOnce[B]) = {
    lazy val it = that.toIterator
    it
  }
}

I'm going to park this ticket again.

@SethTisue
Copy link
Member

SethTisue commented Jul 19, 2022

This'd be a great project for somebody.

It has increased significance lately given that Scala 3 requires the explicit result type:

scala> implicit def x = 3
-- Error: ----------------------------------------------------------------------
1 |implicit def x = 3
  |             ^
  |           result type of implicit definition needs to be given explicitly
1 error found

So I would say that -Xlint should warn about this by default (since including explicit result types has been known at least since 2011 to be good practice), and -Xsource:3 should turn that warning into error.

Is there a volunteer who would like to tackle it?

@sjrd
Copy link
Member

sjrd commented Jul 19, 2022

To whoever would like to take this on, note that the warning should not apply to local definitions, i.e., those for which sym.isLocalToBlock is true. Such definitions are only visible after they're defined, and hence doing cause problematic cycles. Early experimentation in dotty discovered that they had to be allowed to preserve reasonable code patterns.

@som-snytt
Copy link

This'd be a great project for somebody.

Possible typo for Som, homebody. Or is that a portmanteau?

@som-snytt
Copy link

som-snytt commented Jul 21, 2022

Possible noise reduction if it accepted

implicit def f = foo.asInstanceOf[Bar]
implicit def g = foo: Bar

The first form, in particular, adds information. The second could be transposed.

The current exemption is for override in Scala 3/-Xsource:3.

LuciferYang pushed a commit to apache/spark that referenced this issue Dec 1, 2023
… `Implicit definition should have explicit type`

### What changes were proposed in this pull request?
This PR aims to fix `Implicit definition should have explicit type` in Scala 2.13.

This pr includes:
1. Declaration types for global variables of implicit
2. Add scala.annotation.warn

### Why are the changes needed?

- For implicit global variables without explicit type declaration, will get warnning :
warning: Implicit definition should have explicit type (inferred String) [quickfixable]
- No modifications are required for local variables.
Additionally, to handle cases involving reflection-related types like ClassTag in implicit variables, the [scala.annotation.warn](https://github.com/scala.annotation.warn) annotation is used to suppress the warning.

Furthermore, warnings generated in Spark will be treated as errors:

[error] ... Implicit definition should have explicit type (inferred org.json4s.DefaultFormats.type) [quickfixable]
...
[error]   implicit val formats = org.json4s.DefaultFormats

Jira link:
SPARK-45314: https://issues.apache.org/jira/browse/SPARK-45629

Related issue link about `implicit` :
scala/bug#5265

### Does this PR introduce _any_ user-facing change?

no

### How was this patch tested?

Most of the testing is completed through CI, and the example module is locally compiled and tested in IDEA Additionally, there are some writing changes that are verified through demo code

### Was this patch authored or co-authored using generative AI tooling?

no

Closes #43526 from laglangyue/SPARK-45629.

Lead-authored-by: tangjiafu <jiafu.tang@qq.com>
Co-authored-by: tangjiafu <tangjiafu@corp.netease.com>
Signed-off-by: yangjie01 <yangjie01@baidu.com>
asl3 pushed a commit to asl3/spark that referenced this issue Dec 5, 2023
… `Implicit definition should have explicit type`

### What changes were proposed in this pull request?
This PR aims to fix `Implicit definition should have explicit type` in Scala 2.13.

This pr includes:
1. Declaration types for global variables of implicit
2. Add scala.annotation.warn

### Why are the changes needed?

- For implicit global variables without explicit type declaration, will get warnning :
warning: Implicit definition should have explicit type (inferred String) [quickfixable]
- No modifications are required for local variables.
Additionally, to handle cases involving reflection-related types like ClassTag in implicit variables, the [scala.annotation.warn](https://github.com/scala.annotation.warn) annotation is used to suppress the warning.

Furthermore, warnings generated in Spark will be treated as errors:

[error] ... Implicit definition should have explicit type (inferred org.json4s.DefaultFormats.type) [quickfixable]
...
[error]   implicit val formats = org.json4s.DefaultFormats

Jira link:
SPARK-45314: https://issues.apache.org/jira/browse/SPARK-45629

Related issue link about `implicit` :
scala/bug#5265

### Does this PR introduce _any_ user-facing change?

no

### How was this patch tested?

Most of the testing is completed through CI, and the example module is locally compiled and tested in IDEA Additionally, there are some writing changes that are verified through demo code

### Was this patch authored or co-authored using generative AI tooling?

no

Closes apache#43526 from laglangyue/SPARK-45629.

Lead-authored-by: tangjiafu <jiafu.tang@qq.com>
Co-authored-by: tangjiafu <tangjiafu@corp.netease.com>
Signed-off-by: yangjie01 <yangjie01@baidu.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment