Add builtin scope

[lpetre]

Summary

Builtins were not being properly shadowed when redefined. There were two issues:

• BuiltinAssignments were recorded in __getitem__, making iteration unstable
• Two BaseAssignments would be recorded for an overridden builtin

To fix this, I've introduced a scope about GlobalScope called the BuiltinScope and I've moved the BuiltinAssignment logic there.

My justification for introducing the new scope: it feels like it fits the design well. Declarations in the GlobalScope will override declarations in the outer (ie builtin) scope.

Test Plan

python -m unittest libcst.metadata.tests.test_scope_provider.ScopeProviderTest

[zsol]

Looks good in general. I'd like @jimmylai to weigh in too, but before merging we'd need to update the docs to reflect this new scope

[jimmylai]

Overall looks good. The implementation is simple.
The only concern I have is the overhead of creating a BuiltinScope.
There are 100+ names in builtins and the same number of BuiltinAssignment will be created.
@lpetre, can you run it on our large codebase to see it it's significantly slower?

[lpetre]

FWIW I've made the BuiltinAssignments be lazily created in __getitem__ instead of declaring them all in the BuiltinScope constructor.

It would be nice if we could change this in the future, as I'm worried about code that could iterate over the assignments and that iteration being mutable.

[jimmylai]

Makes the Assignment creation lazy looks good.

Another issue is the updated scope example image looks confusing.
The original idea is to some example Python code with their scope visualized in different colors.
The source code of builtin scope is not included in the example Python code. Thus, the class range(...) may make it confusing.
How about change class range(...) as something like (names defined in builtins module).?

[lpetre]

I thought doing the diagram like this would help convey that "range" (used multiple times in the example) is defined in the builtin scope, and that is where accesses will resolve to.

Another note about this diagram: it isn't accurate with respect to what parts of the code will be contained within the various scopes. class Cls is actually in the GlobalScope and def fn is actually in the ClassScope. Not sure if we also want to improve on that?

Maybe we can follow up in a separate PR with some clarifications?

[jimmylai]

I thought doing the diagram like this would help convey that "range" (used multiple times in the example) is defined in the builtin scope, and that is where accesses will resolve to.

Yeah, it'll be better to point out the range code is not included in the example source code.

Another note about this diagram: it isn't accurate with respect to what parts of the code will be contained within the various scopes. class Cls is actually in the GlobalScope and def fn is actually in the ClassScope. Not sure if we also want to improve on that?

Good point. To make it more accurate, the class scope should only contain the code inside the class body, not the class definition line. It's the same for function.

Maybe we can follow up in a separate PR with some clarifications?

Sure, feel free to propose change in another PR.