-
Notifications
You must be signed in to change notification settings - Fork 55
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support for tailstrict #257
base: master
Are you sure you want to change the base?
Conversation
b9b25c8
to
938d7e9
Compare
938d7e9
to
4eb8b30
Compare
@@ -28,7 +28,12 @@ class Evaluator(resolver: CachedResolver, | |||
def materialize(v: Val): Value = Materializer.apply(v) | |||
val cachedImports = collection.mutable.HashMap.empty[Path, Val] | |||
|
|||
def visitExpr(e: Expr)(implicit scope: ValScope): Val = try { | |||
var isInTailstrictMode = false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the thread-safety model here?
By having this be a mutable property of the evaluator itself I think we are foregoing safe multi-threaded usage of Evaluator
.
A few lines up, I see that we have a non-thread-safe mutable HashMap for cachedImports
, so it's possible that this class is de-facto pre-existingly not threadsafe.
Does this match your understanding? And is our current usage in line with this?
I raise this because I've been considering implementing certain performance optimizations which strongly rely on single-threaded evaluation (such as same-thread recycling of certain temporary structures used in string materialization) and it would be convenient to be able to make strong single-threadedness assumptions about evaluator.
} | ||
lhs.cast[Val.Func].apply(argsL, e.namedNames, e.pos) | ||
} | ||
|
||
private def visitApply0(e: Apply0)(implicit scope: ValScope): Val = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we might need handling for tailstrict here too? Despite the fact that a zero-argument function cannot itself be tail recursive, don't we still need to propagate the isInTailstrictMode
flag for use in child evaluations?
case class Apply0(pos: Position, value: Expr, tailstrict: Boolean) extends Expr | ||
case class Apply1(pos: Position, value: Expr, a1: Expr, tailstrict: Boolean) extends Expr | ||
case class Apply2(pos: Position, value: Expr, a1: Expr, a2: Expr, tailstrict: Boolean) extends Expr | ||
case class Apply3(pos: Position, value: Expr, a1: Expr, a2: Expr, a3: Expr, tailstrict: Boolean) extends Expr | ||
case class ApplyBuiltin(pos: Position, func: Val.Builtin, argExprs: Array[Expr]) extends Expr { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm curious about the interaction between tail strict mode, built-in function application, and potential lazy evaluation in built-ins.
For example, it is possible to have a tailstrict
user-defined function which invokes a built-in function which returns lazy values which in turn reference user defined functions that could potentially be affected by tail strict mode? i.e. do we need "memory" of whether tail strict mode was in effect at the time of creation of a lazy value?
@@ -377,11 +377,14 @@ object Val{ | |||
|
|||
override def asFunc: Func = this | |||
|
|||
def apply(argsL: Array[_ <: Lazy], namedNames: Array[String], outerPos: Position)(implicit ev: EvalScope): Val = { | |||
def apply(argsL: Array[_ <: Lazy], namedNames: Array[String], outerPos: Position)(implicit ev: EvalScope, vs: ValScope = defSiteValScope): Val = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a little confused about the semantics of having an implicit parameter which has a default value. Where do we override this? I'm wondering if it might be clearer to have it be in its own parameter group if we don't expect to pass it implicitly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Courtesy of ChatGPT, an example demonstrating that implicits in the caller scope take precedence over the default parameter value:
object DefaultVsImplicitExample extends App {
// A method with an implicit parameter that has a default value
def greet(implicit name: String = "DefaultName"): Unit =
println(s"Hello, $name!")
// No implicit in scope, so it uses the default value
greet // prints: "Hello, DefaultName!"
// Introduce an implicit value in this scope
{
implicit val localName: String = "Alice"
// Now greet() finds the implicit "Alice" instead of using the default
greet // prints: "Hello, Alice!"
}
// Outside that block, the implicit is no longer visible, so greet() falls back to the default
greet // prints: "Hello, DefaultName!"
}
Do we ever actually use the default value of vs
here? i.e. do we ever mutate defSiteValScope
?
I tried dropping the defaults and then ran into compilation problems in built-in functions, e.g. in uniqArr
where we have calls like
val o1Key = keyFFunc.apply1(v, pos.noOffset)(ev)
which won't apply without a vs
. But if this is indeed tail rec mode, don't we want to be using the caller's scope?
I find this pretty confusing overall and I'd like to get a better handle on how this works, esp. to make sure that we're not somehow having unexpected interactions between tail recursive functions and built in functions.
With this PR, I'm implementing tailstrict in sjsonnet.
I'm following the guidance from google/jsonnet#343 (comment):
Quote:
Resolves #189.