fixtures: remove dirty optimization for request.getfixturevalue()

[bluetech]

Currently for each Function we copy the FunctionDefinition's
_arg2fixturedefs. This is done due to an ineffective optimization for
a dynamic request.getfixturevalue() when the fixture name wasn't
statically requested. In this case, we would save the dynamically-found
FixtureDef in _arg2fixturedef, such that if it is requested again in
the same item, it is returned immediately instead of doing a
_matchfactories check again. But this case is already covered by the
_fixture_defs optimization.

I've always disliked this copy and mutation. The _arg2fixturedefs
shenanigans performed during collection are hard enough to follow, and
this only adds to the complexity, due to the mutability and having
multiple different _arg2fixturedefs with different contents.

So summing up:

Pros: faster repeated request.getfixturevalue() in same test (ineffective)
Cons: complexity (reasoning about mutability), extra copy

Even without the _fixture_defs optimization, since
request.getfixturevalue() is mostly a last-resort thing, so shouldn't
be too common, and repeated calls to it in the same test should be
even less common, and if so _matchfactories shouldn't be that slow,
it should be fine to remove it.

[Anton3]

Fixtures are cached per-test in _fixture_defs anyway, so LGTM

[bluetech]

@Anton3 Good point, so the perf con is actually not the case. I will amend the commit message.

Aside on `_fixture_defs`

Speaking of _fixture_defs, I actually think that the _fixture_defs optimization is subtly broken, e.g. I think (but not 100% on it) that this test should pass:

import pytest

@pytest.fixture(scope='module')
def fix(): return "module"

@pytest.fixture()  # scope='module' should also work
def modfix(fix): return fix

class TestIt:
    @pytest.fixture
    def fix(self): return "function"

    def test_it(self, modfix):
        assert modfix == "module"

Currently it has modfix == "function". And if modfix is scope="module", it fails with a scope error... modfix should only see the module fix, the class fix is not visible to it, so it breaks modularity that the class-level fix changes the behavior of the module-level modfix.

[Anton3]

Aside on _fixture_defs

That's probably due to faulty logic in FixtureDef.execute, #14104 should fix it.

[bluetech]

That's probably due to faulty logic in FixtureDef.execute, #14104 should fix it.

#14104 doesn't touch the _fixture_defs optimization, so doesn't change the behavior of the test.

Just as an experiment, I did try now to comment out the _fixture_defs early-exit and see if anything breaks. One test fails, test_fixture_closure_with_overrides_and_intermediary, it's a test I recently added for an unrelated issue. I didn't analyze it too much. Interestingly, when doing the same commenting-out on top of #14104, all tests pass.

(Note: I just noticed that even with commenting out _fixture_defs the test still fails. That's because we currently always run _matchfactories against the item's visibility, not the SubRequest's. That's potentially another issue. But anyway, really getting off topic for this PR 😀 )