Give Pyright what it wants (`alias` attributes everywhere)

[A5rocks]

I don't like pyright nor this behavior, but that doesn't mean users should have to care. We can add a test to prevent any sort of regression regarding this, forever.

cc @mikenerone cause this is from Gitter.

also @CoolCat467 I ran into issues with running the regenerate-files pre-commit hook and couldn't figure them out after about 15 minutes. It was complaining about attrs not being possible to import but unfortunately there's no way to get it to use the test-requirements.txt file. Have you run into this issue regarding it not picking up the virtual environment? (I'm using a seperate git client so that kind of makes sense, but now's the first time I ran into this...)

[codecov[bot]]

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 99.71%. Comparing base (8850705) to head (ce806e9). Report is 156 commits behind head on main.

Additional details and impacted files

```diff @@ Coverage Diff @@ ## main #3114 +/- ## ========================================== + Coverage 99.58% 99.71% +0.12% ========================================== Files 121 122 +1 Lines 18157 23605 +5448 Branches 3272 4022 +750 ========================================== + Hits 18082 23538 +5456 + Misses 52 48 -4 + Partials 23 19 -4 ``` | [Files with missing lines](https://app.codecov.io/gh/python-trio/trio/pull/3114?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=python-trio) | Coverage Δ | | |---|---|---| | [src/trio/\_core/\_local.py](https://app.codecov.io/gh/python-trio/trio/pull/3114?src=pr&el=tree&filepath=src%2Ftrio%2F_core%2F_local.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=python-trio#diff-c3JjL3RyaW8vX2NvcmUvX2xvY2FsLnB5) | `100.00% <100.00%> (ø)` | | | [src/trio/\_core/\_run.py](https://app.codecov.io/gh/python-trio/trio/pull/3114?src=pr&el=tree&filepath=src%2Ftrio%2F_core%2F_run.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=python-trio#diff-c3JjL3RyaW8vX2NvcmUvX3J1bi5weQ==) | `99.40% <100.00%> (+0.37%)` | :arrow_up: | | [src/trio/\_core/\_tests/tutil.py](https://app.codecov.io/gh/python-trio/trio/pull/3114?src=pr&el=tree&filepath=src%2Ftrio%2F_core%2F_tests%2Ftutil.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=python-trio#diff-c3JjL3RyaW8vX2NvcmUvX3Rlc3RzL3R1dGlsLnB5) | `100.00% <ø> (ø)` | | | [src/trio/\_tests/test\_exports.py](https://app.codecov.io/gh/python-trio/trio/pull/3114?src=pr&el=tree&filepath=src%2Ftrio%2F_tests%2Ftest_exports.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=python-trio#diff-c3JjL3RyaW8vX3Rlc3RzL3Rlc3RfZXhwb3J0cy5weQ==) | `99.68% <100.00%> (+0.06%)` | :arrow_up: | ... and [51 files with indirect coverage changes](https://app.codecov.io/gh/python-trio/trio/pull/3114/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=python-trio)

---

🚨 Try these New Features:

• Flaky Tests Detection - Detect and resolve failed and flaky tests

[jakkdl]

this makes me want to write a linter rule, but also feels ridiculous (and idk which flake8 plugin it would fit) to write an attrs+pyright-specific rule.

How slow/fast is the test? If it's anything but trivial I could rewrite it as an ast visitor.

[A5rocks]

this makes me want to write a linter rule, but also feels ridiculous (and idk which flake8 plugin it would fit) to write an attrs+pyright-specific rule.

How slow/fast is the test? If it's anything but trivial I could rewrite it as an ast visitor.

Yeah lint rule would make more sense but unfortunately a lint rule would miss context that's helpful (like whether a class is exported).

It takes 1.75 seconds to run locally, though that's with the overhead of starting pytest.

[A5rocks]

If test is slow, do we need to mark it with @slow or something?

Maybe? Here's the output at the end of pytest --durations=10 locally:

================================================ slowest 10 durations =================================================
3.66s call     src/trio/_tests/tools/test_gen_exports.py::test_process[from collections import Counter\n]
2.73s call     src/trio/_tests/tools/test_gen_exports.py::test_process[from collections import Counter\nimport os\n]
2.53s call     src/trio/_tests/tools/test_gen_exports.py::test_process[from typing import TYPE_CHECKING\nif TYPE_CHECKING:\n    from collections import Counter\n]
2.11s call     src/trio/_tests/test_socket.py::test_many_sockets
2.05s call     src/trio/_tests/test_socket.py::test_SocketType_connect_paths
2.04s call     src/trio/_tests/test_socket.py::test_address_in_socket_error
2.02s call     src/trio/_tests/test_highlevel_open_tcp_listeners.py::test_open_tcp_listeners_ipv6_v6only
1.73s call     src/trio/_tests/test_exports.py::test_pyright_recognizes_init_attributes
0.65s call     src/trio/_tests/test_exports.py::test_static_tool_sees_all_symbols[jedi-trio]
0.41s call     src/trio/_tests/tools/test_gen_exports.py::test_lint_failure

I don't think it's an appreciable slowdown. Maybe it's a good idea to put @slow on everything over 1 second? I can probably write the test to be faster (by not rescanning the tokenizations, not including tests in the glob). Actually I'll do just that.

EDIT: just the second optimization there changed it to 0.64s locally, so I think it's fine if not marked @slow.

[TeamSpen210]

There’s another approach we could use, but it’d might require starting an additional interpreter. Run a script which monkeypatches attrs.field to store whether alias was set into the metadata dict, import trio, then we can just loop through fields.

Unsure if that’d be faster though. We could avoid the new interpreter if the monkeypatch was done before any tests import trio, but that means it affects all tests. Probably not too much of an issue since metadata does nothing, and just wrapping the function should be fairly safe.

[A5rocks]

There’s another approach we could use, but it’d might require starting an additional interpreter. Run a script which monkeypatches attrs.field to store whether alias was set into the metadata dict, import trio, then we can just loop through fields.

Unsure if that’d be faster though. We could avoid the new interpreter if the monkeypatch was done before any tests import trio, but that means it affects all tests. Probably not too much of an issue since metadata does nothing, and just wrapping the function should be fairly safe.

It's not significantly faster:

(.venv) PS C:\Users\A5rocks\Documents\trio> hyperfine 'python -c \"import trio\"'
Benchmark 1: python -c "import trio"
  Time (mean ± σ):     410.5 ms ±  14.2 ms    [User: 45.3 ms, System: 22.9 ms]
  Range (min … max):   390.3 ms … 435.0 ms    10 runs

I think the main improvement would be that it's slightly more reliable. It sounds annoying to implement though.

[A5rocks]

@TeamSpen210 do you have any implementation ideas for your idea?

I was thinking about it just for the gain in reliability, but I think we would then get a flakey test because we delete and reimport trio to get all the warnings. I guess it's fine if we just patch attrs cause then the reimport will still be using the patched attrs?

[TeamSpen210]

Pushed an implementation using monkeypatching here. Unfortunately a downside I realised is that the monkeypatching code has to be either outside the trio package, or directly in __init__ before any other imports. So either this test can't be run by people just installing the package, or we'll have to have a module outside there. I could set it up to just skip if the monkeypatch "plugin" isn't specified though.

[Zac-HD]

I do like the "monkeypatch, and skip that test if not monkeypatched" plan 🙂

[Zac-HD]

@TeamSpen210 @A5rocks I'd really like to get this merged so I can use it; is there anything I can do to help?

[A5rocks]

@TeamSpen210 @A5rocks I'd really like to get this merged so I can use it; is there anything I can do to help?

Nope! I've just been pushing off working on some trio stuff for a bit, I'll incorporate better testing strategy soon:tm: (probably just using our already created pytest plugin nvm I forgot we can't + copy.copy() of the kwargs instead of picking and choosing. Everything else sounds fine)

[TeamSpen210]

Would you like me to make the monkeypatch approach a competing PR, or PR to yours, or do you want to stick with the current approach?

[A5rocks]

If you wouldn't mind, you should be able to commit to this PR.

[TeamSpen210]

Committed, didn't want to do big changes to other people's PRs without permission. Though it is easy to revert so probably that's fine.

[A5rocks]

Should we move the plugin to the (adjacent to src) tests directory or something? Having it in src feels weird...

[TeamSpen210]

That should work, if it's in sys.path so pytest can import.

[A5rocks]

That should work, if it's in sys.path so pytest can import.

Nevermind! I didn't realize pytest's sys.path schenanigans are the way they are. I'm surprised the test passes in CI! Maybe you can explain to me? I'm obviously missing something, because my mental model is:

• this works locally because pytest adds src/ to sys.path. then pytest looks there for the plugin
• but on ci... how? when? where? why would it add src/ to sys.path?

---

It feels like this shouldn't work -- if this is a pytest bug, I literally see no other way of adding to the path. The pythonpath option only applies after plugins (why??). ... well, other than setting the PYTHONPATH environment variable which would be annoying locally!

[TeamSpen210]

Not really sure either, but it's working so don't touch it?

[A5rocks]

Ohhhhh nooooo, it's installing _trio_check_attrs_aliases.py as a module into site-packages as part of the single wheel we make. That's quite the footgun!!!!

As a knee-jerk reaction I think we should revert back to before using a plugin. Maybe you see a way to work around this? Probably setting PYTHONPATH=......

list of packages in `site-packages`

``` + ls 'C:\hostedtoolcache\windows\Python\3.11.9\x64\Lib\site-packages\trio/../' 30fcd23745efe32ce681__mypyc.cp311-win_amd64.pyd 3204bda914b7f2c6f497__mypyc.cp311-win_amd64.pyd MarkupSafe-2.1.5.dist-info OpenSSL OpenSSL-stubs README.txt __pycache__ _black_version.py _cffi_backend-stubs _cffi_backend.cp311-win_amd64.pyd _distutils_hack _pytest _trio_check_attrs_aliases.py alabaster alabaster-0.7.13.dist-info astor astor-0.8.1.dist-info astroid astroid-3.2.4.dist-info async_generator async_generator-1.10.dist-info attr attrs attrs-24.2.0.dist-info babel babel-2.16.0.dist-info black black-24.8.0.dist-info blackd blib2to3 build build-1.2.2.post1.dist-info certifi certifi-2024.8.30.dist-info cffi cffi-1.17.1.dist-info cffi-stubs charset_normalizer charset_normalizer-3.3.2.dist-info click click-8.1.7.dist-info codespell-2.3.0.dist-info codespell_lib colorama colorama-0.4.6.dist-info coverage coverage-7.6.1.dist-info cryptography cryptography-43.0.1.dist-info dill dill-0.3.9.dist-info distutils-precedence.pth distutils-stubs docutils docutils-0.20.1.dist-info docutils-stubs idna idna-3.10.dist-info imagesize imagesize-1.4.1.dist-info imagesize.py iniconfig iniconfig-2.0.0.dist-info isort isort-5.13.2.dist-info jedi jedi-0.19.1.dist-info jinja2 jinja2-3.1.4.dist-info markupsafe mccabe-0.7.0.dist-info mccabe.py ```

[TeamSpen210]

We could set PYTHONPATH I think yes? I was going to do that but didn't trust myself with changing the complicated shell script. Probably should put the plugin in its own folder to isolate. I don't think it matters that the test won't run other than in CI, since it's only going to fail if the source code changes, not due to environment etc.

Another approach would be to use usercustomize perhaps, copying the module into site-packages ourselves.

[TeamSpen210]

The plugin script can't be anywhere in the trio package, because then trying to import it implicitly imports trio first, before we hooked anything. That's why I gave it such a long name.

[A5rocks]

The plugin script can't be anywhere in the trio package, because then trying to import it implicitly imports trio first, before we hooked anything. That's why I gave it such a long name.

Yeah the issue with that commit was that we had -p trio._tests.pytest_plugin in addopts, which then messes things up because turns out pytest runs plugins in the order they're provided. Anyways we don't need that anymore, so I removed it.

---

nevermind, I should know better than make statements based on things running locally... turns out conftest always worked except for --run-slow and that's why we switched. :/ I completely forgot.