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

reportUnusedVariable ignore pattern #1118

Closed
smackesey opened this issue Oct 23, 2020 · 111 comments
Closed

reportUnusedVariable ignore pattern #1118

smackesey opened this issue Oct 23, 2020 · 111 comments
Labels
addressed in next version Issue is fixed and will appear in next published version enhancement request New feature or request

Comments

@smackesey
Copy link

Is your feature request related to a problem? Please describe.

PyRight at present seems to have no way to fine tune the reportUnusedVariable setting to ignore variables matching certain patterns. When implementing a function/method with a third-party-specified interface, it often happens that there will be some function arguments that you write out but don't use. It should be possible to suppress diagnostics for these "expected" unused arguments without turning off unused variable warnings entirely. But at present, it appears that reportUnusedVariable can only be toggled entirely on or off.

Describe the solution you'd like

A common convention for this situation is to prefix the unused arguments with _. Other static analysis tools permit specification of a pattern that matches variable names for which to suppress unused warnings (see varsIgnorePattern for eslint's no-unused-vars) A similar option could be implemented in PyRight.

@erictraut
Copy link
Collaborator

Can you provide an example of a "function argument that you write out but don't use"? Function arguments are expressions, and they represent "a use" of a variable, so I'm a bit confused by what you mean here.

Pyright suppresses this diagnostic rule if the variable is named _. If you want to disable the rule completely, you can do so on a file-by-file basis (using "# pyright: reportUnusedVariable=false") or for the entire workspace or user (using the settings).

@erictraut erictraut added the question Further information is requested label Oct 23, 2020
@smackesey
Copy link
Author

smackesey commented Oct 23, 2020

I don't mean a function argument in the call, I mean in the signature (so, not an expression). A simple example is a method that doesn't use self:

class A:
    def my_method(self):  # this will be flagged with an `unusedVariable` because `self` is not used
        print('this is my method')

It is good to know that PyRight suppresses the warning if the variable is named _, but oftentimes one wants to keep the variable name for readability purposes (and in case you change the method to actually use the variable). Also one might leave multiple arguments (which can't have the same name) unused. That is why one cross-language convention is to mark unused variables/arguments with a leading underscore, like _variable.

@erictraut
Copy link
Collaborator

erictraut commented Oct 23, 2020

Function parameters (even if they are unused) never trigger the reportUnusedVariable diagnostic rule. They will be displayed as "grayed out" if they are unused and your editor supports that feature, but no error or warning is emitted in this case.

@smackesey
Copy link
Author

Hmm, I am getting a warning from Pylance, which I understand uses Pyright and works off the Pyright config file. But maybe Pylance's warning in this case is being generated by different machinery.

@erictraut
Copy link
Collaborator

You are seeing this with the code snippet you provided above? Or with some other code? Can you provide either a screen shot or a code snippet?

@smackesey
Copy link
Author

OK, I'm sorry, it is an information rather than warning diagnostic. My editor (NeoVim/CoC) displays "information" diagnostics similarly to how it displays "error" and "warning" diagnostics. But I suppose it is the client's responsibility to display this appropriately, rather than PyRight's responsibility not to send the information. Thanks for your help, I'll close the issue.

@erictraut
Copy link
Collaborator

OK, I'm confused on two points:

  1. You said you were running Pylance, but Pylance works only with VS Code. Are you sure you're not using Pyright? It works with NeoVim. Or perhaps you just used Pylance with VS Code to try to repro this problem?

  2. Pyright does not emit these as "information" diagnostics. It emits them as "hint" diagnostics with a special "tag" that indicates that it should be displayed as "grayed out". Pyright explicitly checks to see if the client supports this tag and does not emit these hint diagnostics if the client says that it doesn't support the tag. I specifically added this check for clients like NeoVim that don't support the "grayed out text" functionality. Based on what you're seeing, it appears that NeoVim is reporting that it supports this tag but then is not dealing with it correctly. If that's the case, you should file a bug against NeoVim. It should either report that it doesn't handle this tag or it should properly honor these hint diagnostics with the "Unnecessary" tag.

@smackesey
Copy link
Author

smackesey commented Oct 24, 2020

  1. It is possible to use Pylance with other clients "off-label", all you have to do is download it and point your LSP client at the included dist/server.bundle.js`. I also use Pylance in VSCode (which I use for notebooks), so I just configured NeoVim/CoC to use the version downloaded through VSCode.

  2. That is interesting and useful information. I assumed it was information because that was the only other (i.e. not error or warning) category of diagnostic I saw listed in PyRight config docs, and it is showing up as a diagnostic. Including a screenshot here to show you what it looks like:

image

I will investigate why it is not showing as grayed-out and will post an update here when I figure it out. Perhaps it is a function of off-label use of Pylance, and if I just used PyRight it will show correctly.

@arjenzorgdoc
Copy link

(I'm responding to the title of this issue)

I like to name values I ignore:

def get_bar_v1(s: str):
    _foo, bar, _baz = s.split()
    return bar

def get_bar_v2(s: str):
    _, bar, _ = s.split()
    return bar

I think v1 is "better" than v2, but pyright doesn't agree:

  2:5 - error: Variable "_foo" is not accessed (reportUnusedVariable)
  2:16 - error: Variable "_baz" is not accessed (reportUnusedVariable)

@erictraut
Copy link
Collaborator

Symbol names that begin with underscores are treated as "private" variables. For example, if you declare a class variable:

class Foo:
    _foo: int = 3

The single underscore name is specifically reserved for variables that are unused but are placeholders.

If you don't like this convention, you can disable the reportUnusedVariable diagnostic rule in your project.

@mike-kfed
Copy link

I agree with @arjenzorgdoc his v1 is nicer, especially when you read foreign code you have an idea what the other split elements are supposed to be. Disabling reportUnusedVariable as suggested is not a good solution as the hint which variables are unused is actually good to have information. I'd vote for a dummy-variables-rgx config option like pylint has, which by default looks like (_+[a-zA-Z0-9]*?$). This is a long used practice in the python community, and therefore leading underscore is not just for pseudo-private class variables (they are actually accessible, it's just a naming convention). Having an ignore dummy variables pattern avoids noise in warnings/hints shown while helping keeping code clean.

@bartenra
Copy link

bartenra commented Apr 6, 2021

@mike-kfed That might already be implemented

In my case, I am defining callbacks which I am passing to a framework.
Some of the arguments that the callback receives are not used.

This gives me a yellow squiggly line for both arguments:

def callback(parent: Any, info: Any) -> None: ...

This does not:

def callback(_parent: Any, _info: Any) -> None: ...

but on hover I still see the message "_info" is not accessed" (but it's not a warning).

@mike-kfed
Copy link

@bartenra I never claimed it is a warning, it is a hint, which is what you are seeing when you are hovering. The whole point made in this issue is, that this shouldn't be flagged by the linter at all when the variable name matches a certain agreed upon pattern, like other linters already support (not just for Python but other programming languages too, e.g. Rust analyser doesn't flag it when you use the underscore prefix). This would greatly help to keep the list of code-smells to fix limited to actual things that need fixing plus keep readability of code. I use vim, there I can get a list of all things found by my linters, which is convenient to use when improving code quality (given the linters only show report-worthy things). So no it has not been implemented as of today - and for some reason seems to be out of interest to ever be.

@thotypous
Copy link

Please reopen. This is a feature request, not a bug report.

@erictraut
Copy link
Collaborator

@thotypous, feel free to upvote (thumbs up) this issue if you are interested in it. We occasionally look at closed feature requests and reopen then if there is sufficient interest.

@novasenco
Copy link

novasenco commented Jun 2, 2021

I have a valid use-case. Inside of a script that serves as a plugin and is loaded externally (with importlib), every function I make is marked as "not accessed". For example,

from foo import command

def main():
    print("script loaded")

@command("PING")
def pong(server, message):
    server.send_pong(message.params[0])

# etc
# pong() is not called in any script directly, but rather it is loaded with importlib

Indeed, I was searching for a way to disable this diagnostic (on both a file level and on a per-function level). It's quite surprising it's not in and feels frustrating.

@erictraut
Copy link
Collaborator

You can disable any diagnostic on a file level by using a comment of the form:

# pyright: reportUnusedVariable=false

You can similarly change the severity of a diagnostic (e.g. from error to warning):

# pyright: reportUnusedVariable=warning

Of course, you can adjust the severity for (or completely disable) any diagnostic rule for your entire project if you don't like its behavior.

@lucatpa
Copy link

lucatpa commented Jun 8, 2021

I'm facing this issue now, I think there is no easy way to avoid noise if you are just declaring functions to fulfill some interface but you only want to use some arguments for that function, not all of them. This will give you a HINT that you just have to learn to always ignore. It would be good to be able to disable this diagnostic on a per-function fashion.

Maybe an option can be added to the # pyright: marker, like (following the callback example):

# pyright: symbol=callback reportUnusedVariable=false
def callback(parent: Any, info: Any) -> None: ...

The comment could be also put at the start of the file, and will only activate that option for the symbol callback in the current file.

@erictraut
Copy link
Collaborator

The reportUnusedVariable check never generates diagnostics for parameters, so disabling it would have no effect. The hint generated for parameters is not a diagnostic. It's just a hint that tells the editor (if it supports such hints) to display the corresponding text in a subtle grayed-out appearance.

@tshu-w
Copy link

tshu-w commented Jun 10, 2021

@erictraut So If there is an option to hidden these hints or don't show these hints when variables start with _ will be nice. On Emacs we can't distinguish between these hints and information diagnostics.

@erictraut
Copy link
Collaborator

I would consider that a bug in the LSP support for emacs. Please work with the maintainers of this functionality to address this limitation.

If the client (emacs or otherwise) specifies that it doesn't support hints, pyright will not generate them. So one option is to add a configuration flag to the emacs LSP support that disables this client capability.

@jason0x43
Copy link

I've run into this issue pretty frequently as well. The problem is, as several others have pointed out, that the "is not accessed" hints end up creating a lot of noise. I know I can disable hints in the editor, but it seems heavy handed to have to disable all hints just to reduce the clutter generated by this one.

@erictraut
Copy link
Collaborator

As I said above, this is just a hint that should cause the editor to display the text in a "grayed out" fashion, giving you a subtle clue that the symbol isn't accessed. It is not meant to be an error or warning. If your editor is displaying it in some other (heavy-handed) way, then you should lobby for your editor maintainer to change it. It works great in VS Code.

@lucatpa
Copy link

lucatpa commented Jul 27, 2021

This makes sense, I just brought it up to coc-pyright developers: fannheyward/coc-pyright#107 (comment)

@clason
Copy link

clason commented Mar 23, 2023

if those choices deviate from the semantic intent of the spec and from the expectations of your users, you might want to reconsider your choices.

I'm sorry, but that is putting the cart before the horse. If the specification does not express the semantic intent clearly enough, it's clearly not doing its job properly. The whole point of a specification is not having to rely on an interpretation of "author intent".

In any case, I have reached the end of my limited patience with "language servers" that treat their own specification like a pirate's code that is subordinate to VS Code's behavior. What I am taking away from this discussion is that pyright is -- by your own admission -- in fact not (intended to be) a language server according to the specification and merely happens to use the protocol.

@erictraut
Copy link
Collaborator

You keep mentioning "intent" like there is one, but AFAIK this is not covered in the spec.

Here's what I mean when I say "intent". The spec defines diagnostic tags. It defines numeric values for two tags, and it gives symbolic names to each. The comments in the code (included in the spec and pasted below for reference) further explain the intended meaning of each tag. And it suggests a possible UI treatment that would be appropriate for rendering each tag. Perhaps this could be made clearer, but by my reading it's pretty clear that there was an intended meaning and use case behind each of these tags. Do you see it differently?

/**
 * The diagnostic tags.
 *
 * @since 3.15.0
 */
export namespace {
	/**
	 * Unused or unnecessary code.
	 *
	 * Clients are allowed to render diagnostics with this tag faded out
	 * instead of having an error squiggle.
	 */
	export const Unnecessary: 1 = 1;
	/**
	 * Deprecated or obsolete code.
	 *
	 * Clients are allowed to rendered diagnostics with this tag strike through.
	 */
	export const Deprecated: 2 = 2;
}

We will stick as closely to the spec as possible.

It makes sense that you wouldn't want to violate the spec, but the spec provides latitude in UI design choices — and even suggests a potential UI treatment that would be appropriate in this case. Do you see it as a violation of the spec to adopt this suggestion? Or perhaps you're concerned that the suggested approach has some downsides relative to your current UI treatment for these tags?

Do you have any experimentation capabilities (e.g. for A/B testing)? I wonder if it would be worth trying this treatment as an experiment and getting feedback from users. That's a technique we frequently employ to inform our UX decisions. Just a thought.

@clason
Copy link

clason commented Mar 23, 2023

Do you see it as a violation of the spec to adopt this suggestion?

We see it as a violation of the spec on your part to require (or even assume) that this suggestion is adopted. I don't know how to make it clearer than that.

@clason
Copy link

clason commented Mar 23, 2023

And I appreciate the suggestions, but we are just not interested in tweaking the UX to account for a single language server's idiosyncrasies -- that would defeat the whole purpose of the language server protocol.

@lewis6991
Copy link

I think VS Code doesn't show hint diagnostics at all other than a squiggly line, so the subtlety of this check makes sense within that editor.

However other editors (including ours) show hints like all other severities. In general this has been a good default behaviour with servers since diagnostics are usually actionable.

So implementing the suggestion doesn't make sense to us since it becomes very special case because it becomes a function of severity+tag+'UI element' which overall makes configurability very convoluted. By treating each severity equal, we can make configuration flexible. If we just follow the LSP spec, then we only need to change the highlighting.

And as you said, pyright isn't really a language server anymore, so it also wouldn't make much sense to make significant changes to our UI just for a server like this one.

@erictraut
Copy link
Collaborator

I don't think pyright is as idiosyncratic as you are stating here. Surely there are other language servers other than pyright and TypeScript that use these tags to indicate blocks of code that cannot be reached, etc.

So implementing the suggestion doesn't make sense to us ...

Fair enough.

@jason0x43
Copy link

Pyright isn't alone in using hint tags to indicate unused code. The key difference between pyright and other language servers (and the reason this issue was originally opened) is that pyright doesn't provide the ability to silence specific instances of hints. With the TS language server, I can silence a specific instance of a hint if I know it's not actionable, either with a convention (underscore) or a comment. With pyright, my options are to either show all the hints (which may include a lot of non-actionable hints, obscuring the actionable ones), or turn all the hints off, which means I'm missing out on the benefits of hints.

On a related note to @lewis6991's comment yesterday about assigning unused arguments to _, I saw this recommendation from the PyLint manual, which also works:

        def _null_enum(validator, enums, instance, schema):
            del validator, enums, instance, schema

@joaotavora
Copy link

FWIW I also can't understand why prepending my variable's name with _ won't make pyright shut up about it. Sigh. It works if I name the variable _, losing semantic information about what the variable was suppose to mean if it were ever used.

@carschandler
Copy link

I'd love to see _unused implemented as a way of ignoring unused variables as well! It's definitely the most common convention, e.g. Rust has this built-in.

@noamraph
Copy link

noamraph commented Oct 15, 2023

I think that warning on unused variables is very valuable, and I find this pattern to be very useful:

a, b, _c, _d = get_something()

It lets me declare that it's OK that I'm not using the third and fourth return value of a function.
I find it much clearer than a, c, _, _ = get_something(), since then I can know what other return values are available, without having to look up the reference.

Why not just make pyright treat _a like it treats _?

@Cutipus
Copy link

Cutipus commented Dec 16, 2023

I also have been wrapping my head around this.
signal(SIGINT, lambda signo, frame: self.quit()) complains that signo and frame are unused variables - which is true, but I just have no use for these required variables. I thought adding # pyright: ignore would solve it but apparently that just doesn't work. I discovered that _ removes the issue, but that only works for one variable.

Seeing the discussion here it seems like my code will never have that beautiful green ✅ in my editor.

@joaotavora
Copy link

I discovered that _ removes the issue, but that only works for one variable.

People are even suggesting changing Python itself because of this: https://discuss.python.org/t/allow-the-parameter-name-underscore-multiple-times-in-a-function-signature/17475/13 Fortunately some sane people over there agree this is a linter issue.

The real solution of course would be for pyright to ignore this warning for any _-prefixed argument, like most linters.
In that forum they provide a half-decent workaround that works

def foo(used, _a, _b):
    assert _a and _b
    ...

should work most of the time, if you can spare the CPU cycles (if you can't, why are you using Python?)

@Cutipus
Copy link

Cutipus commented Dec 17, 2023

In that forum they provide a half-decent workaround that works

def foo(used, _a, _b):
    assert _a and _b
    ...

Unfortunately this workaround is impossible to do on lambda expressions as you can't put statements in Python's limited lambda expressions. I mostly run into this situation when writing callback functions because they frequently provide more parameters than you actually need, and lambdas fit that niche. If I had to apply this "workaround" to my code I would need to define a function, write the assert or del on the parameters I don't need, and then execute another line of code calling for the clean function that only takes the required parameters. Adding 3 more code lines just to silence pyright.

@joaotavora
Copy link

🤷 I wonder if someone could post a diff here to do this change, just to see if it's very complicated. I might try later, if noone beats me to it.

@erictraut
Copy link
Collaborator

Since there seems to be ongoing interest in this thread, let me summarize its contents for the benefit of those who don't want to read the full history.

The reportUnusedVariable diagnostic rule

  • The reportUnusedVariable is a strict-mode diagnostic rule that is disabled by default. If you don't want pyright to emit a diagnostic for this condition, do not enable this diagnostic rule. Alternatively, you can suppress these diagnostics on a per-file or per-line basis.
  • The reportUnusedVariable rule never emits a diagnostic for parameters, since it's common in python for parameters to go unused.

Hints with Unnecessary tag

  • Pyright makes use of tagged hints (using the Unnecessary tag) in various places to indicate to the client that it should display the text in a dimmed-out manner. This is used for unreachable code, unreferenced symbols, etc., and it is not meant to be displayed as a diagnostic. A tagged hint is not indicative of something wrong with the code that must be "fixed". Rather, it's a subtle visual hint to the user meant to provide additional information about the code. Tagged hints are generated only if the client indicates that it supports the tag. If the client does not support the tag, pyright will refrain from supplying the tagged hint. (Pyright also makes use of a second tag if the client supports it. The Deprecated tag indicates that the client should display the text with a strikethrough or similar treatment.)
  • If the client claims to support a tagged hint, pyright assumes that the client will use it appropriately and not display it as a normal diagnostic. Refer to VS Code for an example of how these tagged hints should be treated.
  • There is currently no way to disable these tagged hints in pyright. For clients that display these tagged hints in the manner suggested by the LSP spec, there is no problem. For clients that display these tagged hints as normal diagnostics, it becomes a problem because users perceive these are "errors" that need to be "fixed" or suppressed.

Renaming parameters to work around the issue

  • Some users have suggested that parameter renaming might be a good workaround for this issue. Contrary to most languages, parameter names are significant in Python because they can be referenced by keyword arguments at the call site. That means renaming parameters is not a recommended solution. It will encourage behaviors that could introduce subtle bugs in the code.

If you are using pyright with a client that claims to support the Unnecessary tag but then displays these tagged hints as regular diagnostics, here are some options:

  • Switch to a different client.
  • Try to convince the maintainer of your client or LSP provider to change the way that hints with the Unnecessary tag are displayed.
  • Find a way to configure your client not to display tagged hints as regular diagnostics.
  • Configure your client's LSP functionality to tell pyright that Unnecessary tags are not supported by your client.

I recognize that none of these are great options, which explains why many users continue to be frustrated and why this thread continues to attract attention.

At this point, I'm considering adding a configuration option to pyright that would tell pyright to refrain from generating any tagged hints even if the client claims to support them. I've been very reluctant to add a configuration option designed entirely to work around (what I perceive to be) a bug (or at least a design flaw) in some clients, but I think we've reached an impasse with the maintainers of these clients, and it's unlikely that they're going to be convinced that they should change the way tagged hints are treated and presented. Adding an option to pyright that allows you to completely disable these tagged hints might be the best option left to us.

@lewis6991
Copy link

lewis6991 commented Dec 17, 2023

Refer to VS Code for an example of how these tagged hints should be treated.

No. The LSP maintainers specifically communicated that clients can display these tags however they want (see microsoft/language-server-protocol#1696) and that vscode is explicitly not a reference implementation.

Pyright is only designed to work well with vscode, and other clients are not considered. And further to that it was communicated in this thread that Pyright is no longer designed to be an LSP and is instead now just a backend to Pylance, a vscode-only LSP server.

The only solutions here are either:

  • for Pyright to allow for these tagged diagnostics to be disabled per line, like any other server.
  • for Pyright to switch these tagged diagnostics to semantic tokens. However this won't happen because it is no longer maintained as an LSP server.

@joaotavora
Copy link

Try to convince the maintainer of your client or LSP provider to change the way that hints with the Unnecessary tag are displayed.

I happen to be the maintainer of the LSP client in GNU Emacs, which supports hundreds of servers, and I'm not convinced.

@jason0x43
Copy link

jason0x43 commented Dec 17, 2023

Pyright makes use of tagged hints (using the Unnecessary tag) in various places to indicate to the client that it should display the text in a dimmed-out manner. This is used for unreachable code, unreferenced symbols, etc., and it is not meant to be displayed as a diagnostic.

This, I think, is a major disconnect between the maintainers of pyright and many of the contributors to this thread regarding unused function parameters. The problem isn't with how Unnecessary tags are being displayed. The problem is that in some cases, particularly with unused function parameters, unused function parameters are not "unnecessary" (in that code will not execute properly if they're left out), and tagging them as Unnecessary is misleading.

The idea behind any sort of visual hint is to convey potentially useful information. If lots of necessary function arguments are being highlighted as unnecessary, it dilutes the value of tagging things as unnecessary in the first place.

Adding an option to pyright that allows you to completely disable these tagged hints might be the best option left to us.

Adding an option to disable these hints just for unused function parameters seems like it would be sufficient, as that seems to be the main case where Unnecessary tags don't convey any helpful information.

@SichangHe
Copy link

My example workaround, verbose but works:

def stream_callback(
    in_data: bytes | None,
    n_frame: int,
    time_info: Mapping[str, float],  # pyright: ignore reportUnusedVariable
    status: int,  # pyright: ignore reportUnusedVariable
):
    """Send input audio data to `audio_queue`."""
    assert in_data is not None
    audio_queue.put((in_data, n_frame))
    return None, paContinue

@erictraut
Copy link
Collaborator

Pyright 1.1.347 includes a new language server setting called "pyright.disableTaggedHints". If you set this to true, pyright will not emit any "hint" diagnostics with tags.

If you disable these hints but are still interested in seeing diagnostics for unreferenced symbols, you can enable reportUnusedImport, reportUnusedClass, reportUnusedFunction or reportUnusedVariable. And if you'd like to see diagnostics for deprecated classes or functions, you can enable reportDeprecated.

@joaotavora
Copy link

joaotavora commented Jan 16, 2024

Thanks, but can someone explain what this means for LSP clients other than VSCode? Can pyright be made to start behaving like other servers in this regard? I.e. is there now some configuration we can give it so that certain unused entities are flagged unless the user explicitly types something in the source code to acknowledge that situation as intentional?

IOW is there now a way to do the workaround by @SichangHe here, but in a less awkward/verbose way?

@Cutipus
Copy link

Cutipus commented Jan 25, 2024

My example workaround, verbose but works:

...
    time_info: Mapping[str, float],  # pyright: ignore reportUnusedVariable
    status: int,  # pyright: ignore reportUnusedVariable
...

This workaround doesn't really work, as discussed in this thread. It does not turn off the hinting for that specific line. If

With the addition of pyright.disableTaggedHints option in Pyright 1.1.347 it is now possible to only ignore reportUnusedVariable while letting other hints show for the whole file. You can, if I'm reading this correctly, use pyright.disableTaggedHints and whitelist the rest - reportUnusedImport, reportUnusedClass, reportUnusedFunction, reportDeprecated - excluding reportUnusedVariable.

This still wont allow you to explicitly mark a specific line or variable to ignore reportUnusedVariable though, so this isn't actually what the issue is about. If @SichangHe's workaround works for anyone please let me know, it would be perfect in my opinion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
addressed in next version Issue is fixed and will appear in next published version enhancement request New feature or request
Projects
None yet
Development

No branches or pull requests