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

Can't call scala.StringBuilder#length_= in 2.13.0-M5 #11158

Closed
xuwei-k opened this issue Sep 15, 2018 · 9 comments · Fixed by scala/scala#10357
Closed

Can't call scala.StringBuilder#length_= in 2.13.0-M5 #11158

xuwei-k opened this issue Sep 15, 2018 · 9 comments · Fixed by scala/scala#10357

Comments

@xuwei-k
Copy link

xuwei-k commented Sep 15, 2018

https://github.com/scala/scala/blob/v2.13.0-M5/src/library/scala/collection/mutable/StringBuilder.scala#L68

Welcome to Scala 2.13.0-M5 (Java HotSpot(TM) 64-Bit Server VM, Java 1.8.0_181).
Type in expressions for evaluation. Or try :help.

scala> val s = new StringBuilder
s: StringBuilder =

scala> s.length = 42
                ^
       error: reassignment to val

scala> s.length_=(42)
Welcome to Scala 2.12.6 (Java HotSpot(TM) 64-Bit Server VM, Java 1.8.0_181).
Type in expressions for evaluation. Or try :help.

scala> val s = new StringBuilder
s: StringBuilder =

scala> s.length = 42
s.length: Int = 42

🤔

@SethTisue SethTisue added this to the 2.13.0-RC1 milestone Sep 17, 2018
@SethTisue
Copy link
Member

what's the underlying cause?

@xuwei-k
Copy link
Author

xuwei-k commented Sep 17, 2018

minimal reproduce example:

CharSeqJava.java

package example;

public interface CharSeqJava {
  int length();
}

Main.scala

package example

trait CharSeqScala {
  def length: Int
}

class A extends CharSeqJava {
  override def length: Int = 1
  def length_=(value: Int): Unit = {}
}

class B extends CharSeqScala {
  def length: Int = 1
  def length_=(value: Int): Unit = {}
}

class C {
  def length: Int = 1
  def length_=(value: Int): Unit = {}
}

class D extends CharSeqJava with CharSeqScala {
  def length: Int = 1
  def length_=(value: Int): Unit = {}
}

class E extends CharSeqScala with CharSeqJava {
  def length: Int = 1
  def length_=(value: Int): Unit = {}
}

object Main {
  val a = new A
  val b = new B
  val c = new C
  val d = new D
  val e = new E

  // compile error
  a.length = 1

  // success
  b.length = 1

  // success
  c.length = 1

  // success
  d.length = 1

  // compile error
  e.length = 1 
}

@xuwei-k
Copy link
Author

xuwei-k commented Sep 17, 2018

Should we swap mixin order? 🤔

 final class StringBuilder(val underlying: java.lang.StringBuilder) extends AbstractSeq[Char]
+  with java.lang.CharSequence 
   with Builder[Char, String]
   with IndexedSeq[Char]
-  with IndexedSeqOps[Char, IndexedSeq, StringBuilder]
+  with IndexedSeqOps[Char, IndexedSeq, StringBuilder] {
-  with java.lang.CharSequence {

@Jasper-M
Copy link
Member

Jasper-M commented Sep 17, 2018

I think we should. Swapping mixin order doesn't seem to be enough...
And emitting a better error for things like this would also be nice.

scala> object foo {
     |   def bar() = 4
     |   def bar_=(a: Int) = println(s"assigning $a")
     | }
defined object foo

scala> foo.bar = 3
<console>:12: error: reassignment to val
       foo.bar = 3
               ^

@adriaanm
Copy link
Contributor

This goes back to scala/scala@b739c4a -- since we're "overriding" a Java method with an empty argument list, we add one to the length method, which then disqualifies it from being a getter (treeInfo.mayBeVarGetter expects a NullaryMethodType, which has no argument lists)

@SethTisue
Copy link
Member

@adriaanm tentatively moved to RC1 milestone, you can make the call though

@adriaanm
Copy link
Contributor

I won't have time for a proper fix. It's kind of tricky :-/

@SethTisue
Copy link
Member

not sure what PR fixed it, but it's been working since 2.13.2

@som-snytt
Copy link

what's the underlying cause?

@inline def length: Int = underlying.length

not sure what PR fixed it, but it's been working since 2.13.2

scala/scala#8558

which instead of fixing getter detection, fixes setter detection for assignment rewrite.

Regression test at scala/scala#10357

Worth adding that there are a few contexts now in which Lukas has said recently (since 2.13) it's time to revert adding a param list to an override of Java (toString()) instead of just handling the override correctly.

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

Successfully merging a pull request may close this issue.

7 participants