`DisableSyntax.NoFinalVal` needs to take into account the context

[BalmungSan]

trafficstars

I totally understand that the old constant-inlining in Scala 2 has some problems, and that Scala 3 inline is better. I also, agree that a final val inside an object or final class is redundant. However, unless I am missing something like a compiler bug or bad practice (and if that is the case I would highly appreciate the feedback), I think there are valid use cases for making a final val.

I will share the situation in particular that we faced today, but I think the root use case is common enough.

// We have a trait defined in a third-party library.
// Which, among other things, defines a resources abstract method for users to override.
trait ExternalTrait {
  protected type Resources
  protected def resources: Resource[IO, Resources]
}

// And we want to define a custom trait in a company-internal library.
// Which, among other things, abstracts the initialization of the common resources our users need.
trait CompanyTrait extends ExternalTrait {
  override protected final type Resources = CompanyResources
  override protected final val resources: Resource[IO, Resources] = ...
}

// Finally, our users MUST not be able to override our setup.
final class UserClass extends CompanyTrait {
  // resources is accessible here, but not modifiable.
}

This, of course, triggers thenoFinalVal warning / error.

For now, we work around the problem using override protected final def which is fine (and Arman will actually recommend doing that because of performance). But, we wonder if the rule is just too strict (since this feels like a normal use case in OOP) or if using final val in this context is still problematic and thus should be avoided.

[bjaglin]

Hi @BalmungSan!

However, unless I am missing something like a compiler bug or bad practice (and if that is the case I would highly appreciate the feedback), I think there are valid use cases for making a final val.

Did you see the (badly escaped) comment about the configuration toggle in the docs?

Report error on final modifier for val definitions, see motivation

My understanding after digging into this is that final val can cause false positives cache hits during incremental compilation with Zinc (and probably other tools), specifically for old scala 2.12 versions (<2.12.4) or old sbt versions (<1.6.0). Although now a very rare scenario, I believe it still has some value.

Since it's in opt-in and documented, I don't see any problem keeping it. What do you suggest? We could issue a warning if that rule is used with recent scala (ignoring the sbt/zinc version since it's not available to rules) - happy to take PRs on this.

[BalmungSan]

@bjaglin Yeah I read the thread. To my understanding, the problem is related to constant-inlining. And, AFAIK, for constant-inlining to happen the final val MUST not have an explicit type annotation. Plus, not sure if constant-inlining is a thing in an abstract context like a trait, all examples I have seen of it have always been in an object; nevertheless even if it can happen in a trait, then the presence of an explicit type annotation in my example should avoid that. Finally, it seems this bug should not affect any recent patch version of 3.3.x, 2.13.x, 2.12.x (not sure about 2.11.x tho), which IMHO is something to consider given that there should be no reason not to upgrade to the latest patch; anyways, even if you want to be conservative about this, again, this should only be a problem if the final val does not have an explicit type parameter.

Thus, my rationale is that in the example I shared we were not doing something wrong. And we would like not to disable the rule in general. So, not sure if the rule could be extended to check if the final val has an explicit type annotation. Of course, there is always the possibility that my understanding is flawed either because I can't read or that there is a more recent discussion that I missed.

[bjaglin]

the rule could be extended to check if the final val has an explicit type annotation

Thanks for clarifying. I am no expert either, but I agree it makes sense to relax the rule when an explicit type is present :+1:

I don't think I will personally prioritize this as the amount of users for which enabling DisableSyntax.NoFinalVal is useful is limited, but happy to accept PRs!