Open laurenceSaes opened 3 years ago
Thanks for your reporting. This one is going to be difficult to fix. The problem is that Stryker tries to replace the logical and (&&
) by a logical or ('||
') leading to scenarios where items
may not be initialised, indeed.
To fix this, we should either:
&&
by |
since then the right part will be evaluated disregarding the left part.Ideally, I would try to implement (1), but I am pretty sure we won't reach 100% detection accuracy, so (2) looks interesting, but I am not sure about other consequences.
We need to discuss this within the Stryker teams to reach a decision.
I am also seeing this quite a lot. Some analysis to prevent it is of course good, but how is this different from other mutations that cause compile errors? (my point being, why is it being reported "should not happen")
Because rollbacks cost runtime, so ideally we don't place ANY mutations that don't compile :) This message is a trigger for us to investigate if we can improve the behavior to reduce bad mutations. We have discussed creating a list of known bad constructs so we don't log this message for those that have already been reported, but have not built this solution.
Here is another example of the same issue Use of unassigned local variable 'status' (Source code: status)
private static async Task<(CommandStatus, T?)> HandleExceptionsAsync<T>(Func<Task<T>> func, CancellationToken cancellationToken = default)
{
T? value = default;
CommandStatus status;
try
{
value = await func().ConfigureAwait(false);
status = new Success();
}
catch (OperationCanceledException) when (cancellationToken.IsCancellationRequested)
{
status = new Canceled();
}
catch (Exception ex)
{
status = new Failure(ex.ComposeMessage());
}
return (status, value);
}
I have a similar issue with out parameters. Example repro code:
public class Thing
{
private readonly static Dictionary<string, Thing> configCache = new();
public string Property { get; private set; }
public string TestWithOutParam()
{
int index = GetIndex();
string key = GetKey();
Thing newThing;
if (index >= -1)
{
if(configCache.TryGetValue(key,out newThing) is not true)
{
newThing = new Thing();
}
}
else
{
throw new Exception();
}
var result = newThing.Property;
return result;
}
private string GetKey()
{
return "key";
}
private int GetIndex()
{
return 1;
}
}
which stryker modifies like so:
public class Thing
{
private readonly static Dictionary<string, Thing> configCache = new();
public string Property { get; private set; }
public string TestWithOutParam()
{
{
int index = GetIndex();
string key = GetKey();
Thing newThing;
if ((Stryker33vlK7xAik0y7wl.MutantControl.IsActive(3) ? !(index >= -1) : (Stryker33vlK7xAik0y7wl.MutantControl.IsActive(2) ? index > -1 : (Stryker33vlK7xAik0y7wl.MutantControl.IsActive(1) ? index < -1 : index >= (Stryker33vlK7xAik0y7wl.MutantControl.IsActive(4) ? +1 : -1)))))
{
if (Stryker33vlK7xAik0y7wl.MutantControl.IsActive(5)) { }
else
{
if (configCache.TryGetValue(key, out newThing) is not true)
{
if (Stryker33vlK7xAik0y7wl.MutantControl.IsActive(7)) { } <-- this mutation is the issue
else
{
newThing = new Thing();
}
}
}
}
else
{
if (Stryker33vlK7xAik0y7wl.MutantControl.IsActive(8)) { }
else
{
if (Stryker33vlK7xAik0y7wl.MutantControl.IsActive(9)) {; }
else
{
throw new Exception();
}
}
}
var result = newThing.Property;
return result;
}
return default(string);
}
}
a potentially starting point for this could be disabling block mutations for the code block immediately after the false path for bool expressions with an out parameter. If someone could point me in the direction of where this might be implemented then I am willing to have a look and see if some progress here could be made. Obviously some of the other examples here are more complicated, but maybe after solving the simple case we would have a better idea of how to extend that to solve some of the more complicated scenarios. If you think this is not worth investigating then say and I won't bother :)
@samholder: I am afraid you are blaming an innocent here. The culprit is actually mutant 9, which bypasses the exception throwing, allowing the code to skip initializing newThing
, leading to your error message.
Actually the problem is not about not generating these mutants, as it is difficult to guess which ones will result in an error (outside trivial cases). It is about identifying the guilty mutant when an error occurs.
see discussion #2296 for a related discussion
Hmmm. I'm sure that you are much more knowledgeable in this area than me, but I removed all stryker mutants apart from 7 and I still get the compilation issue. See this fiddle: https://dotnetfiddle.net/VSjs6z . I don't really see what the exception has to do with anything here.
I'm not sure if that changes anything, but having read the link I don't see how those apply to the use case I outlined where the out argument will be unset for a Try method which returns false, so then removing the block in that case is always going a variable unset issue (because we are removing the block which is usually responsible for setting that variable if the call to the Try method does not) unless:
the variable is set in the code after the if(TryGet*
block which might be less strange but again maybe not an issue.
Anyway I might have a look through the code and try and drum up a test + implementation of a suggested fix over the next couple of weeks. Any pointers as to where to start looking would be appreciated
the variable is set in the true path (which would be strange because its set as a result of the call, but I don't think this would be an issue)
The problem is not the out parameter: out parameters are guaranteed to be initialized after the call, even if the method returns false.
If you remove the mutation in your example, you still get the error. I am not familiar with DotNetFiddle, but if you remove
if (Stryker33vlK7xAik0y7wl.MutantControl.IsActive(7)) { } else
the error is still there. This is because you removed the throw statement.
if you restore it, everything will be ok.
Note that my previous statement was somewhat wrong: you need to remove mutations 8 and 9, sorry about that.
Describe the bug The Stryker log contains a message that I should create an issue on Github.
Logs [08:53:56 WRN] Stryker.NET encountered an compile error in ...\CustomTplBlocks\PriorityBlock.cs (at 111:39) with message: Use of unassigned local variable 'item' (Source code: item) [08:53:56 WRN] Safe Mode! Stryker will try to continue by rolling back all mutations in method. This should not happen, please report this as an issue on github with the previous error message.
Expected behavior That the file could be mutated. This issue is resolved when not reusing the variable item.
(Please note that this is a buggy code snippet, there should be a return after the Complete call.)
Desktop (please complete the following information): OS: Windows 10 Type of project: framework Framework Version: net4.8 Stryker Version: commit cd0c866243a14e79cdb6013b67fa98a9e00eb883
Additional context
The source code:
The source code with line numbers: