Typing issue

[agusdmb]

How do you use Sentry?

Sentry Saas (sentry.io)

Version

1.32.0

Steps to Reproduce

In the following snippet i am changing a super signature with a subclass, using mypy or any other static type checker this is mark as an error, but when the sentry decorator is added (as in the example) the error is hidden.

import sentry_sdk


class A:
    def foo(self, bar: int) -> int:
        return bar


class B(A):
    @sentry_sdk.trace
    def foo(self, bar: str) -> str:
        return bar

The issue is in the following lines:

sentry-python/sentry_sdk/tracing.py

Lines 958 to 959 in 085595b

	def trace(func=None):
	# type: (Any) -> Any

the decorator is changing the signature of both input and output to Any. This is not the expected behavior since the function will not actually be changed. And because the original signature expects integers, but the overriden one expects strings (which are not compatible) but when type checking the checker all it sees is "any" then because int is part of any is not giving any error when it should.

This can be easily fix by using a TypeVar in this case, like so:

from typing import TypeVar

T = TypeVar("T")

def trace(func=None):
    # type: (T) -> T

Expected Result

Type checker (like mypy) complains that the signature is not correct

Actual Result

Type checker (like mypy) doesn't complain at all, the bug is hidden

[sentrivana]

Hey @agusdmb, thanks for reporting this. I imagine the challenge here (and maybe the reason why we went with Any here) is to do this in a way that's compatible with all Python versions we currently support (2.7, 3.5-3.11).

[agusdmb]

Hey @agusdmb, thanks for reporting this. I imagine the challenge here (and maybe the reason why we went with Any here) is to do this in a way that's compatible with all Python versions we currently support (2.7, 3.5-3.11).

im not sure if this would work on python 2.7 (which is deprecated now, i couldn't find in the documentation about the TypeVar) however:

• it will certainly work for python 3.5 an above
• there is plenty of usages of TypeVar in some other modules so i guess it wouldn't be a problem for python 2.7 (https://github.com/search?q=repo%3Agetsentry%2Fsentry-python%20typevar&type=code)

let me know if im missing something or if you need anything else from me.

BTW, this is how i "fixed" it for my usage:

from typing import Any, Callable, TypeVar

import sentry_sdk

F = TypeVar("F", bound=Callable[..., Any])


def trace(func: F) -> F:
    return sentry_sdk.trace(func)  # type: ignore[no-any-return]

i am now using that trace instead of the sentry one

[agusdmb]

this might not seem like a big issue, but here at Nebuly we have the whole code base type hinted, and we recently started using sentry to find get more insight from different issues, but in some cases loosing the type hint caused more issues than the ones it helped discover with the traces

[sentrivana]

im not sure if this would work on python 2.7 (which is deprecated now, i couldn't find in the documentation about the TypeVar) however:

• it will certainly work for python 3.5 an above
• there is plenty of usages of TypeVar in some other modules so i guess it wouldn't be a problem for python 2.7 (https://github.com/search?q=repo%3Agetsentry%2Fsentry-python%20typevar&type=code)

Thanks for investigating, you're right, one of the occurrences, for instance, is in the WSGI integration, which is used across all Python versions, so looks like we're not as strict about type hints as I thought. Which makes sense, since in 2.7 typing was a rather niche thing, unlike now, so it's mostly important it works in newer versions.

In that case, your TypeVar suggestion sounds good!

let me know if im missing something or if you need anything else from me.

All good!

We probably won't get around to doing this soon, but PRs are very much appreciated -- I'll mark this as good first issue so that more folks see it and maybe someone has time to look into it earlier than we would.

[agusdmb]

@sentrivana ok great to hear that

i have considered opening a PR but i wanted to make sure the change was welcome before investing time on it... i can give it a try in the coming days

thanks