Fix QuotesCache caching quoted symbol definitions with incorrect owners #20492
Conversation
Previously we would always cache and reuse unpickled trees, which was a problem for quoted code which included symbol definitions. In those cases, even if those quotes were being created within another context (which should dictate owners of symbol definitions), it would be ignored, and previous potentially incorrect symbol definitions would be reused. Now we include the quote symbol owner while caching, which is only taken into account if the quoted code contains a symbol definition.
| val cp = Paths.get(url.toURI).toString + JFile.pathSeparator + Properties.scalaLibrary | ||
| val cp = Paths.get(url.toURI).toString + | ||
| JFile.pathSeparator + Properties.scalaLibrary + | ||
| JFile.pathSeparator + Properties.dottyLibrary |
There was a problem hiding this comment.
I had to add the dottyLibrary to runtime of the test here, because otherwise the run_staging tests would fail on windows (as it would be fail trying to access scala.quoted.runtime.Patterns.patternType after the other changes). Not sure why this problem was windows only, but the way classpath is reconstructed from classloader in the staging compiler is a heuristic, so that might be the reason.
| tree.foreachSubTree(identity) | ||
| new TreeTraverser { | ||
| def traverse(tree: Tree)(using Context): Unit = | ||
| tree match | ||
| case _: DefTree => | ||
| if !tree.symbol.hasAnnotation(defn.QuotedRuntime_SplicedTypeAnnot) | ||
| && !tree.symbol.hasAnnotation(defn.QuotedRuntimePatterns_patternTypeAnnot) | ||
| then | ||
| includesSymbolDefinition = true | ||
| case _ => | ||
| traverseChildren(tree) | ||
| }.traverse(tree) |
There was a problem hiding this comment.
This part was inspired by 94f069e, but instead of looking for any symbol in the unpicked tree to decide whether to cache/uncache, we decide whether to cache with or without the context owner. This keeps the previous performance improvements while keeping the owners unpicked symbols always correct
There was a problem hiding this comment.
I've had a chat with @jchyb offline and I believe that the suggested fix here will not be performant. I've suggested an implementation based on a TreeTraverser to reconcile the owners and keep the caching performance !
|
Superseded by #21932, a way simpler (and faster!) idea for a fix (thank you @hamzaremmal for the suggestion!) |
Previously we would always cache and reuse unpickled trees, which was a problem for quoted code which included symbol definitions. In those cases, even if those quotes were being created within another context (which should dictate owners of symbol definitions), it would be ignored, and previous potentially incorrect symbol definitions would be reused.
Now we include the quote symbol owner while caching, which is only taken into account if the quoted code contains a symbol definition.
Fixes #20471