astroid icon indicating copy to clipboard operation
astroid copied to clipboard

astroid brain for turtle module

Open rioj7 opened this issue 3 years ago • 8 comments

PyLint generates a lot of errors in a program using the turtle module: PyCQA/pylint#1928 and Microsoft/vscode-python#1056

The following brain fixes this. It uses the same method as the turtle module to generate stubs for astroid to parse.

brain_turtle.py

import astroid

def register(linter):
  pass

def transform():
  import turtle
  def _make_global_funcs(functions, cls):
    funcs = []
    for methodname in functions:
      method = getattr(cls, methodname)
      paramslist, argslist = turtle.getmethparlist(method)
      if paramslist == "": continue
      funcs.append(f"def {methodname}{paramslist}: return")
    return funcs
  funcs = []
  funcs.extend(_make_global_funcs(turtle._tg_screen_functions, turtle._Screen))
  funcs.extend(_make_global_funcs(turtle._tg_turtle_functions, turtle.Turtle))
  return astroid.parse('\n'.join(funcs))

astroid.register_module_extender(astroid.MANAGER, "turtle", transform)

rioj7 avatar Nov 01 '20 10:11 rioj7

@rioj7 thanks a lot for this. Would you mind making a PR? If so don't forget to add you as contributor and also unit test would be valuable. I'll be please to review it. By the way, i saw that you import the turtle module but it is not something that is recommended because of security issues. You can have a look to other brain to see how we can handle this without importing the module. If you encounter some difficulties do not hesitate to ask questions.

hippo91 avatar Nov 17 '20 18:11 hippo91

@hippo91 Looking at the other brains the code is written out literal. It would mean I need to copy over the class Screen and Turtle to get the parameters correct, now they are introspected from the turtle module. Also if they add or remove methods they are automatically updated.

What is the security risk of importing turtle?

rioj7 avatar Nov 18 '20 16:11 rioj7

@rioj7 pylint is a static tool that means that astroid is also a static inference engine. It means that no dynamic execution is required nor authorized. This way importing module is not an option as it means executing code.

Writing a brain for astroid doesn't mean copying an entire class but just the signature of function/member that can not be inferred.

hippo91 avatar Nov 19 '20 06:11 hippo91

@hippo91 I have the brain_turtle.py using a literal string with the function signatures.

But what should the unittest test?

I have looked at a few unittest files and it looks like they perform tests on the "inferred" (??) value of an extracted node.

rioj7 avatar Nov 19 '20 14:11 rioj7

@rioj7 you should add a new unittest module in the astroid/tests package. This new module should be named something along unittest_brain_turtle.py. The usual way of testing your brain consist in writing a piece of python code in a string such as:

node="""
import turtle
turtle.NiceTurtle(4, "hercule")
"""

where i supposed your brain is "mocking" a class named NiceTurtle of the turtle module. Then you ask astroid to infer the last node of the string (i.e the instance of the class) with inferred_val = astroid.builder.extract_node(node). Inferring just means detect to what kind of python object does the node correspond to. For example, here the node should be an Instance of a class named NiceTurtle. Thus you can test that it is an instance with:

self.assertIsInstance(inferred_val, astroid.Instance)

hippo91 avatar Nov 26 '20 13:11 hippo91

@hippo91 I have created a pull request but travis reports a problem and I have no clue what the cause is.

It took a moment to get the tests running in a clone of the astroid repository.

I had to create a virtual environment and install the following packages

lazy-object-proxy
pytest
six
wrapt

Why do we need pytest when all tests are written for unittest?

rioj7 avatar Dec 31 '20 21:12 rioj7

@rioj7 sorry for the delay. pytest is the tool astroid and pylintuse to launch unit tests. pytest is fully compatible with unittests and can launch tests written for this last one.

hippo91 avatar Jan 23 '21 15:01 hippo91

@hippo91 Have you looked at the Pull Request and the error that Travis reports, it is not merge block.

rioj7 avatar Jan 25 '21 09:01 rioj7