bazel icon indicating copy to clipboard operation
bazel copied to clipboard

`--incompatible_existing_rules_immutable_view` breaks json encoding of `existing_rule/s` objects

Open tetromino opened this issue 1 year ago • 5 comments

Originally posted by @dws in https://github.com/bazelbuild/bazel/issues/13907#issuecomment-1239478273

If --incompatible_existing_rules_immutable_view is set, json.encode (or encode_indent) will encode an immutable native.existing_rule or existing_rules object as an array of field names, instead of a dict (because our DictLikeView object is a StarlarkIterable).

proto.encode_text is worse: it will fail with Error in encode_text: [...] got ExistingRulesView, want string, int, bool, or struct when trying to encode a struct one of whose fields has an existing_rule object as value.

Although there is a workaround (encode dict(native.existing_rule("foo").items())), this is obviously a regression compared to the situation before the flag flip.

tetromino avatar Sep 12 '22 21:09 tetromino

I think a solution would have the following shape:

  • fix https://github.com/bazelbuild/bazel/issues/13829
  • ... allowing us to have a non-throwing version of get() for DictLikeView, in turn allowing DictLikeView to extend Map (see https://github.com/bazelbuild/bazel/issues/13605#issuecomment-939146471 for why we want to extend Map and not go with a custom map-like creature)
  • ... which would immediately fix json; for proto, we'd have to allow it to support arbitrary mappings, not just Dict.

An alternative would be for DictLikeView to implement Json.Encodable (whose interface we'd need to change a bit), and the corresponding ProtoModule.Encodable (which we'd need to add) - but IMHO that would be quite ugly. Especially the duplication between the two encodable interfaces.

tetromino avatar Sep 12 '22 21:09 tetromino

@brandjon fyi

tetromino avatar Sep 12 '22 21:09 tetromino

After looking at this a bit, proto.encode_text cannot reasonably support the output of native.existing_rule(): the encoder cannot produce any reasonable schema for a dict (or dict-like entity) whose values may be lists/tuples, as opposed to scalars. Which is a reasonable limitation, considering the proto format.

We do still want to fix json encoding, however.

tetromino avatar Sep 15 '22 22:09 tetromino

@comius Is this something you can look into? This is also blocking the 6.0 release.

meteorcloudy avatar Sep 16 '22 09:09 meteorcloudy

@meteorcloudy - I have a fix in review internally

tetromino avatar Sep 16 '22 14:09 tetromino

Bazel 6.0 cut is scheduled on Oct 10th, do you think we can fix this in time?

meteorcloudy avatar Sep 28 '22 09:09 meteorcloudy

I hope to submit the fix this week

tetromino avatar Sep 28 '22 15:09 tetromino