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

Repeated parameter typed as T* rather than Seq[T] #4176

Closed
scabug opened this issue Jan 20, 2011 · 19 comments
Closed

Repeated parameter typed as T* rather than Seq[T] #4176

scabug opened this issue Jan 20, 2011 · 19 comments
Assignees
Milestone

Comments

@scabug
Copy link

@scabug scabug commented Jan 20, 2011

=== What steps will reproduce the problem ===

scala> def id[A](as: A*) = as
id: [A](as: A*)A*

scala> id[Int] _
res12: (Int*) => Int* = <function1>

scala> res12(1) 
res13: Int* = WrappedArray(1)

scala> res12(Nil :_*)
res14: Int* = List()

=== What is the expected behavior? ===

According to the spec:

"4.6.2 Repeated Parameters

The last value parameter of a parameter section may be suffixed by “*”, e.g. (..., x:T*). The type of such a repeated parameter inside the method is then the sequence type scala.Seq[T]"

Firstly, the inferred type of ids should be Seq[A].

Secondly, the eta expansion of ids[Int] _ should result in (Seq[Int]) => Seq[Int])

=== What do you see instead? ===

The internal type of the repeated parameter leaks out; and, via eta expansion, results in a Function1 with def apply(v1: T*).

=== Additional information ===

Such behaviour was explicitly excluded in #3652 and #3180.

=== What versions of the following are you using? ===

  • Scala: 2.8.1
@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Jan 20, 2011

@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Jan 24, 2011

@paulp said:
The types do become what they ought to be, but it doesn't happen until uncurry, after which the types have already been pickled with A* in there. (The repl shows the types right after typer.) Not sure how to address.

@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Jan 24, 2011

@retronym said:
Okay, that explains the behaviour.

There is some commented out code in the typechecker relevant to this problem. Looks like the Eta expansion used to prevented:

http://lampsvn.epfl.ch/trac/scala/browser/scala/trunk/src/compiler/scala/tools/nsc/typechecker/Typers.scala?annotate=blame&rev=24068#L298

As pf r12607, this check was commented out. The commit comment isn't illuminating -- #1269 is a worksforme and a cursory search over the interwebs doesn't reveal the origin or intention of (Stephan?) Kolstov's patch.

For the benefit of future ticket readers, here's the problem without the REPL. If scalac followed the letter of the spec, f(1) would not be allowed.

E:\code\scratch\4176>cat 4176_1.scala
object test1 {
  def id[A](as: A*) = as
  val f = id[Int] _
}
E:\code\scratch\4176>cat 4176_2.scala
object test2 {
  def main(args: Array[String]) {
     val f = test1.f
     f(1)
     f(Nil: _*)
  }
}
E:\code\scratch\4176>scalac -d out 4176_1.scala

E:\code\scratch\4176>scalac -classpath out -d out 4176_2.scala

E:\code\scratch\4176>scala -classpath out test2

E:\code\scratch\4176>scalac -classpath out -d out -Xprint:typer 4176_2.scala
[[syntax trees at end of typer]]// Scala source: 4176_2.scala
package <empty> {
  final object test2 extends java.lang.Object with ScalaObject {
    def this(): object test2 = {
      test2.super.this();
      ()
    };
    def main(args: Array[String]): Unit = {
      val f: (Int*) => Int* = test1.f;
      f.apply(1);
      {
        f.apply((immutable.this.Nil: _*));
        ()
      }
    }
  }
}
@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Jan 24, 2011

@retronym said:
Okay, last bit of archeology: r6634

@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Jan 26, 2011

@adriaanm said:
first step in fixing this: https://github.com/adriaanm/scala/tree/2d61c08

I think adapt should turn repeated paramtypes into the corresponding type after uncurry since the spec says 'The type of such a repeated parameter inside the method is then the sequence type scala.Seq[T]'.

I don't know what to derive from this concerning the type of the argument of the method and its eta-expansion since these are outside of the method, and the spec fragment talks about inside the method.

@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Jan 26, 2011

@retronym said:
I would just outlaw eta expansion for these, and force use of an anonymous function instead.

@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Jan 26, 2011

@adriaanm said:
I would like to as well (some more testing indicates this is going to require mad marshland navigator skills), but Martin said we can't take eta-expansion of these methods away anymore -- it would break existing code. (But who, besides mr. Koltsov, would do such a thing!?).

Maybe we could deprecate it in 2.9? Re-assigning to scala meeting to re-open the discussion.

@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Jan 26, 2011

@paulp said:
Replying to [comment:3 retronym]:

As pf r12607, this check was commented out. The commit comment isn't illuminating -- #1269 is a worksforme and a cursory search over the interwebs doesn't reveal the origin or intention of (Stephan?) Kolstov's patch.

Tip from a seasoned, weathered, barely hanging on scala archaeologist: if the ticket number doesn't make any sense odds are it's in the pre-trac bug database, which is archived online. Here is #1269.

http://www.scala-lang.org/sites/default/files/aladdin/displayItem.do%3Fid=1269.html

It was opened 8/8/07 and the commit is 8/21/07 and logic takes us the rest of the way.

@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Feb 22, 2011

@odersky said:
I should never have allowed that patch at the time. I had a bad feeling about it, because it was checked in without changing the spec. I have since then become much stricter in that respect. The problem now is that we have to make illegal some code that worked before. That's always a mine field. You never know what will blow up when you do that.

I'll change the inferred result type. But somebody else needs to follow up on the eta expansion, or we leave it as a won't fix. I simply do not have the bandwidth anymore to follow breakages in user code which will inevitably happen when we do this.

@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Jun 24, 2011

@paulp said:
See #2210 for another manifestation.

@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Jun 24, 2011

@paulp said:
And see #3229 for yet another.

@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Jul 3, 2011

@paulp said:
I (locally) fixed the inferred type and the eta-expansion, as far as I know. Anyone want to suggest what I've overlooked, given the following transcript?

scala> def id[A](as: A*) = as
id: [A](as: A*)Seq[A]

scala> id[Int] _
res0: Seq[Int] => Seq[Int] = <function1>
@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Jul 3, 2011

@paulp said:
Granted, I don't know what martin means by "follow up on the eta expansion." If the priority is not to break code, then it should be A* => Seq[A], not Seq[A] => Seq[A].

@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Jul 3, 2011

Commit Message Bot (anonymous) said:
(extempore in r25227) Modified return type inference not to allow T* to leak from varargs
methods. Since I don't know what is supposed to be done about eta
expansion of these methods, I left the behavior as it was (except the
return type) and a boolean val in Types to change it.

def id[T](xs: T*) = xs
etaExpandKeepsStar = true // (id[Int] _) is Int* => Seq[Int]
etaExpandKeepsStar = false // (id[Int] _) is Seq[Int] => Seq[Int]

References #4176, leaving open pending resolution of eta expansion.
Review by odersky.

@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Sep 6, 2011

@paulp said:
Going to make it Seq[A] => Seq[A] and leave an option to A* it just in case.

@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Oct 9, 2011

Commit Message Bot (anonymous) said:
(extempore in r25809) Flipped varargs eta-expansion behavior.

(T*)U now eta-expands to Seq[T] => U, not T* => U. There is a
transition command line switch to get the old behavior for any who
may have relied upon it:

-Yeta-expand-keeps-star

Closes #4176, no review.

@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Jun 4, 2012

@retronym said:

Welcome to Scala version 2.10.0-20120504-065643-e52be82eef (Java HotSpot(TM) 64-Bit Server VM, Java 1.6.0_31).
Type in expressions to have them evaluated.
Type :help for more information.

scala> def foo(a: String*) = a
foo: (a: String*)Seq[String]

scala> foo _
res0: Seq[String] => Seq[String] = <function1>

scala> (foo: Seq[String] => Seq[String])
<console>:9: error: type mismatch;
 found   : String* => Seq[String]
 required: Seq[String] => Seq[String]
              (foo: Seq[String] => Seq[String])
               ^
@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Jun 24, 2012

@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Jul 1, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants
You can’t perform that action at this time.