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

'"mock_fixture" is not accessed', and 'Import cannot be resolved' in editor, works fine when running #684

Closed
macintacos opened this issue Dec 3, 2020 · 30 comments
Assignees
Labels
waiting for user response Requires more information from user

Comments

@macintacos
Copy link

Hello! I have an issue where Pylance cannot seem to determine where my imports are and it cannot access a fixture I've created via pytest, but the code runs fine with no errors.

Feel free to clone the following repo, which is minimally created so that I could reproduce the issue
in a small project instead of the larger one I'm actually working on: https://github.com/macintacos/pylance-pytest-repro

After getting setup and verifying the code works as expected, if you check out tests/test_pylance.py, you'll notice that there is a mock_fixture being passed into the only test in there. Removing that will actually cause the test to fail, so it is working properly and being imported, however I'm seeing a warning from Pylance that says "mock_fixture" is not accessed, which is obviously incorrect since the tests pass with it.

Additionally, if you open tests/conftest.py, it is saying Import "fixtures.test_fixtures" could not be resolved even though, as we can see if we remove that import, the tests fail because Pytest can't find the fixtures that were imported via conftest.py.

As far as I know, this is the correct way to set up fixtures with Pytest (as evidenced by the tests passing, and the documentation that I've been poring over), so I'm not sure why Pylance can't resolve these things.

Environment data

  • Language Server version: 2020.11.2 (pyright 750a296b)
  • OS and version: macOS 10.15.7
  • Python version (& distribution if applicable, e.g. Anaconda): 3.8.6

Expected behaviour

Pylance can find the imports and understands that the fixture is being accessed.

Actual behaviour

Pylance can't find the imports and doesn't understand that the fixture is being accessed.

Logs

I didn't see any errors with trace logging, so I don't think logs will be relevant here. This looks like an issue with Pylance itself.

Code Snippet / Additional information

As mentioned above, code for a repro is in https://github.com/macintacos/pylance-pytest-repro

Please let me know if you need any additional information to track this down. Thanks!

@github-actions github-actions bot added the triage label Dec 3, 2020
@macintacos
Copy link
Author

I actually just realized that there was an update to Pylance recently, however I have since updated it and I'm still seeing the same thing.

@judej judej added the needs investigation Could be an issue - needs investigation label Dec 3, 2020
@github-actions github-actions bot removed the triage label Dec 3, 2020
@jakebailey
Copy link
Member

The issue here is that you're importing as though tests is an import root, but there's no configuration that says that should happen. You're getting some import root modification automatically (we detect src as an import root), but without more configuration we're expecting import tests.fixtures.test_fixtures.

See: https://github.com/microsoft/pylance-release/blob/master/TROUBLESHOOTING.md#unresolved-import-warnings

@jakebailey jakebailey added waiting for user response Requires more information from user and removed needs investigation Could be an issue - needs investigation labels Dec 3, 2020
@macintacos
Copy link
Author

macintacos commented Dec 3, 2020

Ah-ha, thanks @jakebailey, that solves one-half of the mystery 🙂 The import issue has seemed to have gone away now.

However, the "mock_fixture is not accessed" issue remains. That troubleshooting page doesn't seem to address that particular issue.

@jakebailey
Copy link
Member

What is mock_fixture? It is a special injected module that doesn't actually exist on disk?

@macintacos
Copy link
Author

macintacos commented Dec 3, 2020

mock_fixture is defined here: https://github.com/macintacos/pylance-pytest-repro/blob/trunk/tests/fixtures/test_fixtures.py

It essentially intercepts the call to app.return_data in the test and returns the return_value defined there. It gets there by way of conftest.py (docs explaining that bit are here if you need it: https://docs.pytest.org/en/2.7.3/plugins.html?highlight=re)

@jakebailey
Copy link
Member

Oh, my mistake, I didn't read "not accessed" and thought it was still an import resolution issue. Oops.

"Not accessed" is not a real diagnostic; it should only exist to state that something is unused and therefore gray it out (but not say anything in the problems window as a regular diagnostic). Is that what you're seeing here? Or is it showing up some other way?

@macintacos
Copy link
Author

Yeah that's what I'm seeing, however this feels like it shouldn't be the case - it is being used, it's just not being called directly in the function. I was hoping there would be some way to make it so that the editor understands that it's actually being utilized, since it seems to confuse myself and folks that are PR'ing my code 🙂

@macintacos
Copy link
Author

Is there any way to hint to Pylance that the parameter in that test method is being used, so that it treats it as I would expect (i.e. not say that it's "not accessed")?

@jakebailey
Copy link
Member

jakebailey commented Dec 3, 2020

This is special dynamic behavior implemented by pytest, we can't really know what any given library is actually going to do under the hood when it comes to special behavior like this. From a static analyzer's point of view, it's just a parameter that isn't being used in the function body. The fact that pytest works like this (repurposing a parameter and inspecting for it at runtime) is a little unfortunate.

If you don't want to show that, you could try sticking # type: ignore on the line where the parameter is present, or disable that diagnostic for the whole file with # pyright: reportUnusedVariable = false at the top (though you will lose other valid messages too).

@macintacos
Copy link
Author

Thanks, adding # type: ignore to the top of the file seems to have "fixed" it, but I couldn't seem to find a way to set it for just the line. Could you show me how you would call that in test_pylance.py if you could?

@erictraut
Copy link
Contributor

Adding # type: ignore at the top of the file will suppress all type-checking errors for the entire file. Is that what you want? You could be more selective and use # pyright: reportUnusedVariable=false if you want to disable only that diagnostic rule within the file. That's how my team deals with the unfortunate way in which pytest fixtures work.

@macintacos
Copy link
Author

macintacos commented Dec 3, 2020

No, I'd like to disable it for just the def line if possible. Where do I place that reportUnusedVariable or type: ignore line to make that possible in the following code?

import app


def test_pylance_with_fixtures(mock_fixture):
    # Note that this is NOT "some data", and passes, but Pylance claims "mock_fixture" is not accessed
    expected = ["fixture data"]
    actual = app.return_data()
    assert expected == actual

@erictraut
Copy link
Contributor

If you place # type: ignore at the end of a line, it will suppress all diagnostics for that line. If you place it at the top of a file (on its own line), it will suppress all diagnostics for the entire file. This behavior is defined in PEP 484.

@jakebailey
Copy link
Member

jakebailey commented Dec 3, 2020

import app


def test_pylance_with_fixtures(mock_fixture): # type: ignore
    # Note that this is NOT "some data", and passes, but Pylance claims "mock_fixture" is not accessed
    expected = ["fixture data"]
    actual = app.return_data()
    assert expected == actual

Is what I would expect to work.

We currently don't support any other way to suppress on a line or region. I think there's an open issue for that somewhere...

@macintacos
Copy link
Author

Unfortunately that doesn't appear to be working with my setup:

image

@jakebailey
Copy link
Member

You must be using the error lens extension. This is a known issue where they're showing these UI-only diagnostics as regular diagnostics: #272

@jakebailey
Copy link
Member

See: phindle/error-lens#35

@jakebailey
Copy link
Member

But, I'm surprised to see # type: ignore not working for this case...

@macintacos
Copy link
Author

I disabled Error Lens, reloaded, and am seeing the same message on hovering over mock_fixture. Not sure it's Error Lens, since when I put it at the top of the file the "not accessed" message goes away.

I have a fairly extensive configuration so something else might be going on here, so I'll consider this solved for the time being. If I run out of ideas, I'll open a separate issue. Thanks @jakebailey and @erictraut for your speedy responses!

@jakebailey
Copy link
Member

Yes, hovering will still show it still (as it's being grayed out), I just meant the big message off to the side. You can configure the extension to not explicitly show "hints" if you'd prefer. In VSC, you can have a diagnostic marked as a hint, and they're supposed to have minimal UI (small ... or the color change), but the error lends makes them more visible.

@macintacos
Copy link
Author

@jakebailey but there's no difference between having type: ignore there and not having it in that case (with or without Error Lens, the behavior is exactly the same with that annotation), whereas if it's at the top of the file there's no "hint" shown saying that the parameter can't be accessed (again, with Error Lens and without).

@jakebailey
Copy link
Member

Yes, because # type: ignore isn't working on that line for some reason. Sticking it at the top disables diagnostics completely.

@erictraut
Copy link
Contributor

# type: ignore won't disable the grayed-out text because it's not an error/warning/information diagnostic. Grayed-out text is implemented under the covers with a special "hint" diagnostic. I'm guessing that Error Lens is incorrectly treating it as an error or warning rather than treating it as a "hint" and ignoring it. We've seen other VS Code extensions that contain this bug. If that matches the behavior you're seeing, then I recommend filing a bug against Error Lens. They shouldn't be treating hints as real diagnostic messages.

@ikalnytskyi
Copy link

@jakebailey

This is special dynamic behavior implemented by pytest, we can't really know what any given library is actually going to do under the hood when it comes to special behavior like this. From a static analyzer's point of view, it's just a parameter that isn't being used in the function body. The fact that pytest works like this (repurposing a parameter and inspecting for it at runtime) is a little unfortunate.

Does it mean that there are no plans to support pytest? I mean, I know this is weird, yet pytest is probably the most popular test framework out there. https://github.com/palantir/python-language-server supports it (probably via https://github.com/davidhalter/jedi), and it would be nice to see some support here as well.

@jakebailey
Copy link
Member

jakebailey commented Dec 14, 2020

I guess I don't know what you mean by "support pytest", per se. pytest is actually fairly well annotated, and we support the standardized annotations/stubs. As far as I'm aware, things work pretty well with pytest (besides import resolution, which is a universally difficult thing to get right in an editor).

This specific issue is just the detection of unused variables, where pytest is unique in that it treats certain function parameters as names to do other lookups on, and as such they aren't exactly "unused"; that's the bit that is non-standard, but this only affects the visuals in the editor of us graying out the text as if it were any other function, it'd be dead code.

I don't think jedi does dead code analysis; maybe pylint does, but that's a completely different kind of beast.

@ikalnytskyi
Copy link

This specific issue is just the detection of unused variables

Yes, I understand. But in the conversation above it was mentioned dependency injection performed by pytest in runtime (which are specified as function parameters). And so my question is, is there any plans to support completion of pytest fixture? I.e. when you write a test it'd be nice to complete its arguments based on the known pytest fixtures. Jedi, btw, supports this (link).

@jakebailey
Copy link
Member

I see, that'd be an enhancement we could consider if it's requested enough. Would you mind making a new issue for it, potentially with an example?

@macintacos
Copy link
Author

macintacos commented Dec 15, 2020

# type: ignore won't disable the grayed-out text because it's not an error/warning/information diagnostic. Grayed-out text is implemented under the covers with a special "hint" diagnostic. I'm guessing that Error Lens is incorrectly treating it as an error or warning rather than treating it as a "hint" and ignoring it. We've seen other VS Code extensions that contain this bug. If that matches the behavior you're seeing, then I recommend filing a bug against Error Lens. They shouldn't be treating hints as real diagnostic messages.

@erictraut I mentioned this before, but I don't believe it has anything to do with Error Lens, since when I disabled it I saw the exact same behavior. What you said here about putting # type: ignore in-line with the method call to disable the diagnostic for that line doesn't seem to work, regardless of Error Lens being enabled or not.

Although maybe I'm missing this bit; is # type: ignore not meant to suppress "hints" when it's used on a line? Does it only disable hints when supplied at the top of the file?

@jakebailey
Copy link
Member

jakebailey commented Dec 15, 2020

By "exact same behavior", do you mean on hover? That is still expected behavior; there's no way to make VS Code hide the reason why it's graying out some piece of code.

We were talking about Error Lens thanks to screenshots like #684 (comment) which show the diagnostic text repeated again off to the side in a code lens, because the extension by default shows all hint diagnostics (which are used for graying/striking out code).

@erictraut
Copy link
Contributor

Let me explain what I meant by "not meant to suppress hints". The language server protocol allows language servers to return diagnostics. Each diagnostic contains a text message, a document range (start/stop), and a "severity". The severity is one of the following: error, warning, informational, hint. In general, Pylance doesn't generate diagnostics of the form "hint". There's one exception though. There's a special form of "hint" that allows an optional "tag" to be added to the diagnostic. If this tag is present, it tells the client that it shouldn't display the hint as a regular diagnostic but instead interpret it as a request to display the text in gray. Pylance uses this technique for unaccessed variables and unreachable code. The VS Code editor properly interprets these "tagged hints", but we've seen that some extensions don't properly handle them.

With all of that context in mind, when I said that # type: ignore does not suppress "hints", I was talking specifically about those "tagged hint" diagnostics. It will suppress errors, warnings or informational diagnostics on that line. That's what it's designed to do.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
waiting for user response Requires more information from user
Projects
None yet
Development

No branches or pull requests

5 participants