Support JDK 16 records in Java sources

[harpocrates]

JDK16 introduced records (JEP 395) for reducing the boilerplate associated with small immutable classes. This new construct automatically

• makes fields private/final and generates accessors for them
• overrides equals/hashCode/toString
• creates a final class that extends java.lang.Record

The details are in "8.10. Record Classes" of the Java language specification.

Fixes scala/bug#11908

[harpocrates]

Is equalsStructure the right way to check that the method signatures match?

[lrytz]

This is tricky.. The parser-only implementation breaks here:

public record Test(String s) {
  public Test(java.lang.String s) {
    this.s = s;
  }
}

Using it from Scala:

Test.scala:2: error: ambiguous reference to overloaded definition,
both constructor Test in class Test of type (s: String): Test
and  constructor Test in class Test of type (s: String): Test
match argument types (String)
  def t = new Test("")
          ^
1 error

Maybe it's good enough for a first implementation. But to do it right we have to compare the parameter types symbolically, which is a little tricky: it needs to be done early enough, so that type checking a new R() expression finds the constructor, but late enough so that symbolic information is available...

See here for an example:

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

Lines 1215 to 1229 in d974e99

	// add the copy method to case classes; this needs to be done here, not in SyntheticMethods, because
	// the namer phase must traverse this copy method to create default getters for its parameters.
	// here, clazz is the ClassSymbol of the case class (not the module). (!clazz.hasModuleFlag) excludes
	// the moduleClass symbol of the companion object when the companion is a "case object".
	if (clazz.isCaseClass && !clazz.hasModuleFlag) {
	val modClass = companionSymbolOf(clazz, context).moduleClass
	modClass.attachments.get[ClassForCaseCompanionAttachment] foreach { cma =>
	val cdef = cma.caseClass
	def hasCopy = (decls containsName nme.copy) || parents.exists(_.member(nme.copy).exists)
	// scala/bug#5956 needs (cdef.symbol == clazz): there can be multiple class symbols with the same name
	if (cdef.symbol == clazz && !hasCopy)
	addCopyMethod(cdef, templateNamer)
	}
	}

[harpocrates]

Right. This is exactly what I was worried about. Thank you for the pointer to similar code.

I'm a little concerned that the perfect solution won't be possible, but I'll try. If it isn't, I think we should lean towards assuming the constructor is different. It would be undesirable to assume the constructor is the same in a case like the following:

public record Test(String s) {
  public Test(some.other.fully.qualified.String s) {
    this.s = s;
  }
}

Unlike the case where we incorrectly mark String and java.lang.String as different, incorrectly assuming String and some.other.fully.qualified.String are the same isn't fixable from the Java source without renaming some.other.fully.qualified.String (something which might not even be possible if it comes from another package).

[SethTisue]

At present, our CI only runs on 8, but maybe this should be the kicker that prompts us to finally address that.

scala/scala-dev#751 has some context — we've been slow on moving forward on this mostly because of indecision about the bigger CI picture

[SethTisue]

#9579 has landed resolving the CI situation, but now this is waiting on #9587 to land

[harpocrates]

I've rebased to take advantage of the handy javaVersion: 16+ test filters added in #9587. The tests pass for me locally on both JDK 8 (where the new tests are skipped) and JDK 16.

[lrytz]

Looks very good already, thank you! Pushed a few review observations, and there's one concern about the generated canonical constructor.

[lrytz]

This is tricky.. The parser-only implementation breaks here:

public record Test(String s) {
  public Test(java.lang.String s) {
    this.s = s;
  }
}

Using it from Scala:

Test.scala:2: error: ambiguous reference to overloaded definition,
both constructor Test in class Test of type (s: String): Test
and  constructor Test in class Test of type (s: String): Test
match argument types (String)
  def t = new Test("")
          ^
1 error

Maybe it's good enough for a first implementation. But to do it right we have to compare the parameter types symbolically, which is a little tricky: it needs to be done early enough, so that type checking a new R() expression finds the constructor, but late enough so that symbolic information is available...

See here for an example:

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

Lines 1215 to 1229 in d974e99

	// add the copy method to case classes; this needs to be done here, not in SyntheticMethods, because
	// the namer phase must traverse this copy method to create default getters for its parameters.
	// here, clazz is the ClassSymbol of the case class (not the module). (!clazz.hasModuleFlag) excludes
	// the moduleClass symbol of the companion object when the companion is a "case object".
	if (clazz.isCaseClass && !clazz.hasModuleFlag) {
	val modClass = companionSymbolOf(clazz, context).moduleClass
	modClass.attachments.get[ClassForCaseCompanionAttachment] foreach { cma =>
	val cdef = cma.caseClass
	def hasCopy = (decls containsName nme.copy) || parents.exists(_.member(nme.copy).exists)
	// scala/bug#5956 needs (cdef.symbol == clazz): there can be multiple class symbols with the same name
	if (cdef.symbol == clazz && !hasCopy)
	addCopyMethod(cdef, templateNamer)
	}
	}

[lrytz]

Another lower-priority TODO would be the handling of annotations, which are copied to the synthesized fields / accessors / constructors. But I'm totally fine leaving that for now.

[harpocrates]

Thank you for the thorough review! I'll go back to see what I can do about

• doing a better job of detecting when there is a constructor with the right signature
• handling annotations on synthesized fields / accessors / constructors (I could've sworn I handled that in at least one of those cases)

Fair warning: this week is quite busy, so I may not be able to address the feedback too promptly. 😰

[lrytz]

👍

Actually, instead of the copy method, you better look at how toString and similar are added to case classes. copy is overly complicated because of default arguments.

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

Line 395 in 089bad7

for ((m, impl) <- methods ; if shouldGenerate(m)) yield impl()

[dwijnand]

@harpocrates we would need to get this merged this week in order for it to be in 2.13.6 (although it might be too late already, given the domain)

[harpocrates]

I'll address the review feedback later today - sorry for letting this languish. If it gets into 2.13.6, great. If not, then there's always 2.13.7. 😄

[SethTisue]

@harpocrates interested in returning to this...?

[SethTisue]

@harpocrates there was an inquiry about this at https://contributors.scala-lang.org/t/scala-community-build-on-jdk-17/5153/3 . would you like assistance getting it over the finish line...?

[harpocrates]

@SethTisue sorry for letting this languish! I've fixed (I think) the issue around annotations on generated methods. All that remains is the issue that @lrytz raised about the canonical generated constructor. For that, I do need some help...

Reading through the links above for what Scala does elsewhere in similar situations, I think I'm supposed to use ClassMethodSynthesis#typeInClazz for this. However, in order to construct a ClassMethodSynthesis, I need a Typer. This is where I get lost, since there is nothing like that in JavaParsers (which I think makes sense - this is the first Java construct I know of where you need to do name resolution before you can find out if a member is generated).

[lrytz]

After trying for a while, I'm not sure what is the best approach to handle user-defined primary constructors. To recap, the difficulty is:

public record A(String i) {
  public A(java.lang.String i) { this.i = i; }
}

In the parser we can't tell that the user-written constructor is the primary constructor. So we need to do something later when symbols and types are available, but early enough so that new A("") will type check (lookup has to return one matching constructor).

My first thought was to generate the constructor later. The difficulty is that we have to somehow come up with its signature during namer, but the class parameters not available in the AST, the parser usually just generates a primary constructor. So we'd have to store them in an attachment or somesuch, and they'd be untyped.

My current proposal is to generate the constructor in the parser unconditionally, and then, in case its signature matches another constructor, unlink its symbol in Namer once its type gets completed. I pushed this change.

[lrytz]

This is ready and LGTM. @retronym could you give it a review?

[retronym]

Great work. The parser/namer interplay is tricky but seems like the best solution to me. I've pushed some minor changes as an extra commit.

[SethTisue]

note that the corresponding Scala 3 PR is scala/scala3#16762 ; it has been merged and will appear in 3.3.1 (it's in 3.3.1-RC5 already)