Fix StarlarkIndexable.getIndex implementations that return null.

[katre]

They should return Starlark.NONE instead.

Part of Optional Toolchains (#14726).

[katre]

I was originally going to fix this in EvalUtils.index, but the comments there make me think that isn't a preferred change. Let me know if that would be easier.

[tetromino]

I would argue that for consistency with dict (and python), getIndex() should throw an EvalException if the key is missing.

[sgowroji]

Hello @katre, Can you please share the update on the review comments. Thanks!

[katre]

@sgowroji: Please note the timestamps: @tetromino commented at about 10pm ET last night, it is currently not even 8am ET now. I appreciate that you are keeping on top of PRs, but please give all contributors at least 12 hours to read and respond.

@tetromino: I will respond once I actually start my work day and can think through your response.

[katre]

There are three cases here:

• ConstraintCollection and the test can just throw the exception.
• StarlarkToolchainContext is more complicated
  • If the toolchain type (the key) is known and a toolchain exists, return it
  • If the toolchain type is known and there is no toolchain, return None
  • If the toolchain type is unknown, throw an exception

The entire point of the work I am doing is to allow for optional toolchains, and the design we settled on there was that ctx.toolchains[/missing/optional/toolchain/type] would return None.

Does this make sense, @tetromino ?

[tetromino]

@katre - understood. I originally misread the code, but I now see that for both ConstraintCollection and StarlarkToolchainContext, getIndex and containsKey behave consistently.

One nit I suggest fixing is update the docstring in ToolchainContextApi.java to explain when context["//pkg:my_toolchain_type"] can be None.

Besides that, LGTM.

[katre]

Good point about the docs, fixed that. Let me know if this is sufficiently clear.

[tetromino]

I am really sorry for being unclear (and for being mistaken in my first comment) - the previous version of the code you hard (returning Starlark.NONE) was correct. For consistency with dict's behavior, we don't want getIndex(x) to throw if containsKey(x) returns true.

[katre]

Oh, I see. Changed it back.