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

Don't show "not accessed" / graying out for "required" method parameters #194

Closed
Cellorator opened this issue Jul 31, 2020 · 27 comments
Closed
Labels
enhancement New feature or request fixed in next version (main) A fix has been implemented and will appear in an upcoming version

Comments

@Cellorator
Copy link

Why does Pylance grey-out the "self" parameter when i don't use it in the method and shows this when I hover over it:

image

@erictraut
Copy link
Contributor

Variables that are not referenced are colored in gray as a subtle hint that they haven't been accessed. This can sometimes indicate a bug. If you intended for this symbol to be unaccessed, you can ignore the subtle clue. This behavior is consistent with other language servers.

If you find the grayed-out text annoying, you can adjust your color theme settings to display the same color as normal text (in your case, white).

@jakebailey
Copy link
Member

This would be a behavior change, but I think it may be an improvement to not gray out "required" parameters (e.g. self, cls, things needed to properly overload / implement interfaces), as they aren't helpful in those cases.

@savannahostrowski savannahostrowski added enhancement New feature or request needs spec and removed triage labels Jul 31, 2020
@janosh
Copy link

janosh commented Aug 7, 2020

... I think it may be an improvement to not gray out "required" parameters...

@jakebailey I agree. Came here to suggest exactly that.

@mdavezac
Copy link

mdavezac commented Aug 7, 2020

There is also the case where self is accessed indirectly via super. Those are still diagnosed as not accessed.

@jakebailey jakebailey changed the title What does "self" is not accessed mean? Don't show "not accessed" / graying out for "required" method parameters Aug 21, 2020
@yohan-pg
Copy link

It would be nice if we could mark variables, say by naming them with a leading underscore, to indicate that having no accesses to them is OK.

@erictraut
Copy link
Contributor

Pylance never emits an error for an unreferenced parameter.

By default, it also doesn't emit an error for an unreferenced variable, although you can enable the reportUnusedVariable diagnostic rule if you'd like. If this rule is enabled, it will not emit an error if you name the variable _ (single underscore). That's the convention within the Python community to indicate that a variable is intentionally unused. A longer variable name that begins with a single underscore is used to indicate a "private" symbol — one that is not intended to be used outside of the class or module in which it appears.

@yohan-pg
Copy link

yohan-pg commented Nov 12, 2020

Thanks for your answer. You are absolutely right that _ prefixes signify private access in python; I mostly used this as an example since it has precedent in other languages. The only problem with replacing the entire names with an underscore that pylance flags duplicate parameters even if they are underscores (Duplicate parameter "_"). Perhaps this could be changed?
EDIT: it just occurred to me that the duplicate parameter error also occurs at runtime, so of course that's not really an option.

I understand that graying out variables is mostly meant as a subtle cue rather than a real warning. However, unused variables are so often a symptom of real errors that gray variables name really become nagging.

@jaapz
Copy link

jaapz commented Jan 19, 2021

Prefixing with _ is more often used as a way to "mark" a variable as private, not as a way to indicate it explicitly not being used.

Apart from that, prefixing self and cls with _ would be counter to what the python community does anyway so I don't think that rule applies here. self is always there in methods, and very often it is not accessed. Emitting a message like this is not helpful, and is even a bit annoying. Whether it's an "error" or a "helpful message" doesn't really change that.

In itself, this rule (x is not accessed) is very helpful in all other cases so I'd rather not disable it entirely.

EDIT: i read that wrong, you meant replacing the entire variable name with _, but the point still stands: that's not convention so it would be very strange for pyright/pylance to replace the self and cls with _, and that also doesn't agree with pep8

@erictraut
Copy link
Contributor

erictraut commented Jan 19, 2021

Pylance does not emit a diagnostic message for this condition. It simply grays out the text which is a subtle visual queue that the variable is not accessed. Are you seeing an actual diagnostic message (like an error or warning) somewhere?

@pzelnip
Copy link

pzelnip commented Feb 10, 2021

This would be a behavior change, but I think it may be an improvement to not gray out "required" parameters (e.g. self, cls, things needed to properly overload / implement interfaces), as they aren't helpful in those cases.

I agree with this, whereas the OT was removing the greying out altogether. I find the greying out useful in the general case, but with "required"/conventional arguments I find it an annoying distraction. That is, in this snippet:

class Foo:
    @classmethod
    def bar(cls, somearg):
         return 42

I'd expect somearg to be greyed out, but cls to not be greyed out.

@jaapz
Copy link

jaapz commented Feb 11, 2021

Pylance does not emit a diagnostic message for this condition. It simply grays out the text which is a subtle visual queue that the variable is not accessed. Are you seeing an actual diagnostic message (like an error or warning) somewhere?

In my case (using coc.nvim) it's a bit more distracting as the color scheme doesn't just gray things out but also puts a marker on every line and underlines it. You could argue that's my problem and nvim should highlight things like this more subtly, but that's not the point.

Like @pzelnip says, it's highlighting stuff as "possibly shouldn't be here", whereas it absolutely should be there to conform with how Python does (class)methods.

@janosh
Copy link

janosh commented Feb 11, 2021

This would be a behavior change, but I think it may be an improvement to not gray out "required" parameters (e.g. self, cls, things needed to properly overload / implement interfaces), as they aren't helpful in those cases.

@jakebailey Just in case you weren't thinking of ABC when you wrote this (although I'm guessing you were), Pylance should also not gray out any variables in signatures of abstract methods:

abc

@ProgrammingLife
Copy link

Is there any solution on this?

@janosh
Copy link

janosh commented Mar 25, 2021

@ProgrammingLife Not yet. Feel free to upvote the original issue to let the Pylance team know it's important to you.

@jakebailey
Copy link
Member

jakebailey commented Apr 20, 2021

The next release changes the behavior such that the following cases are no longer marked as "not accessed":

  • self and cls in methods.
  • Parameters for methods explicitly marked as abstract.
  • Parameters for methods defined in Protocol classes.
  • Parameters in @overload decorated functions.

I think this should cover all of the cases where either it's "required" (makes Python mad if you omit it) or is a part of defining a contract (where the parameters are a part of that definition).

Thanks for the feedback.

@jakebailey jakebailey added fixed in next version (main) A fix has been implemented and will appear in an upcoming version and removed needs spec labels Apr 20, 2021
@jakebailey
Copy link
Member

This issue has been fixed in version 2021.4.2, which we've just released. You can find the changelog here: https://github.com/microsoft/pylance-release/blob/main/CHANGELOG.md#202142-21-april-2021

@jaapz
Copy link

jaapz commented Apr 22, 2021

Thanks for fixing this!

@disrupted
Copy link

Nice to see this being adressed in the latest release, thanks!

@LeonardoRick
Copy link

I'm having similar issues with a lot of my annotations (@). Is there a issue already opened to that?

@jakebailey
Copy link
Member

If the behavior differs from what I defined above, then a new issue would be appreciated.

@MSK61
Copy link

MSK61 commented Dec 14, 2022

I might be missing on something here, but what about an overriding method in a derived class(that overrides a method in a base class) where not all the parameters are used? Also if I'm passing a lambda/function as an argument to another function where the passed lambda/function is going to be invoked with a certain signature(number and types of arguments) but the lambda/function passed as an argument doesn't make use of all parameters?

@erictraut
Copy link
Contributor

If a parameter is not used in the lambda/function, it will be displayed in gray. This isn't an error condition. It's a subtle visual cue that the symbol is not accessed. Sometimes a symbol that is not accessed can be indicative of a bug, but in other cases it's a legitimate condition. This visual cue gives you the ability to detect cases where you expect a symbol to be accessed but it is not.

@MSK61
Copy link

MSK61 commented Dec 14, 2022

And after I check and determine that not referencing the parameter isn't an inadvertent bug the cue will remain, so someone else viewing the code(or even myself after a while) will see the cue and check again. There should be a way to turn off the cue so that it doesn't always prompt the reader to check the parameter usage.

@Lamby777
Copy link

Lamby777 commented Aug 9, 2023

The next release changes the behavior such that the following cases are no longer marked as "not accessed":

  • self and cls in methods.
  • Parameters for methods explicitly marked as abstract.
  • Parameters for methods defined in Protocol classes.
  • Parameters in @overload decorated functions.

I think this should cover all of the cases where either it's "required" (makes Python mad if you omit it) or is a part of defining a contract (where the parameters are a part of that definition).

Thanks for the feedback.

What about dunder methods?
image

@erictraut
Copy link
Contributor

What about dunder methods?

The parameters in your example are not referenced in the method, so they are displayed accordingly. There are plenty of examples of where it's fine for a parameter to go unreferenced. That's true for dunder methods as well as non-dunder methods. Pylance therefore never emits an error for unused parameters. It simply causes them to be displayed as "grayed out" as a subtle reminder that they are unreferenced.

@Lamby777
Copy link

Lamby777 commented Aug 9, 2023

What about dunder methods?

The parameters in your example are not referenced in the method, so they are displayed accordingly. There are plenty of examples of where it's fine for a parameter to go unreferenced. That's true for dunder methods as well as non-dunder methods. Pylance therefore never emits an error for unused parameters. It simply causes them to be displayed as "grayed out" as a subtle reminder that they are unreferenced.

Makes sense, but I have the same issue as jaapz (won't @ them since it was 2 years ago) where the LSP puts a warning marker at the end of the line, and the diagnostics list has unnecessary stuff you just gotta skip past and ignore. Is there a way to turn off the diagnostic in configs, specifically for dunder methods?

@erictraut
Copy link
Contributor

@Lamby777, are you using VS Code? I'm guessing that you're using some other editor that is misinterpreting these "unused code" hints as diagnostics rather than indications that the text should be displayed as "grayed out". If that's the case, you should raise the issue with the maintainer of your editor. Or switch to VS Code, which doesn't have this problem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request fixed in next version (main) A fix has been implemented and will appear in an upcoming version
Projects
None yet
Development

No branches or pull requests