Skip to content

Class constructor parameter undercompilation #19910

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

Closed
sake92 opened this issue Mar 9, 2024 · 4 comments · Fixed by #19911
Closed

Class constructor parameter undercompilation #19910

sake92 opened this issue Mar 9, 2024 · 4 comments · Fixed by #19911

Comments

@sake92
Copy link

sake92 commented Mar 9, 2024

Compiler version

3.3.1

Minimized code

// Hello.scala
object Hello extends App {
  val service = MyService("fssdfs")
  println("Works!")
}

// MyService.scala
class MyService(str: String)

compiles and runs fine.

But when you add a parameter to MyService constructor, it still compiles but fails at runtime with NoSuchMethodError.

Output

Exception in thread "main" java.lang.NoSuchMethodError: 'void example.MyService.<init>(java.lang.String)'
        at example.Hello$.<clinit>(Hello.scala:5)
        at example.Hello.main(Hello.scala)

Expectation

Code fails to compile.

@sake92 sake92 added itype:bug stat:needs triage Every issue needs to have an "area" and "itype" label labels Mar 9, 2024
@sake92
Copy link
Author

sake92 commented Mar 9, 2024

@eed3si9n
Copy link
Member

eed3si9n commented Mar 9, 2024

Likely the relevant code is around here where Scala 3 compiler builds up the dependency graph:

override def traverse(tree: Tree)(using Context): Unit = try {
tree match {
case Match(selector, _) =>
addPatMatDependency(selector.tpe)
case Import(expr, selectors) =>
def lookupImported(name: Name) =
expr.tpe.member(name).symbol
def addImported(name: Name) = {
// importing a name means importing both a term and a type (if they exist)
addMemberRefDependency(lookupImported(name.toTermName))
addMemberRefDependency(lookupImported(name.toTypeName))
}
for sel <- selectors if !sel.isWildcard do
addImported(sel.name)
if sel.rename != sel.name then
rec.addUsedRawName(sel.rename)
case exp @ Export(expr, selectors) =>
val dep = expr.tpe.classSymbol
if dep.exists && selectors.exists(_.isWildcard) then
// If an export is a wildcard, that means that the enclosing class
// has forwarders to all the applicable signatures in `dep`,
// those forwarders will cause member/type ref dependencies to be
// recorded. However, if `dep` adds more members with new names,
// there has been no record that the enclosing class needs to
// recompile to capture the new members. We add an
// inheritance dependency in the presence of wildcard exports
// to ensure all new members of `dep` are forwarded to.
val depContext = depContextOf(ctx.owner.lexicallyEnclosingClass)
rec.addClassDependency(dep, depContext)
case t: TypeTree =>
addTypeDependency(t.tpe)
case ref: RefTree =>
addMemberRefDependency(ref.symbol)
addTypeDependency(ref.tpe)
case t: Closure =>
addInheritanceDependencies(t)
case t: Template =>
addInheritanceDependencies(t)
case _ =>
}

And here's how you can see the AST after parser phase:

$ mkdir /tmp/hello
$ scalac scalaHelloWorld/src/main/scala/example/*.scala -d /tmp/hello -Xprint:parser -Yplain-printer
[[syntax trees at end of                    parser]] // scalaHelloWorld/src/main/scala/example/Hello.scala
PackageDef(Ident(example), List(
  ModuleDef(Hello,
    Template(DefDef(<init>, List(), TypeTree(), Thicket(List())),
      List(Ident(App)), ValDef(_, Thicket(List()), Thicket(List())), List(
      ValDef(service, TypeTree(),
        Apply(Select(New(Ident(MyService)), <init>), List(Literal("fssdfs")))),
      Apply(Ident(println), List(Literal("Works!")))))
  )
))

[[syntax trees at end of                    parser]] // scalaHelloWorld/src/main/scala/example/MyService.scala
PackageDef(Ident(example), List(
  TypeDef(MyService,
    Template(
      DefDef(<init>, List(List(ValDef(str, Ident(String), Thicket(List())))),
        TypeTree(), Thicket(List())),
    List(), ValDef(_, Thicket(List()), Thicket(List())), List())
  )
))

In particular Apply(Select(New(Ident(MyService)), <init>), List(Literal("fssdfs")))).

@eed3si9n
Copy link
Member

Here's my PR for this - #19911

@Gedochao Gedochao added area:incremental-compilation and removed stat:needs triage Every issue needs to have an "area" and "itype" label labels Mar 12, 2024
@bishabosha
Copy link
Member

Thank you @sake92 for reporting and @eed3si9n for the investigation and PR!

bishabosha added a commit that referenced this issue Mar 17, 2024
Fixes #19910
Fixes sbt/zinc#1334

## Problem
Scala 3 compiler registers special `zincMangledName` for constructors
for the purpose of incremental compilation. Currently the
`zincMangledName` contains the package name, which does not match the
use site tracking,
thereby causing undercompilation during incremental compilation after a
ctor change, like adding a parameter.

There is an existing scripted test, but coincidentally the test class
does NOT include packages, so the test ends up passing.

## Solution
1. This PR reproduces the issue by adding package name to the test.
2. This also fixes the problem by changing the `zincMangedName` to
`sym.owner.name ++ ";init;"`.

## Note about zincMangedName
`zincMangedName` in general is a good idea, which adds the class name
into otherwise common name `<init>`.
By making the name more unique it prevents overcompilation when one of
the ctors changes in a file.
@Kordyjan Kordyjan added this to the 3.4.2 milestone Mar 28, 2024
@Kordyjan Kordyjan modified the milestones: 3.4.2, 3.5.0 May 10, 2024
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.

5 participants