Replace varnames with Identifiers. #5305
Conversation
ttsugriy
force-pushed
the
env-identifier
branch
2 times, most recently
from
06876a2
to
ad06c08
Compare
|
I've rebased this change to exclude commits that have already been merged. @laurentlb, please take a look. |
This makes the API cleaner, but more importantly opens up new possibilities for optimizations around ways to perform lookups.
|
Can you clarify how this is going to help? For fast lookups, we can add some extra information in (cc @alandonovan @brandjon) |
| .entrySet() | ||
| .stream() | ||
| .collect( | ||
| ImmutableMap.toImmutableMap(e -> Identifier.of(e.getKey()), e -> e.getValue()))); |
There was a problem hiding this comment.
An Identifer is a syntax node. Only the parser should be in the business of creating them.
| @@ -899,7 +905,7 @@ public ConstraintSemantics getConstraintSemantics() { | |||
| } | |||
|
|
|||
| /** Returns all skylark objects in global scope for this RuleClassProvider. */ | |||
| public Map<String, Object> getTransitiveGlobalBindings() { | |||
| public Map<Identifier, Object> getTransitiveGlobalBindings() { | |||
There was a problem hiding this comment.
It is the job of the static resolver (validation) pass to resolve each identifier to the logical variable to which it refers. For a global variable, you could use a small integer index into a flat array, or the string name of the variable (just as the existing code does) since names within a module are unique. It makes no sense to use Identifiers in the representation of the environment: identifiers are not canonical.
I recommend you first read and understand how the Go implementation resolves identifiers and represents the environment, as it is the clearest expression of the necessary algorithm.
|
@laurentlb, I agree that the ultimate goal is to use an approach similar to what go implementation does. The motivation for this change was:
If you don't believe that comments above are relevant, should they be updated? I'll also start looking into go implementation to see how hard would it be to port its logic to Bazel's interpreter. |
|
Good point. Let's remove these two comments. I don't think they are needed to implement (3). |
|
makes sense. Closing this PR. |
Comments are misleading, as discussed on #5305 (comment) RELNOTES: None. PiperOrigin-RevId: 205841782
Comments are misleading, as discussed on bazelbuild#5305 (comment) RELNOTES: None. PiperOrigin-RevId: 205841782
Comments are misleading, as discussed on bazelbuild/bazel#5305 (comment) RELNOTES: None. PiperOrigin-RevId: 205841782
This builds on top of #5304 and replaces varnames with
Identifierinstances tomake the API cleaner, but more importantly open up new possibilities for optimizations
around ways to perform lookups.