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

Issue with mutmut mutation in presence of type hints #302

Closed
pablo-snz opened this issue Jan 24, 2024 · 17 comments
Closed

Issue with mutmut mutation in presence of type hints #302

pablo-snz opened this issue Jan 24, 2024 · 17 comments

Comments

@pablo-snz
Copy link

Hi! I'm wondering if there's a direct way to exclude type hints within mutmut. For example, in the following code, I'm getting errors because mutmut tries to mutate the "|" operator to its counterpart "&":

@dataclass
class Specification:
    filter: Criteria | None = None  # Mutates to => Criteria & None = None
    order: Order | None = None  # Also happens in all of the cases below
    limit: int | None = None
    offset: int | None = None

Plus it becomes more complex in a scenario like this where I can't just whitelist the entire line:

user: User | None = self.repo.match(Specification(filter=Equals("email", user.email) | Equals("username", user.username)))

On the other hand, if mutmut cannot handle this right now in a straightforward manner, would it be possible for me to contribute to the repository by adding this functionality?

@boxed
Copy link
Owner

boxed commented Jan 24, 2024

Hmm. I guess in general it would be a good idea to ignore type hints totally, except they are used for behavior in stuff like pydantic.

I am very much open to a PR. Not sure what should be done though. The smallest change would be for the operator mutation for or to not trigger if inside an annotation. A big ugly but would fit this case.

@efine-vivodyne
Copy link

I just started playing with mutmut today, and this was one of the immediate pain points I ran into. I would also very much be interested in better support for type hints.

I see the potential issue with pydantic, but also I've never run across any code actually using & in a type hint. I think a flag in the configuration that said "never attempt to mutate a | to a & in a type hint" with be something I would gladly turn on and never have a second thought that I'm risking less thorough mutation testing of my code

@efine-vivodyne
Copy link

I don't know if there's another issue for this already, but another odd thing I saw was

-    port: int | None = None
+    port: int | None = ""

I understand in general that mutating default values is a good thing to probe. But if someone is using type hints in their code, presumably they're also using a type-checker, which would flag an attempt like this to assign a default value that doesn't match the type hint. This seems like a false positive that mutmut is creating

@EdgyEdgemond
Copy link

EdgyEdgemond commented Jun 27, 2024

Have taken advantage of mutmut_config.py to allow skipping lines with # nomut comment, would this be a suitable work around (default pre_mutation to add to mutmut)?

def pre_mutation(context) -> None:
    line = context.current_source_line.strip()
    if "# nomut" in line:
        context.skip = True

Would be preferable to not have to ignore every type hinted line that gets mutated, and/or add a mutmut_config.py to every project to allow ignoring typehints.

But commenting them out is better than not having a solution :)

@boxed
Copy link
Owner

boxed commented Jun 27, 2024

Why not use the built in syntax # pragma: no mutate?

But yea, I agree that type hints getting mutated seems unfortunate. A PR would be very much welcome.

I think there is a case for SOME type hints to be mutated though, like for dataclasses or serializers.

@EdgyEdgemond
Copy link

EdgyEdgemond commented Jun 27, 2024

# pragma: no mutate sounds good

edit: nomut was closer to noqa which I use more often than pragma: no cover so was first to mind.

@EdgyEdgemond
Copy link

Regarding the type hint mutation for |

I found this to work locally:

-def operator_mutation(value, node, **_):
+def operator_mutation(value, node, context, **_):
     if import_from_star_pattern.matches(node=node):
         return
 
@@ -343,6 +343,9 @@ def operator_mutation(value, node, **_):
     if value in ('*', '**') and node.parent.type in ('argument', 'arglist'):
         return
 
+    if value == "|" and re.findall(r".*:.*|.*", context.current_source_line):
+        return
+
     return {
         '+': '-',
         '-': '+',

Happy to open a PR to add this change, but I am not having a lot of luck getting tests to successfully run, so not sure I'd be able to fully add it in to the point the PR is actually acceptable.

@boxed
Copy link
Owner

boxed commented Jun 28, 2024

The change seems a bit heavy handed. Just checking if there is a comma before a pipe is not reasonable imo. It would mean it would remove all other mutations for all operators for lines which has colon and pipe in that order.

@EdgyEdgemond
Copy link

Would you be able to provide a better regex to match var: type | type. Can you provide additional scenarios you envision that this would catch accidentally? (I've managed to find the correct place in tests to capture that a | b still mutates but var: a | b does not currently.

@EdgyEdgemond
Copy link

https://github.com/boxed/mutmut/pull/327/files including tests for existing/new intended behaviour, if you can suggest a less all inclusive regex, or some additional test cases I can expand on it.

@EdgyEdgemond
Copy link

EdgyEdgemond commented Jun 28, 2024

The change seems a bit heavy handed. Just checking if there is a comma before a pipe is not reasonable imo. It would mean it would remove all other mutations for all operators for lines which has colon and pipe in that order.

It will only remove mutations for | operator and lines that match the regex.

Test case for other mutations still occuring.
https://github.com/boxed/mutmut/pull/327/files#diff-8ccb3dfbce788ac7fc87e69a419163b3bd5d2c31834c387c494999c6cfbd90a1R116

@boxed
Copy link
Owner

boxed commented Jun 28, 2024

The regex is not precise. You should look at the AST, not do a regex on the line. The approach itself is incorrect.

@boxed
Copy link
Owner

boxed commented Jun 28, 2024

I like your enthusiasm but you have now sent two PRs which were both incorrect and misunderstood the problem. You need to take it a bit slower and think through things and discuss them properly before acting.

@EdgyEdgemond
Copy link

Thanks for the feedback, I'll take a look into the AST and come back if I am able to make any progress.

@EdgyEdgemond
Copy link

EdgyEdgemond commented Jun 28, 2024

https://github.com/EdgyEdgemond/mutmut/pull/1/files

@boxed when you have a moment could you check the draft over on my fork, currently handling the type hints I've had issues with, as well as the ones raised at the top of this issue, along with allowing the complicated use case raised to still be mutated. Confirm if this is more in line with your thinking.

Continuing to see if I can stop the issue with declared type hints like below from having string mutations applied (can # pragma: no mutate these easily if not. If you have any suggestions on how these could be tackled that would be appreciated.

-Handler = typing.Callable[["ExceptionHandler", Request, Exception], typing.Optional[Problem]]
+Handler = typing.Callable[["XXExceptionHandlerXX", Request, Exception], typing.Optional[Problem]]

Similar for other type hints

-PreHook = typing.Callable[[Request, Exception], None]
+PreHook = None

@boxed
Copy link
Owner

boxed commented Jun 28, 2024

@EdgyEdgemond yea that looks more correct.

@boxed boxed closed this as completed Oct 20, 2024
@boxed
Copy link
Owner

boxed commented Oct 20, 2024

I just released mutmut 3, which is a big rewrite. I believe this issue no longer applies anymore. Feel free to reopen it if it still exists.

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

No branches or pull requests

4 participants