New check: import-outside-toplevel (close #3067)

[nickdrozd]

Steps

• [x ] Add yourself to CONTRIBUTORS if you are a new contributor.
• [ x] Add a ChangeLog entry describing what your PR does.
• [ x] If it's a new feature or an important bug fix, add a What's New entry in doc/whatsnew/<current release.rst>.
• [ x] Write a good description on what the PR does.

Description

This check warns when modules are imported from places other than a
module toplevel, e.g. inside a function or a class.

Type of Changes

	Type
✓	✨ New feature

[coveralls]

Coverage increased (+0.003%) to 90.031% when pulling c9b2d21 on nickdrozd:import-outside-toplevel into 6b3afd4 on PyCQA:master.

[coveralls]

Coverage increased (+0.02%) to 90.219% when pulling 1ed5801 on nickdrozd:import-outside-toplevel into a7f2365 on PyCQA:master.

[AWhetter]

Looks good to me!

[nickdrozd]

There's a false negative for this check:

def k():
    for _ in range(5):
        import sys  # should trigger, but doesn't

Is there a good way to detect this situation?

[AWhetter]

Having looked at this again, please could we fix the false negative before merging?

[AWhetter]

If you use node.scope() here, it will traverse up the syntax tree until it finds the node that defines the scope (https://astroid.readthedocs.io/en/latest/api/base_nodes.html#astroid.node_classes.NodeNG.scope). Anything other than a Module would mean that the message should be raised. This should even account for cases like the following where we wouldn't want to raise a message:

if some_condition:
    import this
else:
    import that

and cases like the following where we would want to raise the message:

def func():
    if some_condition:
        import this
    ...

[nickdrozd]

I knew there was something like that, but I couldn't remember the name. This should be fixed now.

[PCManticore]

Overall this looks solid, thanks @nickdrozd !

One minor suggestion before getting this in. Sometimes some imports need to be scoped to a function scope in case they are related to a circular import between two components. In that case, we'd need a csv option for allowing certain imports to be exempted from this check when encountered. This would avoid a proliferation of # disable: import-outside-toplevel for such cases.

[nickdrozd]

************* Module pylint.reporters.text
pylint/lib/python3.6/site-packages/pylint/reporters/text.py:206:16: C0415: Import outside toplevel (colorama) (import-outside-toplevel)

It seems to work!

[nickdrozd]

@PCManticore You mean like a whitelist of modules that can be imported outside the toplevel? I have no idea how to implement that. Are there any similar examples?

[PCManticore]

@nickdrozd Yes, something along those lines.

This would involve:

• adding a new option in the checker's options value. Here's an example from the logging checker: https://github.com/PyCQA/pylint/blob/eeca6cf0baa7263ff46598f06e4eda0f3072262b/pylint/checkers/logging.py#L116, which accepts a comma separated value of module names
• the option is then accessible under self.config.option_name, but with the dashes replaced by underscore. Here's another example in the logging checker: https://github.com/PyCQA/pylint/blob/eeca6cf0baa7263ff46598f06e4eda0f3072262b/pylint/checkers/logging.py#L145
• in check_toplevel function we need to exempt the current module if its name is present in that option list.

[nickdrozd]

@AWhetter @PCManticore Okay, I think that should do it. I added some unit tests as well.

[PCManticore]

Thank you @nickdrozd !