interpreter: Support debug-hooks #8097
Conversation
| Expression::DebugHook { expression, id: _id } => { | ||
| let value = eval_expression(expression, local_context); | ||
| #[cfg(feature = "internal-highlight")] | ||
| let value = crate::debug_hook::debug_hook_triggered(&local_context.component_instance, _id.clone(), value); |
There was a problem hiding this comment.
I'm thinking we should not pass the value in the debug_hook.
The debug hook can return a Option and only if that is None (default), then we would evaluate the inner expression.
There was a problem hiding this comment.
I absolutely do want to see the unchanged value. That information is super valuable.
There was a problem hiding this comment.
Can you elaborate how do you make use of it?
Remember that this function can be called several times for all instance of this property.
There was a problem hiding this comment.
But the debug-hooks assigned to those instances should have different ids.
Not perfect, true, as repeaters will duplicate those, but still pretty valuable. I should really try to get the enclosing components repeater-index (as an Option maybe?) so that it is trivial to see what came trough at least one level of repeaters and what did not.
There was a problem hiding this comment.
They will have the same ids.
component Foo {
in property <int> hello: 42;
in property <int> something : hello;
Text {
text: something;
}
}
component Bar {
foo1 := Foo {
hello: 85;
}
foo2 := Foo {}
foo3 := Foo { something: -8; }
}If you edit any property of Foo (hello, something or text), you never know if it is the one from foo1, foo2, or foo3.
Repeater only makes it worse, having the index won't really help.
What is the use of it anyway? What's written in the property editor should be based on what is on the source code.
There was a problem hiding this comment.
But why do you need it anyway?
We shouldn't be computing the value if that's not required.
Normally the value are pure but if they are not this might have side effect (eg)
foo: { debug("hello"); return 42; }
If you edit that, it will still call debug("hello") even if it is not supposed to be called.
And it will create more dependency than needed.
There was a problem hiding this comment.
This PR will have a side-effect on side-effects one way or the other.
We have "real input changes" when the input to the debug-hooked expression changes and we have "override changes" when the override itself changes.
If I calculate the "real" value and then check for an override, I trigger too many side effects (on "real input changes" and on "override changes").
If I ask for the override value and do not ask for the real value if there is an override, then I miss side effects (when we have "real input changes" while an override is n effect).
Not missing "real input changes" seems more important to me, so I went to calculate the "real value" at all times. When I do that I might as well pass the "real value" to the override call... More information is always better than less information when making a decision:-)
I do not expect either to be a problem in the real world. People already try to avoid side-effects. There are not too many in practice, especially not side effects that are not debugging-related.
There was a problem hiding this comment.
I'm still not convinced on how you'd use the value.
I'd like to see it in use in your LSP patch.
If I ask for the override value and do not ask for the real value if there is an override, then I miss side effects
(when we have "real input changes" while an override is n effect).
What side effects do you miss? I don't understand what you mean with real input changes. Do you have an example?
I do not expect either to be a problem in the real world. People already try to avoid side-effects.
Yes that's true. That said I still don't understand what you need the value for in the LSP.
I'd say it's more efficient to not compute the value and avoid useless dirty tracking.
There was a problem hiding this comment.
I'd say it's more efficient to not compute the value and avoid useless dirty tracking.
With a debug hook I have two reasons to mark a property as dirty:
- The actual binding that is used in the UI without the debug hook triggers an update. This code may have side-effects.
- A override is set/changed/removed. No side-effects here:-)
My approach
I want to make sure see any side-effect triggered by the non-override code, independent of whether an override is in effect or not. For that I need to always evaluate the non-override path.
I do get a few spurious side effects triggered when an override is set/changed/removed.
Your suggestion
You safe a few cycles by not evaluating the non-override code while an override is in effect.
Downside: By not evaluating the non-override path, you do not register the dependencies in that path. So you will not re-evaluate the property if any of these dependencies become dirty. That way you miss side-effects you would see in a run without where no override is ever present.
This is probably a theoretical difference only:
- Overrides are not going to be that widely applicable. They get removed pretty regularly by setting values.
- I do not expect overrides to be in effect for very long
- I seriously hope users do not rely on side-effects too much in the first place
In the rare cases where I need to rely on side-effects, I really need those side-effects to be visible and get into trouble if one of them goes missing. So I wanted to err on the side of seeing more side-effects than normal over seeing less than normal.
There was a problem hiding this comment.
I have already asked many time and you haven't yet provided an answer of what you'd do with this value. (Or why you'd want the re-evaluation to happen)
hunger
force-pushed
the
feature/debug-hook-interpreter
branch
from
a5533f5 to
823905c
Compare
|
|
||
| /// Set a callback triggered by `Expression::DebugHook``. | ||
| #[cfg(feature = "internal-highlight")] | ||
| pub fn set_debug_hook_callback(&self, callback: Option<crate::debug_hook::DebugHookCallback>) { |
There was a problem hiding this comment.
This should go in the CompilationResult, because this would apply to all the components. Or even in the Compiler then you can enable the write compilation config.
But sionce it's internal anyway, it should probably not be in the API at all, and just call crate::debug_hook::set_debug_hook_callback from the LSP.
| Expression::DebugHook { expression, id: _id } => { | ||
| let value = eval_expression(expression, local_context); | ||
| #[cfg(feature = "internal-highlight")] | ||
| let value = crate::debug_hook::debug_hook_triggered(&local_context.component_instance, _id.clone(), value); |
There was a problem hiding this comment.
I'm still not convinced on how you'd use the value.
I'd like to see it in use in your LSP patch.
If I ask for the override value and do not ask for the real value if there is an override, then I miss side effects
(when we have "real input changes" while an override is n effect).
What side effects do you miss? I don't understand what you mean with real input changes. Do you have an example?
I do not expect either to be a problem in the real world. People already try to avoid side-effects.
Yes that's true. That said I still don't understand what you need the value for in the LSP.
I'd say it's more efficient to not compute the value and avoid useless dirty tracking.
After Eastern then. I need to fix the new stuff in the LSP first. |
No description provided.