SI-10097 Error if no non-implicit case class param

[som-snytt]

Case class must have a non-implicit param list.

Error early, error often.

JIRA: https://issues.scala-lang.org/browse/SI-10097

[som-snytt]

Destructuring in reflection also makes assumption about empty first param list that must be tweaked for case class. UnMkTemplate must be case-class aware. That changes tree printing.

[szeiger]

The spec doesn't mention any restrictions on case class parameter lists. I think they should be added. (This also goes for the existing restriction on requiring a parameter list at all)

[som-snytt]

git push to clean this syntax is hanging for some reason. Try again later.

[lrytz]

Looks very good. I think the last commit should be an error, and not only a warning under a flag. Can we change the parser to make a parameter list implicit also if the implicit modifier is not the first? The following should be an error IMO (also the test case you added):

scala> class Ann extends annotation.Annotation
defined class Ann

scala> class D(@Ann implicit val i: Int)(implicit s: String)
defined class D

Finally, I think we should be careful to introduce this (require () for case classes) into a minor release, it should probably target 2.13.x

[som-snytt]

I think the bug is typer taking a param section as implicit if the first param is implicit.

That's why (as an afterthought) I put the noise about "effectively multiple" under a flag; I also considered -Xlint. This is really an implementation restriction and not in the spec.

paulp said on gitter that implicit mod on a param is a non-feature. But people like { implicit x => } syntax. It occurs to me that self-type syntax could work: class C(c: Int) { self(implicit c) => } with the goal of avoiding turning the param into a field such as: class C(c0: Int) { private implicit val c = c0; }. For a function param, an extra local is more benign. def f(c0: Int) { implicit val c = c0; ... }.

But maybe that is syntax in the service of a non-feature.

For 2.12 migration of missing non-implicit case class params, it can silently add the empty param section, but error under -Xfuture. But it should probably warn with deprecation.

One of the tests used a space to indicate a missing param list. Since the syntax showed up in a couple of tests, probably it exists in the wild (and not just in Bippy).

[lrytz]

OK, I should have checked the spec earlier. I didn't realize that implicit is a valid modifier for class parameters, and if it doesn't appear at the beginning of the parameter list, it just applies to one specific parameter.

You say in one of the commit messages:

Current semantics are that leading implicit param turns the parameter section into an implicit section (though without making other params implicitly implicit).

That sounds like a bug, shouldn't we fix this instead?

Instead of -Xfuture, should we use -Xsource:2.13 so that things will just work when the future arrives?

In the REPL i get two warnings:

scala> case class C(implicit i: Int)
<console>:1: warning: case classes should have a non-implicit parameter list; adapting to 'case class C()(...)'
case class C(implicit i: Int)
            ^
<console>:11: warning: case classes should have a non-implicit parameter list; adapting to 'case class C()(...)'
       case class C(implicit i: Int)
                   ^

Also the warning text adapting to 'case class C()(...)' is not entirely accurate:

scala> C()(1).productIterator.toList
res0: List[Any] = List(1)

scala> case class C()(implicit i: Int)
defined class C

scala> C()(1).productIterator.toList
res1: List[Any] = List()

[som-snytt]

The perils of relying on slave labor:

Slave went offline during the build
ERROR: Connection was broken: java.io.IOException: Unexpected termination of the channel
	at hudson.remoting.SynchronousCommandTransport$ReaderThread.run(SynchronousCommandTransport.java:50)
Caused by: java.io.EOFException
	at java.io.ObjectInputStream$PeekInputStream.readFully(ObjectInputStream.java:2353)
	at java.io.ObjectInputStream$BlockDataInputStream.readShort(ObjectInputStream.java:2822)
	at java.io.ObjectInputStream.readStreamHeader(ObjectInputStream.java:804)
	at java.io.ObjectInputStream.<init>(ObjectInputStream.java:301)
	at hudson.remoting.ObjectInputStreamEx.<init>(ObjectInputStreamEx.java:48)
	at hudson.remoting.AbstractSynchronousByteArrayCommandTransport.read(AbstractSynchronousByteArrayCommandTransport.java:34)
	at hudson.remoting.SynchronousCommandTransport$ReaderThread.run(SynchronousCommandTransport.java:48)

[som-snytt]

/rebuild

[retronym]

IntelliJ seems to implement automatic insertion of the empty param lists (consistently with non-case classes), and gets the copy method and absence of accessor method correct.

If we go ahead with this change, /cc @Alefas to add the deprecation to IntelliJ 2.12 and the error to IntelliJ 2.13.

[retronym]

However, I'm not sure that we should treat case and non-case classes differently like this. We could just insert the empty parameter list, like we do for regular classes, and make sure that the other case class machinery is aware of this.

Maybe we can argue in favor of the the inconsistency because of fact that we're just fixing an existing restriction:

scala> case class Foo
<console>:1: error: case classes without a parameter list are not allowed;
use either case objects or case classes with an explicit `()' as a parameter list.
case class Foo
              ^

scala> case class Foo()
defined class Foo

Rather than adding a new one.

With that in mind:

LGTM

[som-snytt]

IIUC, you mean the transitional behavior of assuming empty parens for case class K(implicit k: K) should be the default, without warning. I think the combination of Lukas's suggestion to push the error to 2.13 plus the intuition that K is probably an error (either oversight or misunderstanding) led to this proposal.

If K looks natural, then I agree that just supplying parens is sufficient. But then shouldn't case class C also supply parens? In 2017, parameterless is no more naive than non-implicitless. It used to look naive because the community was still coming to terms with what a case class can do. (Such as inheritance.)

I'll push whichever warning the Scala team prefers. I guess that's why the Scala team always has an odd number of members, so Martin needn't break any ties. (Including personal ties.)

[retronym]

In the end I'm happy to impose the new restriction, I was thinking out loud.

[Alefas]

@retronym I created new ticket for us: https://youtrack.jetbrains.com/issue/SCL-11169
Big thanks for the notice.

[som-snytt]

There's an issue for "where implicit appears in front of first class param" (which lrytz suggests fixing above):

https://issues.scala-lang.org/browse/SI-9494

There's also SIP for multi-implicits:

scala/docs.scala-lang#520

[milessabin]

I agree that SI-10097 needs fixing, but I'm unclear why this is the correct fix. Why isn't an implicit parameter list considered to be the parameter list required for a case class?

[SethTisue]

Why isn't an implicit parameter list considered to be the parameter list required for a case class?

you really want to do that? (or think someone else would?)

[som-snytt]

One justification is so that a "constructor pattern" looks like a constructor invocation.

The abstract case class trick lets you control apply but not unapply. Maybe an ergonomic solution to that would satisfy the implicit case elements use case.

A second justification might be that capturing implicits as vals is unexpected?

[som-snytt]

@lrytz The extra warnings were from parsing, SI-10130 #5647

[som-snytt]

Another case class anomaly is case class C(private[this] val c: Int). That's SI-1422. It was closed for pragmatic reasons. Since accessors are synthesized for private members, for purposes of pattern matching, it should also be OK for object private. So case classes deviate from ordinary classes in ways that may or may not be justified.

[milessabin]

On reflection, an implicit first (and only) parameter list for a case class isn't particularly useful because the bare name will always be interpreted as a reference to the companion object.

So, fine by me.

[SethTisue]

the Scala team always has an odd number of members

we also have a number of odd members.