Skip to content
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

2933 static using math #3205

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

jacinyan
Copy link

Summary

Fixes #2933 by using the semantic model to detect calls to System.Math when using static using. This ensures Floor(5.5) is correctly mutated, just like Math.Floor(5.5).

Changes

  • Added semantic model checks in both member access and direct call branches.
  • Updated MathMutator to handle IdentifierNameSyntax calls to Math methods.

Testing

  • Verified with a sample C# project using static imports (FloorValue, MixedFloorValue, etc.).
  • Mutation score increased and static calls now generate mutants.

@dupdob
Copy link
Member

dupdob commented Feb 24, 2025

thanks for the PR. You need to add some tests to explicitly test for static using.

May I also suggest you should check first if the method name is of interest before resolving its symbol ?
As it is written, this mutator will ask to resolve the full symbol for every method invocation which may add to the total execution time.

Alternatively, having a benchmark (on a significant/large project) that demonstrates the cost being negligible is ok too .

@jacinyan
Copy link
Author

jacinyan commented Mar 1, 2025

thanks for the PR. You need to add some tests to explicitly test for static using.

May I also suggest you should check first if the method name is of interest before resolving its symbol ? As it is written, this mutator will ask to resolve the full symbol for every method invocation which may add to the total execution time.

Alternatively, having a benchmark (on a significant/large project) that demonstrates the cost being negligible is ok too .

Hi @dupdob ,

Thanks for your feedback. I've added explicit tests for static using calls and updated MathMutator to check the method name before resolving the symbol. Unfortunately, I haven't been able to run benchmarks on a large project yet, but based on theoretical analysis, the filtering should mitigate performance concerns.

Let me know if you have any further suggestions.

@dupdob
Copy link
Member

dupdob commented Mar 1, 2025

thanks @jacinyan. This is exactly what I had in mind; filtering on the name and then checking for actual type looks like the optimized approach.
I added some remarks, in line, but I will reproduce them here as it looks you did not see them:

Thanks for the optimization. There is still one small issue: this mutator contains two versions of the list of mutateable mathematical functions: one as strings (KnownMathMethodNames) and one as an enumeration (MathExpression). Which means that if someone changes one of the list, she will have to do it on both list, otherwise strange issues may appear.
I think you can remove the strings version using Enum.TryParse(methodNameText, out _) instead of KnownMathMethodNames.Contains(methodNameText)).
This way, we just need one list which means easier and safer maintenance.
That being done, please fix failing unit tests and this PR should be good to merge.

@jacinyan
Copy link
Author

jacinyan commented Mar 2, 2025

thanks @jacinyan. This is exactly what I had in mind; filtering on the name and then checking for actual type looks like the optimized approach. I added some remarks, in line, but I will reproduce them here as it looks you did not see them:

Thanks for the optimization. There is still one small issue: this mutator contains two versions of the list of mutateable mathematical functions: one as strings (KnownMathMethodNames) and one as an enumeration (MathExpression). Which means that if someone changes one of the list, she will have to do it on both list, otherwise strange issues may appear.
I think you can remove the strings version using Enum.TryParse(methodNameText, out _) instead of KnownMathMethodNames.Contains(methodNameText)).
This way, we just need one list which means easier and safer maintenance.
That being done, please fix failing unit tests and this PR should be good to merge.

Thanks @dupdob . I've made some changes accordingly. All tests passed. Please review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Math mutations are not placed when static using is used
2 participants