Skip to content

Make sure isinstance(typing_extensions.ParamSpec("P"), typing.TypeVar) is unaffected by sys.setprofile() #407

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

Merged
merged 13 commits into from
May 23, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions .github/workflows/publish.yml
Original file line number Diff line number Diff line change
Expand Up @@ -92,8 +92,8 @@ jobs:
export path_to_file=$(find dist -type f -name "typing_extensions-*.tar.gz")
echo "::notice::Unpacking source distribution: $path_to_file"
tar xzf $path_to_file -C dist/
cd ${path_to_file%.tar.gz}
python src/test_typing_extensions.py
cd ${path_to_file%.tar.gz}/src
python test_typing_extensions.py
Comment on lines +95 to +96
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had to make this change or the subprocess wasn't able to import typing_extensions. It was raising ModuleNotFoundError

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With your change it uses the copy of typing_extensions in the repo instead of the installed one; that is incorrect. We shouldn't do this.

Copy link
Member Author

@AlexWaygood AlexWaygood May 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, thanks. Do you have any idea why the subprocess wasn't able to import typing_extensions prior to this change, but only for this CI job? I don't fully understand that (see https://github.com/python/typing_extensions/actions/runs/9197903212/job/25299411282?pr=407)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh actually, this might be fine, since in this job we're not attempting to test an installed version of typing_extensions. I'm not actually sure why this doesn't work in CI though. Possibly the test itself is picking up the wrong copy of typing_extensions.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm actually not sure how the pre-existing tests currently pass without this? I think it's correct? Otherwise we unpack the version of typing_extensions we want into an src/ directory, but never cd into that src directory, right?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that's what it looks like.


test-sdist-installed:
name: Test installed source distribution
Expand Down
8 changes: 8 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,11 @@
# Unreleased

- Fix incorrect behaviour of `typing_extensions.ParamSpec` on Python 3.8 and
3.9 that meant that
`isinstance(typing_extensions.ParamSpec("P"), typing.TypeVar)` would have a
different result in some situations depending on whether or not a profiling
function had been set using `sys.setprofile`. Patch by Alex Waygood.

# Release 4.12.0rc1 (May 16, 2024)

This release focuses on compatibility with the upcoming release of
Expand Down
42 changes: 42 additions & 0 deletions src/test_typing_extensions.py
Original file line number Diff line number Diff line change
Expand Up @@ -5020,6 +5020,48 @@ def test_eq(self):
# won't be the same.
self.assertNotEqual(hash(ParamSpec('P')), hash(P))

def test_isinstance_results_unaffected_by_presence_of_tracing_function(self):
# See https://github.com/python/typing_extensions/issues/318

code = textwrap.dedent(
"""\
import sys, typing

def trace_call(*args):
return trace_call

def run():
sys.modules.pop("typing_extensions", None)
from typing_extensions import ParamSpec
return isinstance(ParamSpec("P"), typing.TypeVar)

isinstance_result_1 = run()
sys.setprofile(trace_call)
isinstance_result_2 = run()
sys.stdout.write(f"{isinstance_result_1} {isinstance_result_2}")
"""
)

# Run this in an isolated process or it pollutes the environment
# and makes other tests fail:
try:
proc = subprocess.run(
[sys.executable, "-c", code], check=True, capture_output=True, text=True,
)
except subprocess.CalledProcessError as exc:
print("stdout", exc.stdout, sep="\n")
print("stderr", exc.stderr, sep="\n")
raise
Comment on lines +5047 to +5054
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried running this in an isolated scope using exec(), but it was still causing some pollution of the global environment that was causing other, unrelated tests to fail. This seems to be the only way of doing a test with total interpreter isolation.


# Sanity checks that assert the test is working as expected
self.assertIsInstance(proc.stdout, str)
result1, result2 = proc.stdout.split(" ")
self.assertIn(result1, {"True", "False"})
self.assertIn(result2, {"True", "False"})

# The actual test:
self.assertEqual(result1, result2)


class ConcatenateTests(BaseTestCase):
def test_basics(self):
Expand Down
2 changes: 1 addition & 1 deletion src/typing_extensions.py
Original file line number Diff line number Diff line change
Expand Up @@ -1711,7 +1711,7 @@ def kwargs(self):

def __init__(self, name, *, bound=None, covariant=False, contravariant=False,
infer_variance=False, default=NoDefault):
super().__init__([self])
list.__init__(self, [self])
self.__name__ = name
self.__covariant__ = bool(covariant)
self.__contravariant__ = bool(contravariant)
Expand Down