google / closure-templates

A client- and server-side templating system that helps you dynamically build reusable HTML and UI elements
Apache License 2.0
640 stars 194 forks source link

Change SoyValue to be an `abstract` base class instead of an `interface`. #2171

Closed copybara-service[bot] closed 2 months ago

copybara-service[bot] commented 2 months ago

Change SoyValue to be an abstract base class instead of an interface.

Merge many Abstract* classes into their interfaces which have now been made abstract. Mark as many methods final as possible.

The goal here is to:

The trickiest part of this is the multiple inheritance in the collection hierarchies. Before we had SoyMap SoyRecord and SoyLegacyObjectMap as parallel interfaces. Many of the implementations like DictImpl or SoyMapData implemented all 3, some like SoyMapImpl or SoyRecordImpl implemented only 1. To fit everything into the singly-rooted hierarchy we had to pick a hierarchy across these 3. I decided on....

SoyMap extends SoyRecord extends SoyLegacyObjectMap

The reason I selected this relationship was due to prior art for SoyProtoValue, and this also seemed to be mostly compatible with the Js object system. if SoyLegacyObjectMap is just Object<string,?> then SoyRecord is some concrete struct (which can always be upcast to object), and SoyMap is an es2015 Map which is a very special kind of object.

The main alternative I considered was changing many methods to be static. This is a much larger breaking change, and at least my attempt ended up being a regression. AFAICT, The reason why is because static -> virtual -> static is worse than virtual -> static. So the only really good usecase for making things static is if they don't need to call a virtual method, if they do then we are inhibiting the JITs ability to understand that virtual call. Making a method final is just as valuable and far more compatible so this approach ends up being simpler.

Changing the type hierachy is risky, so i spent some time auditing various instanceof tests on the SoyLegacyObjectMap and SoyRecord interfaces to see if there might be latent issues and these are thankfully quite rare and seem fine.