Ensure macro changes invalidate downstream classes

[ephemerist]

Given the following code:

object A {
  import scala.language.experimental.macros
  import scala.reflect.macros.blackbox.Context
  def a(c: Context)(): c.Tree = {
    import c.universe._
    q"1"
  }
}
class A {
  import scala.language.experimental.macros
  def a(): Int = macro A.a
}

class B {
  def b(): Int = new A().a()
}        

If q"1" is updated to q"2" then both A and B should be invalidated, but because the API of A hasn't changed, we need to rely on the timestamp to trigger a recompilation of B (in IncrementalCommon.detectAPIChanges). That means it's not safe to zero the timestamp when writing analysis.

[ephemerist]

THIS SOFTWARE IS CONTRIBUTED SUBJECT TO THE TERMS OF THE Organization Contribution License Agreement v2.0, dated July 13, 2012.THIS SOFTWARE IS LICENSED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE AND ANY WARRANTY OF NON-INFRINGEMENT, ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. THIS SOFTWARE MAY BE REDISTRIBUTED TO OTHERS ONLY BY EFFECTIVELY USING THIS OR ANOTHER EQUIVALENT DISCLAIMER IN ADDITION TO ANY OTHER REQUIRED LICENSE TERMS.

[SethTisue]

Looks plausible, but could use a test...? Probably in zinc/src/sbt-test/macros?

[Friendseeker]

For some context we had discussion about the issue in sbt/sbt#7790. This issue is the reason why sbt 1.10.4 make consistent analysis opt-in by default.

[eed3si9n]

This would effectively make the analysis format not consistent, so I don't think this is a good direction. I thought by default Zinc would invalidate any macro calls? /cc @szeiger

[Friendseeker]

By default Zinc would invalidate a macro call iff the compilation timestamp in associated AnalyzedClass before & after mismatches.

zinc/internal/zinc-core/src/main/scala/sbt/internal/inc/IncrementalCommon.scala

Lines 310 to 326 in ced4262

	def classDiff(className: String, a: AnalyzedClass, b: AnalyzedClass): Option[APIChange] = {
	// log.debug(s"[zinc] classDiff($className, ${a.name}, ${b.name})")
	if (a.compilationTimestamp() == b.compilationTimestamp() && (a.apiHash == b.apiHash)) None
	else {
	val hasMacro = a.hasMacro || b.hasMacro
	if (hasMacro && IncOptions.getRecompileOnMacroDef(options)) {
	Some(APIChangeDueToMacroDefinition(className))
	} else if (
	APIUtil.isAnnotationDefinition(a.api().classApi()) ||
	APIUtil.isAnnotationDefinition(b.api().classApi())
	) {
	Some(APIChangeDueToAnnotationDefinition(className))
	} else {
	findAPIChange(className, a, b)
	}
	}
	}

Hence this PR does point out a real undercompilation issue but our discussed solution of using protobuf as default 1.x format and using product hash based invalidation (to replace timestamp based invalidation) in 2.x branch should effectively address the issue.

[Friendseeker]

Looks plausible, but could use a test...? Probably in zinc/src/sbt-test/macros?

We probably need to upgrade our scripted test infrastructure to support analysis format switching. Currently it is only possible to add scripted test that uses the default Analysis format.

Also we probably don't need to add new tests, just letting the existing macro scripted tests run with consistent analysis format would surface the timestamp issue.

[Friendseeker]

@ephemerist Do you have a particular need of using new analysis format in sbt 1.x but without the erasure behavior?

If there's sufficient amount of people wanting to use new analysis format but without timestamp erasure I am thinking if we should revive #1479. It basically does the same thing as your PR except #1479 makes the erasure behavior toggleable.

[ephemerist]

@ephemerist Do you have a particular need of using new analysis format in sbt 1.x but without the erasure behavior?

If there's sufficient amount of people wanting to use new analysis format but without timestamp erasure I am thinking if we should revive #1479. It basically does the same thing as your PR except #1479 makes the erasure behavior toggleable.

At Morgan Stanley we've got a large scala/java monorepo and in some scenarios the amount of time taken to serialize/deserialize analysis files is a meaningful part of the total build time. Having seen the performance improvements offered by consistent analysis we're keen to use it instead of the protobuf impl, but not at the expense of correctness - undercompiling would lead to a lot of complaints from our users.

I'd be keen to see #1479 resurrected (ideally with reproducible = false as the default, and some commentary to explain the risks of setting it to true) - right now the behaviour is broken on the 1.10.x branch. Fortunately our own unit tests caught this before we switched over to using it, but developers of other build tools might be caught out in future.

[Friendseeker]

@ephemerist Do you have a particular need of using new analysis format in sbt 1.x but without the erasure behavior?
If there's sufficient amount of people wanting to use new analysis format but without timestamp erasure I am thinking if we should revive #1479. It basically does the same thing as your PR except #1479 makes the erasure behavior toggleable.

At Morgan Stanley we've got a large scala/java monorepo and in some scenarios the amount of time taken to serialize/deserialize analysis files is a meaningful part of the total build time. Having seen the performance improvements offered by consistent analysis we're keen to use it instead of the protobuf impl, but not at the expense of correctness - undercompiling would lead to a lot of complaints from our users.

I'd be keen to see #1479 resurrected (ideally with reproducible = false as the default, and some commentary to explain the risks of setting it to true) - right now the behaviour is broken on the 1.10.x branch. Fortunately our own unit tests caught this before we switched over to using it, but developers of other build tools might be caught out in future.

@SethTisue @eed3si9n What do you think? I am personally leaning towards granting Zinc users the ability to control the erasure behaviour of new analysis format, now there's evidence that users indeed do want such controllability.

@ephemerist Going slightly off topic here but I wonder if your guys have a preferred communication channel (e.g. Scala Discord). I do feel if there were more communication between Zinc Contributors and Morgan Stanley engineers, things would have moved more smoothly. Like I am pretty sure we duplicated effort investigating this particular issue, which was avoidable.

[ephemerist]

@ephemerist Going slightly off topic here but I wonder if your guys have a preferred communication channel (e.g. Scala Discord). I do feel if there were more communication between Zinc Contributors and Morgan Stanley engineers, things would have moved more smoothly. Like I am pretty sure we... duplicated effort investigating this particular issue, and the duplication could have been avoided if we communicated earlier.

Sadly, for regulatory reasons all our day-to-day communication has to happen on internal MS systems. I could have probably just raised this as an issue rather than creating a PR as well, but even before creating the issue I'd have done some digging to check on a root cause. Going even more off-topic we do have a larger zinc change planned (basically upstreaming some work on jar editing that we already have internally); I'll ask the developer who's working on it to get in touch.

[Friendseeker]

@ephemerist Going slightly off topic here but I wonder if your guys have a preferred communication channel (e.g. Scala Discord). I do feel if there were more communication between Zinc Contributors and Morgan Stanley engineers, things would have moved more smoothly. Like I am pretty sure we... duplicated effort investigating this particular issue, and the duplication could have been avoided if we communicated earlier.

Sadly, for regulatory reasons all our day-to-day communication has to happen on internal MS systems. I could have probably just raised this as an issue rather than creating a PR as well, but even before creating the issue I'd have done some digging to check on a root cause.

True. However I still do feel there's some topics we can discuss which can be mutually beneficial. For example I am curious to know whether consistent analysis is... fast enough. I was looking into Apache Fury and I do believe analysis serialization can be even faster, if there's a need for it.

Going even more off-topic we do have a larger zinc change planned (basically upstreaming some work on jar editing that we already have internally); I'll ask the developer who's working on it to get in touch.

That sounds fantastic! I am not experienced with compile to jar workflow (Eugene & Adrien are active sbt maintainers familiar with jar workflow), so ideally the dev can get in touch with either of the two.

[eed3si9n]

What do you think? I am personally leaning towards granting Zinc users the ability to control the erasure behaviour of new analysis format, now there's evidence that users indeed do want such controllability.

The background context here is that we have multiple consumers of Zinc, and they are sorting making different tradeoff with regards to the ConsistentAnalysisFormat, including:

• custom Scala rule for Bazel
• sbt 1.x
• sbt 2.x
• OBT

I think it would be good to make the timestamp feature configurable.

[Friendseeker]

Closing this PR since #1479 is approved