Skip to content
This repository was archived by the owner on Apr 14, 2022. It is now read-only.

Give a diagnostic when creating a function in a class and not specifying the first argument as self #1279

Closed
wants to merge 28 commits into from

Conversation

CTrando
Copy link
Contributor

@CTrando CTrando commented Jul 3, 2019

Waiting on #1273 before I can merge this one.

e.g

class Test:
    def hello(x, y, z):
         pass

@CTrando CTrando changed the title [WIP]: Give a diagnostic when creating a function in a class and not specifying the first argument as self Give a diagnostic when creating a function in a class and not specifying the first argument as self Jul 16, 2019
@jakebailey
Copy link
Member

I don't think we should merge this one, at least not for now. Although calling the first argument "self" is convention, it's not a runtime-requirement and code which doesn't say self will still work if the first variable is used correctly.

(Arguably, we should be trying to rely on the name self less and less, but that's another issue.)

@MikhailArkhipov
Copy link

It is an error per http://pylint-messages.wikidot.com/messages:e0213. As in big VS, one can always suppress it via settings/pragmas. By default though, VS shows violations or common conventions although said violations may not always translate to crashes.

https://legacy.python.org/dev/peps/pep-0008/#function-and-method-arguments
Always use self for the first argument to instance methods.
Always use cls for the first argument to class methods.

@CTrando CTrando changed the title Give a diagnostic when creating a function in a class and not specifying the first argument as self [WIP]: Give a diagnostic when creating a function in a class and not specifying the first argument as self Jul 23, 2019
@CTrando
Copy link
Contributor Author

CTrando commented Jul 23, 2019

WIP until I can add no class argument and no method argument.

@CTrando CTrando changed the title [WIP]: Give a diagnostic when creating a function in a class and not specifying the first argument as self Give a diagnostic when creating a function in a class and not specifying the first argument as self Jul 23, 2019
@brettcannon
Copy link
Member

Not using self as the name of the first parameter of an instance method is not an error and potentially done on purpose (e.g. if you are using **kwargs and you don't want self to be captured as a positional or keyword argument). It also won't work if you use staticmethod (where it doesn't make sense), or classmethod (which is traditionally cls). This also doesn't cover other clever decorators, metaclasses, etc.

If this being done as a stylistic thing then I would leave to a style linter like Pylint. If the worry is someone mis-typed self then the preexisting check for undefined variables should catch that. And if it's due to someone simply forgetting to include self then tests should capture that, but on its own I would say that since this isn't an obvious run-time error that this is going to be difficult to get right like the redefinition check.

@MikhailArkhipov
Copy link

MikhailArkhipov commented Jul 24, 2019

Per links above it is considered an error in pylint which then refers to https://legacy.python.org/dev/peps/pep-0008/#function-and-method-arguments. Tests may be useful, but in C# I's rather see that I mistyped this to ths before I even get to compilation. VS editor does not require even to build to see issues.

Besides, some people never write tests and that is their choice. We can't really insist on 'write tests and that will catch the error'.

The question is a) how to classify the message. By default it is a warning, not an error. And b) how to let people to suppress categories easily. Like in VS I have 600 info messages from C# and Resharper which I suppress with a single click.

image

/// <returns></returns>
private bool SelfIsMetaclass() {
// Just allow all specialized types in Mro to avoid false positives
return _self.Mro.Any(b => b.IsSpecialized);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to check for Type in the Mro, don't know an easy way besides this

public static bool HasValidDecorators(this IPythonFunctionType f, IExpressionEvaluator eval) {
bool valid = true;
// If function is abstract, allow all decorators because it will be overridden
if (f.IsAbstract) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I looked at this for the abstract check

return valid;
}

private static void ReportInvalidDecorator(NameExpression name, string errorMsg, IExpressionEvaluator eval) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very similar to the one in PythonFunctionExtensions, any idea how to remove the duplication?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of passing type, pass actual flags. I.e. HasValidDecorators(bool isAbstract, bool isClassMethod, ...)

@CTrando
Copy link
Contributor Author

CTrando commented Aug 12, 2019

Diagnostics on function params:

Gives diagnostics on:

  • Not usingself in a regular function
class A: 
  def tmp(x, y, z): ...
  • Not using cls in a class method or an abstract class method
class A: 
  @classmethod / @abstractclassmethod
  def tmp(x, y, z): ...
  • Not using cls in an automatic class method, I have __new__, __init_subclass__, __class_getitem__.
class A: 
  def __new__(x, y, z): ...

No diagnostics on:

  • Methods declared with @staticmethod
  • Methods declared in a class that is in a metaclass chain, (has type in its Mro)

What I don't handle:
Decorators with somewhat unorthodox behavior such as singledispatch.

I could add something that checks if there are any unknown decorators on the function, don't give any diagnostics on the parameters.

Diagnostics on decorator combinations:

Properties:

  • Static method + property is not allowed
class A:
  @property  
  @staticmethod 
  def tmp(self): ...
  • Class method + property is not allowed
class A:
  @property  
  @classmethod 
  def tmp(self): ...

Functions:

  • Static method + Class method is not allowed
class A:
  @staticmethod  
  @classmethod 
  def tmp(self): ...

Note: Important to note that if there are any errors found in evaluating decorator combinations, no diagnostics are given on the parameters of the given property or function.

Copy link

@MikhailArkhipov MikhailArkhipov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Check #1401, it has a null ref around similar __init__ check.

@@ -37,8 +37,5 @@ public static bool IsGeneric(this IPythonType value)
dst.Location = src.Location;
}
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing because was unused and does not check for declaration type and null check for name.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants