Zinc doesn't handle constants (final val) correctly

[jvican]

@gkossakowski explains why here. This is a port from an old Zinc issue. The issue is that scalac inlines constants too early, and the incremental compiler doesn't see the dependency on the constants because no reference is left.

In essence, the fix for this one has to happen in scalac land. The germane JIRA ticket is SI-7173. Adriaan already tried to fix it in this PR but it was closed for timing issues (I guess). Future solutions should build on his approach, which is the most likely to be fixed in Scalac. Jason has also done some work along these lines, see Adriaan's PR.

[jvican]

Okay, I propose a partial solution to this problem (which I have implemented and will push soon). Since typer is inlining constants trees in current Scala and this is unlikely to change in the short term, I suggest that we keep track of constant types by extracting their stringValue and registering them.

Here's how I've done it. I added the following case to the traverse method inExtractUsedNamesTraverser:

      case valDef: ValDef =>
        // Recognise final constants at type level
        if (valDef.symbol.hasFlag(Flags.FINAL)) {
          valDef.tpt.tpe match {
            case ConstantType(value) =>
              /* HACK: Register known constants at the type level since current
               * Scala typer inlines constants at the tree level. This does not
               * cover all the cases, see #227 for more information. */
              val names = getNamesOfEnclosingScope
              val constantName = NoSymbol.name.newName(value.stringValue)
              if (!isEmptyName(constantName) && !names.contains(constantName))
                names += constantName
            case _ => ()
          }
        } else ()

As you see, we're only interested in constants defined by ValDef.

This solution is partial because it does not handle the case where typer widens types on its own. This happens when we have a constant type on the right-hand side of a ValTree and a "super", more general type on the left-hand side, in which case typer unifies them. The following code snippet illustrates the cases where it does and doesn't work:

object Foo {
  final val foo = 12 // works
  final val foo: Int = 12 // doesn't work, typer widens type for declaration to `Int`
}

Note that if the definition of foo is inherited from a super class and is abstract, widening will also occur.

With this workaround, we can keep track of constants partially in Zinc. If changes to constants are not detected, remove the type ascription in the left-hand side of the val definition and you're good to go. I believe this will cover the majority of cases of constants use, since the case where a constant definition is inherited is not common (most of the times, constants are defined in the same class where they are used).

[jvican]

Okay, I've declared victory too soon.

My previous approach missed the real point: it's at the use site when we don't know that a constant type belongs to a definition in another source. Ha. Failed.

However, I've had two ideas to fully work around this limitation.

First approach

There is indeed one thing that is kept from the original tree in the inlined constant type: the position. This position points to the original expression in which it was used, in the example provided here:

// A.scala
object A {
  final val x = 12
}

// B.scala
object B { 
  def abc: Int = A.x
}

that corresponds to A.x in the body of B.abc.

So, what we can do is to use that position to parse the previous source, get the identifier that introduced the constant and use it for the name hashing algorithm. You will think: that's the most terrible hack I've ever heard. Yeah, but it works. I'm familiar with this approach because that's how the Scala Meta semantic API works and, to some extent, Scalafix.

This could indeed cause some trouble in paradise, so what we should do is to enable this under a experimental flag, just to protect ourselves. But I think it's interesting and worth a try. I don't really expect any issue with this approach, there can only be issues in general and intricate cases that try to cover all the use cases, but we know for a fact that inlined trees keep their position (because that's what the implementation does). So, honestly, the only reason why I would put this under a flag is because of the increased cost in performance (since now we have to parse a source file and identify what's the name we should register).

Second approach (best one)

Another thing that we can do is to register another phase before typer that goes through all the final vals and registers their position in a cache, that is later read by ExtractAPI to match the positions with the inlined constant types. This is probably the best strategy and the performance cost will be low. I will experiment with it at some point in the next days. For now, I'm putting this on the back burner.

[romanowski]

@jvican I think that second approach can be also used for inspecting original form of e.g. pattern matching and as you said cos will be marginal (especially that for constants we don't need to traverse e.g. bodies of functions).

How do you plan to handle final vals from external dependencies/libraries?

I also see another problem here: We need to hash constants differently (hash value alongside type) since if constant value changes all places where it was inlined needs to be recompiled.

[jvican]

Yes, the second approach is super general, and that's why it's the best. We can use it to do practically everything. I'll experiment with it and devote it a longer explanation, as it deserves it.

What do you mean by final vals from external dependencies? I don't get you.

Yes, of course, handling final vals will require changes to API if we're not recording values of (private) final vals. I'm not sure we're not though, I'll look into it.

[romanowski]

External dependency is entry on classpath for which we can provide analysis (e.g. project in sbt that we depends on).

[jvican]

But in that case the dependency is explicit, no? Typer doesn't get to inline the constants because it's not in the same compilation unit and doesn't have the tree information, only symbol information. Or am I misunderstanding you?

[dwijnand]

#267 "Implement used scopes for used names and implement PatMat scope" (now merged!) states

In future this mechanism can be used in #227 #226

So I guess this is no longer "blocked"?

[jvican]

No, this is blocked. I already explained that in the zinc Dev Gitter channel. #226 has no relation with this.

[dwijnand]

I'm saying #267 bears relevance here, not #226. Please could you summarise here why this is still blocked?

[jvican]

My point was that #267 bears no relevance here. I explain it here.

This is blocked by this ticket in Scala Dev. Unfortunately, until we don't have a generic way to keep track of desugarings in typer, we cannot solve this issue. I explain the rationale in the ticket, but in a nutshell, doing so would require to hijack analyzer (namer, packageobject and typer phases) and would make Zinc fail on any library using macros or Scala Meta (the only two compiler plugins I'm aware of that do the same).

The reason why we need to hijack typer is because Scalac does not allow you to register new phases before typer and after namer.

Zinc needs to be built with the guarantee that it works in any Scala codebase, so this is not an option.

[jvican]

I have PR'd a change to scala/scala targetting 2.12.4: scala/scala#6013. If that gets in, we'll be able to handle constants in Zinc. I'll follow up with a PR here to make sure we do indeed handle them.

[jvican]

I did a PR for this, but I'm delaying it until 2.13 because it causes complications on the way we publish the bridges for Scala 2.x versions, and requires us to publish two bridges for 2.12.x if we want to support 2.12.x. I don't think this cost is worth it.

[eed3si9n]

This was fixed by @ephemerist in #985, but I commented out the compiler bridge for Scala 2.12 because 2.12.0~2.12.2 doesn't have the OriginalTree, and @dwijnand and @retronym patched it in #997.