Avoid adding constants to Enumerable

[casperisfine]

Ref: aws/aws-sdk-ruby#2670

Some gems like aws-sdk-core use Object#extend(Enumerable). It's not a very good pattern, but it's somehwat handled ok by Ruby.

However if Enumerable has constants, then any time the module is extended, the global constant cache is flushed and this has a very negative impact on performance for the virtual machine, and even worse for JITs.

I tried to preserve backward compatibility as much as possible, so Enumerable::SoleItemExpectedError can still be referenced.

[byroot]

Discussed this with @matthewd, he agreed that it's a bit ugly, but unfortunately necessary, so I'll merge.

[etiennebarrie]

One thing that's not compatible is Enumerable.constants which used to return [:SoleItemExpectedError] and now doesn't anymore. I don't think that's a huge deal though.

I think that hack shouldn't stick around though, should we deprecate that constant and have it somewhere else, maybe simply ActiveSupport::SoleItemExpectedError?

[byroot]

should we deprecate that constant and have it somewhere else, maybe simply ActiveSupport::SoleItemExpectedError?

Yeah, I though about this and I'd be in favour of it.

@matthewd what do you think?

[byroot]

Another idea was to raise something like IndexError, but not sure if it would be backward compatible enough.

[matthewd]

I like that its current location is more communicative about where it came from (essentially contextualizing the words ("sole") in the actual constant name, helping to point to what's complaining). But it's also pretty easy to mount an argument that we shouldn't be littering constants outside our namespace without a very good reason, and this is not that.

... and thinking further, realistically, it's a "your code has a bug" exception, so not one I'd expect people to have much need of rescuing [and thus referencing]. Yeah, maybe let's just drop it in ActiveSupport. Slightly unfortunate to pollute that top-level namespace, but there's no existing deeper namespace that makes sense... and given the low likelihood of direct references, it's not catastrophic if we decide to move it again later. 🤷🏻‍♂️

(From a quick survey, we do litter Module::DelegationError, while the other core_ext-defined errors end up in separately-necessary namespaced support classes.)