Close global after compute ondemand semanticdb files #1348
Conversation
| @@ -347,6 +348,18 @@ object MainOps { | |||
| } else { | |||
| args.rules.syntacticPatch(doc, args.args.autoSuppressLinterErrors) | |||
| } | |||
| // close global if evaluated | |||
| cleanGlobalRessources(args.global) | |||
There was a problem hiding this comment.
should that be in a finally block instead?
There was a problem hiding this comment.
I don't understand your question - here?
There was a problem hiding this comment.
try {
val tree = LazyValue.later { () =>
args.parse(input).get: Tree
}
val doc = SyntacticDocument(input, tree, args.diffDisable, args.config)
val results = if (args.rules.isSemantic) {
val relpath = file.toRelative(args.sourceroot)
val sdoc = SemanticDocument.fromPath(
doc,
relpath,
args.classLoader,
args.symtab,
() => compileWithGlobal(args, doc)
)
val result =
args.rules.semanticPatch(sdoc, args.args.autoSuppressLinterErrors)
assertFreshSemanticDB(
input,
file,
result.fixed,
sdoc.internal.textDocument
)
result
} else {
args.rules.syntacticPatch(doc, args.args.autoSuppressLinterErrors)
}
results
} finally {
// close global if evaluated
shutdownCompiler(args.global)
}
There was a problem hiding this comment.
something like this ?
There was a problem hiding this comment.
hard to tell without proper indentation and highlighting but yes that's what I had in mind 👍 (the results val can be removed)
mlachkar
force-pushed
the
global
branch
2 times, most recently
from
acfe84d to
01fcac8
Compare
There was a problem hiding this comment.
Maybe a larger question but do we want to keep supporting ondemand generation of semanticdb files in 1.0? This will require significant effort to support it with Scala 3 sources, and as far as I know, that feature is undocumented.
What are the use-cases behind it? Was that done to cover shortcomings of build tool integration? It looks like @olafurpg added it while working on ExplicitResultTypes but the git history is not very clear.
|
It's useful if the semanticdb files are not present, especially if it's used through the API when we don't depend on compile task. |
There was a problem hiding this comment.
It's useful if the semanticdb files are not present, especially if it's used through the API when we don't depend on compile task.
We may open another issue to discuss this.
👍
LGTM (moving the call to shutdownCompiler in a finally block would be a bonus I guess)
|
try finally will be usefull since wr throw a potentiel exception with the Assert. Thank you for the advise |
| } else { | ||
| args.rules.syntacticPatch(doc, args.args.autoSuppressLinterErrors) | ||
| } | ||
| } finally shutdownCompiler(args.global) |
There was a problem hiding this comment.
actually I guess the finally block could be scoped to where it's actually used - around the thunk calling compileWithGlobal (or potentially move it there)?
There was a problem hiding this comment.
You are right, it's again on the wrong place .. we need it elsewhere, where the function is called. we don't want to open a new gloabl for each file.
|
The library API could support an enum mode to determine how to load SemanticDB files. I agree it can sometimes be confusing when the fallback triggers. |
The state before the fix. It possible to reproduce by running multiple time the test on-demand with scalacOptions
The presentation compiler is never closed.