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

Set isThirdPartyImport and isThirdPartyPyTypedPresent for interim files #6155

Merged
merged 17 commits into from
Oct 23, 2023

Conversation

debonte
Copy link
Collaborator

@debonte debonte commented Oct 13, 2023

Addresses microsoft/pylance-release#4059

Things I asked for comments on when initially posting this PR:

  1. Is it correct to set isThirdPartyImport to true on SourceFile and SourceFileInfo only when moduleNameAndType.importType === ImportType.ThirdParty? We could have done that before, so I'm not sure if I'm missing some complexity here.
  2. When calling _shouldWalkUp if current is outside of the root directory, the return value of _shouldWalkUp is based on comparing the lengths of the two paths which seems wrong since they are unrelated. For example, if current is %LOCALAPPDATA%\Programs\Python\Python311\Lib" and root is the workspace root, their relative lengths are irrelevant. Should pathUtils.containsPath be getting called somewhere? Or am I using _shouldWalkUp incorrectly?

@github-actions

This comment has been minimized.

Copy link
Collaborator

@erictraut erictraut left a comment

Choose a reason for hiding this comment

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

This change looks reasonable to me. It's touching piece of code that deals with a lot of complexity (given all of the weird edge cases of the Python import system), so I can't say with full confidence that it won't introduce regressions in some edge case. Your changes don't appear to have any impact on the core import resolution logic (i.e. the logic used to model the behavior of import statements), so if there is a regression, it will presumably show up only in completion suggestions and the pytest fixture functionality.

@github-actions

This comment has been minimized.

@heejaechang
Copy link
Collaborator

heejaechang commented Oct 14, 2023

Is it correct to set isThirdPartyImport to true on SourceFile and SourceFileInfo only when moduleNameAndType.importType === ImportType.ThirdParty? We could have done that before, so I'm not sure if I'm missing some complexity here.

when I looked it last time, this one was when we set it as third party even if importType is not ThirdParty

https://github.com/microsoft/pyright/blob/main/packages/pyright-internal/src/analyzer/program.ts#L1320

so, when ThirdParty is set, isThridPartyImport is always true, but when Local is imported by ThirdPartyImport, then it is considered ThirdPartyImport even if it is Local.

@debonte
Copy link
Collaborator Author

debonte commented Oct 16, 2023

when I looked it last time, this one was when we set it as third party even if importType is not ThirdParty

https://github.com/microsoft/pyright/blob/main/packages/pyright-internal/src/analyzer/program.ts#L1320

so, when ThirdParty is set, isThridPartyImport is always true, but when Local is imported by ThirdPartyImport, then it is considered ThirdPartyImport even if it is Local.

Yeah, I saw that code, but since _createInterimFile is creating a file rather than analyzing an import, the "local import from 3rd party file" logic didn't seem relevant to me in this scenario.

@heejaechang
Copy link
Collaborator

heejaechang commented Oct 16, 2023

Yeah, I saw that code, but since _createInterimFile is creating a file rather than analyzing an import, the "local import from 3rd party file" logic didn't seem relevant to me in this scenario.

so, currently, if you have a file/module in typings or extraPath (ex, typings/pandas/core/accessor.py), and pandas package has absolute path from pandas.core import accessor rather than relative path somewhere, it could load one in typings rather than one in pandas package.

pyright supports this kind of shadowing so that user can provide per file stub in typing folder as long as I know.

if those files are marked as user files (match include/exclude in configOption), then we create sourceFileInfo when program is created. but if they are not and if they are brought into program by other sourceFile then they are marked as third party

now question is if that file is brought into the program by either interimFile or setFileOpen, how should we mark it. isThridParty? or not.

might be fine to just not mark it. I am just saying difference on if that file is loaded in program differently, it might have marked as isThirdParty

@github-actions

This comment has been minimized.

1 similar comment
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions
Copy link
Contributor

According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅

@debonte debonte merged commit 5d3d703 into microsoft:main Oct 23, 2023
12 checks passed
@debonte debonte deleted the pylance4059 branch October 23, 2023 19:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants