Don't lift the argument of a synchronized block in scoverage

[KacperFKorban]

possible fix for lampepfl#16940

[ckipp01]

I'm not 100% sure why it was vals instead of defs in the first place. I also can't think of anything wrong with this, but just to make sure I also tagged @TheElectronWill on this. However it's a LGTM from me. Thanks!

[TheElectronWill]

Will every argument be turned into a def, in all circumstances? I'm a bit worried about the performance and size overhead of that.

[nicolasstucki]

Isn't this changing the evaluation semantics?

[nicolasstucki]

From the test case, it seems that the failure is on a by-name argument. If so changing the binding of that by-name argument to a def is the correct fix. But we should use defs for by-name arguments.

[ckipp01]

From the test case, it seems that the failure is on a by-name argument. If so changing the binding of that by-name argument to a def is the correct fix. But we should use defs for by-name arguments.

@KacperFKorban is it possible to just make this change and only use defs for the by-name arguments?

[mohe2015]

Any updates on this?

[KacperFKorban]

Sorry, this PR got a bit buried for me.
@ckipp01 I tried doing that, but it doesn't fix the problem. This specific problem is with synchronized which takes a normal argument, not a by-name one. (Though by-name arguments also aren't correctly lifted)
@nicolasstucki Does lifting to defs actually change the semantics? Don't we guarantee that every lifted expression will be referenced exactly once?

[odersky]

I am not sure whether lifting to defs would break the semantics, i.e. whether we can guarantee used exactly once. Probably we can't. I would feel very nervous with such a change. Also lifting to defs certainly will make the code go much slower in many cases.

[nicolasstucki]

Does lifting to defs actually change the semantics?

It should not. We already do that for by-name arguments of inline definitions.

Don't we guarantee that every lifted expression will be referenced exactly once?

It is referenced once at the call site, but possibly evaluated many times in the function.

[nicolasstucki]

Also lifting to defs certainly will make the code go much slower in many cases.

In this case, lifting the by-name argument into a def should not make it much slower (or at all). We can use that def directly to encode that lambda that is passed as an argument. If it is currently not the case, then we probably have a general optimization opportunity.

[odersky]

As far as I know, cbn arguments are already lifted into defs. It's just cbv arguments that are lifted into vals.

[KacperFKorban]

Ok. In that case, I changed this fix to just avoid lifting arguments of synchronized blocks.

[KacperFKorban]

@nicolasstucki does this look ok now?

[odersky]

I think that makes sense.