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

Special case known good names for use-before-def #429

Conversation

jakebailey
Copy link
Member

Fixes #391.

Sort of a hack, but these hardcoded names are pulled from the list of atoms in the Python grammar. I've still been unable to reproduce True/False being use-before-def (other than once without a debugger attached), but it shouldn't occur given that I now explicitly ignore them.

Unfortunately, this may mask a bug in ProjectState.BuiltinModule.GetMember, since I don't think there's a good reason why None should fail to be found...

@MikhailArkhipov
Copy link

This can happen when builtins module is not loaded/scraped, which would be a big problem in many other cases. Members are assigned in the AstBuiltinsPythonModule

            if (boolType != null) {
                _members["True"] = _members["False"] = new AstPythonConstant(boolType);
            }

This can be a threading issue such as code analyzed before builtins are loaded

@jakebailey
Copy link
Member Author

Well, if you think there's a better way to approach this, I'd be curious to see what you think about the test case for None presented in the most recent comment in #391.

My previous fix to ignore ConstantExpression doesn't really work because the node being inspected is some sort of index expression. I think that fix was actually also masking this same problem, because None should have shown up in the builtins.

@MikhailArkhipov
Copy link

Might be falling to fallback in

            var moduleRef = await Modules.TryImportAsync(_builtinName, token).ConfigureAwait(false);
            if (moduleRef != null) {
                _builtinModule = (BuiltinModule)moduleRef.Module;
            } else {
                _builtinModule = new BuiltinModule(fallback, this);
                Modules.SetModule(_builtinName, BuiltinModule);
            }

@MikhailArkhipov
Copy link

MikhailArkhipov commented Nov 21, 2018

On the None - try adding it to members as we do with True and False

                    if (typeId == BuiltinTypeId.NoneType) {
                        noneType = m as IPythonType;
                    }
...
_members["None"] = new AstPythonConstant(noneType);

@jakebailey
Copy link
Member Author

Doing that does actually fix that Generator test (and is way less hacky), but I'm not exactly sure where how to approach finding how the analysis for builtins doesn't happen before the rest of the diagnostics happens.

@jakebailey
Copy link
Member Author

Closing in favor of #447 for the None part. The True/False issue remains unsolved and will likely need to be approached in another way.

@jakebailey jakebailey closed this Dec 3, 2018
jakebailey added a commit that referenced this pull request Dec 3, 2018
Fixes #442.
Replaces #429, which only masks the underlying True/False bug in #391.

I also removed the hack in #336 which checked for `ConstantExpression`.
@jakebailey jakebailey deleted the maybe-fix-use-before-def-constants branch February 14, 2019 20:22
jakebailey added a commit to jakebailey/python-language-server that referenced this pull request Nov 1, 2019
Fixes microsoft#442.
Replaces microsoft#429, which only masks the underlying True/False bug in microsoft#391.

I also removed the hack in microsoft#336 which checked for `ConstantExpression`.
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.

2 participants