There is one compatibility issue:

Func<object?> code = () => null;
Assert.That(code, Is.Not.Null);

Previously this was executed as Assert.That<TActual>(TActual value, ...) which checked if the 'pointer' was null.
Now, it executes the delegate and checks the returned value.

To test a Func<T> variable for null one must cast it to object?:

Assert.That((object?)code, Is.Not.Null);

or obtain the value indirectly.
This calls Func<TActual> with TActual being Func<object?>.
It returns the code variable which then can be test for null:

Assert.That(() => code, Is.Not.Null);

Note this is the same behaviour if the user had used an ActualValueDelegate variable:

ActualValueDelegate<object?> code = () => null;
Assert.That(code, Is.Not.Null);

Note that Action and TestDelegate are only executed for ThrowsConstraint.
In other case it use the function point as the actual value to the constraint.

@manfred-brands So a major change here is that with your changes, all delegates are executed, not only the ones that have the Throws constraints?
Have I got that right ?

Current behavior (version 4.5.X):

@OsirisTerje

Have I got that right ?

Regarding your Never (useless) comment for Func<T>.
As I said before it can be used to test for null.
Is there practically a case where you would have a method that return a value?
I can think of optional processing tasks like Func<T, T>.

Func<T>? getValue = DetermineGetValueMethod();
Assert.That(getValue, Is.Not.Null);
T value = getValue.Invoke();

Ok, but with your changes in this PR, are the delegates executed ?

Ref

Now, it executes the delegate and checks the returned value.

Ok, but with your changes in this PR, are the delegates executed ?

Ref

Now, it executes the delegate and checks the returned value.

Yes

@manfred-brands Is this PR just waiting for nunit/nunit.analyzers#982 ?

@manfred-brands Is this PR just waiting for nunit/nunit.analyzers#982 ?

Also on a decision if we want to put this in the 4.6 branch instead possible with a setting to switch behaviour as it then would be a non-breaking feature. It would get my vote.

I'm just catching up after being away for a week, but if I read right then it sounds like we are invoking the lambda in all cases now?

I don't think I have an "informed" opinion yet, but my first instinct is that the previous behavior felt a bit more natural to me, as did the workaround. Previously, where we only executed for throws, one could "work around" this by just manually invoking the method and passing the return value in. It also followed the "opt-in" model where the lambda could be "opted into" executing by having calling code do that. If that's right, then having to cast a Func to an object to "opt out" feels a bit more "hidden" to me and less clear.

To help me understand, was there a use case towards moving to the new model?

@stevenaw to clarify, the new behaviour for Func<T> is the same as for ActualValueDelegate<T>. There is also no difference for lambdas.

The only difference is for a variable declared as Func<T> which previously was treated like object but now the same as ActualValueDelegate<T>.

a setting to switch behaviour as it then would be a non-breaking feature.

We cannot make such a setting as we cannot distinguish between a variable of type Func<T> or a lambda.
We could make an analyzer rule which can detect if a variable is used and tested for null, like:
Assert.That(FuncVariable, Is.Not.Null);
If FuncVariable has a non-class return type, that is clearly a test on the variable.
But if the return type is a class, we can't tell if the user intended execution or not.

to clarify, the new behaviour for Func is the same as for ActualValueDelegate. There is also no difference for lambdas.

The only difference is for a variable declared as Func which previously was treated like object but now the same as ActualValueDelegate.

Ok that's great then, thanks for confirming @manfred-brands . I have no concerns with how you've described this

Also on a decision if we want to put this in the 4.6 branch instead possible with a setting to switch behavior as it then would be a non-breaking feature. It would get my vote.

Since most of the possible other usages was "useless" and also invoked warnings, I think this may go into 4.6 (imho).

But, the analyzer then needs to be updated wrt those warnings. The once I see may be affected is

If the switch is to be able to be sent down from the runsettings, then we need to add that setting to the adapter too.

IF anyone wants to check a delegate for null (current behavior) I am fine with the (object) cast.

And we need to document all of this, so a document issue is needed.

PS: One thing I like here is that this will also support deferred execution which we use for the After modifier (delay and repeat).

Also on a decision if we want to put this in the 4.6 branch instead possible with a setting to switch behavior as it then would be a non-breaking feature. It would get my vote.

Since most of the possible other usages was "useless" and also invoked warnings, I think this may go into 4.6 (imho).

But, the analyzer then needs to be updated wrt those warnings. The once I see may be affected is

I'll check my PR for the Func with parameters.

If the switch is to be able to be sent down from the runsettings, then we need to add that setting to the adapter too.

We cannot do a switch as that would disable all code using lambda.
The lambda will use the Func overload so we cannot switch that to old behaviour.
Basically any recompiled code will evaluate the Func.
We could add special code that:
IFF the constraint is Null or 'Not.Null' AND the Func does not return a class, it must want to check the delegate itself.

IF anyone wants to check a delegate for null (current behavior) I am fine with the (object) cast.

And we need to document all of this, so a document issue is needed.

Sounds good to me.
Do you change the target to main ?

Just noted this analyzer issue nunit/nunit.analyzers#982, so that comment above is ok

@OsirisTerje I retargeted to main.

Locally, I get build errors which look related to what you are working on:

     ConsoleWrite_WritesToResult_AdhocContext (7ms): Error Message:   Assert.That(stdout.ToString(), Is.EqualTo(SOME_MORE_TEXT))
        Expected string length 27 but was 0. Strings differ at index 0.
        Expected: "Should be written to stdout"
        But was:  <string.Empty>

Re local build fails:

Tested locally here, and it WOMM. But this test is flaky, once the Phase2 is merged, it should be better, I hope.

Wonder a bit why we never get rid of that appveyor build......

Re local build fails:

Tested locally here, and it WOMM. But this test is flaky, once the Phase2 is merged, it should be better, I hope.

Wonder a bit why we never get rid of that appveyor build......

My suspicion is that GH must've been having issues at the time the PR was last updated. @manfred-brands I was able to resolve the issue previously in a PR by amending the most recent commit to include whitespace, which forced GH to reevaluate the branch due to the changing commit hash.

The most recent GH Actions build seemed to have passed. I have rerun it as well just now to see if that also helps
https://github.com/nunit/nunit/actions/runs/24320163783

EDIT: I wonder if it's the change in target branch that is complicating this. That build I linked seems to still reference v5 and should be ignored

How breaking is it?

I don't think it is too harmful (will probably come back to bite me, but....).
Suggest we keep this on 4.6. We can make a note in the release notes about the possibilities for breaking there.

Then we should continue the discussion on versioning we started, and also consider if we should be more aggressive with using the major versioning. Right now we have a self-inflicting wound/rule to keep as few major versions as possible. We don't have to do that. But this is for #5223 .