Aggressive import "fixing" creates syntactically invalid code

[luzader]

Environment data

• Language Server version: 2022.8.20 (pyright 888eb243)
• OS and version: ubuntu 20.04.4
• Python version (& distribution if applicable, e.g. Anaconda): python3.7

Code Snippet

try:
    import smbus2
except ImportError:
    # Allow smbus2 to be missing in order
    # to generate documentation
    smbus2 = None

Repro Steps

1. Ensure 'smbus' does not exist, it is not very useful on desktop PCs and you develop embedded python, but this file needs to be importable for generating documentation with Sphinx
2. Save the file

Expected behavior

1. pylance should not create a syntax error where one did not exist previously
2. pylance should not alter import statements which are inside a control structure (it doesn't "know" why the structure is there, so it should be preserved by default)
3. pylance should not alter code in new and surprising ways (Principle of Least Astonishment)

Actual behavior

1. pylance saves the following syntactically invalid string of text:

try:
    except ImportError:
    # Allow smbus2 to be missing in order
    # to generate documentation
    smbus2 = None

[bschnurr]

for 3 I'm assuming you have codeActionsOnSave ?

"editor.codeActionsOnSave": [
    "source.fixAll"
]

[luzader]

Yes which used to be safe. But the definition of 'all' seems to have changed. So I'm turning that off.

[heejaechang]

We recently added 2 new fix all code actions in the extension and unfortunately,

"editor.codeActionsOnSave": [
    "source.fixAll": true
]

will automatically pick up any fix all existing.

while we taking a look at how to improve our behavior, you can use this to turn it off

"source.fixAll.unusedImports": false

for other fix all, you can take a look at this

[heejaechang]

There is also this issue - microsoft/vscode#82718 - in vscode that is tracking editor.codeActionsOnSave intellisense issue.

Basically, editor.codeActionsOnSave currently only shows 2 predefined entries; source.fixAll and source.organizeImports. problem is source.fixAll is a catch all option. but it doesn't show all individual fixAlls exposed by all extensions. so a lot of people end up just put "source.fixAll" and hope when extensions are updated, new fix all code action added won't cause a problem.

the issue above will let each extensions expose fixAlls they provide so users can enable only ones they want rather than enable all blindly and hope for best.

For now, people need to reference documentations of all extensions they use to find out specific names of each fix all code actions and use them to configure or use catch all - "source.fixAll"

[graham8]

IMHO the real problem here is the syntax try; import foo; except ImportError should not be flagged for fixing if foo doesn't exist. It's perfectly valid code and very useful, in particular when remote debugging via ssh when code on the local machine can't see the modules installed on the target.

[bschnurr]

the feature of removing unused imports is going to remove unused imports.

Instead of the On/Off coarse behaviour of source.fixAll.unusedImports maybe we could add a way to tag certain imports as expected to be unused.

[graham8]

add a way to tag

The tag is already there, if it's possible to have pylance recognise the import (and subsequent calls to it) is in a try clause, ie have a look around for the context.

[heejaechang]

"remove all unused imports" will only remove top level imports now. we still allow users invoking "remove unused imports" for nested imports but now we will remove leading whitespaces correctly.

thank you for reporting the issue.

[debonte]

This issue has been fixed in prerelease version 2022.9.11, which we've just released. You can find the changelog here: CHANGELOG.md