The static constants AbstractTermFactory.EMPTY_LIST and TermFactory.EMPTY_LIST should be declared private because their use potentially contradicts the value of defaultStorageType that would be used when calling TermFactory.makeList(). In particular, building an application with an EMPTY_LIST argument fails in ParentTermFactory because no attachment can be registered to EMPTY_LIST.

Submitted by Sebastian Erdweg on 18 March 2011 at 11:47

Good point. I think it should be marked as deprecated. Internally, a different private field could be used. For other classes, a call to makeList() should be used instead.

I am actually experiencing bugs because of this issue, thus I would prefer a more invasive fix then depreciation only.

I’m always hesitant to just break binary compatibility like that. The real problem is with how EMPTY_LIST is used now. I think all those uses should be reconsidered. Many of them are just harmless comparisons, intended to help performance a bit, they could use some kind of package-internal SHARED_EMPTY_LIST field instead. The cases where EMPTY_LIST is actually returned are harmful. One particularly problematic case is probably StrategoTerm.getAnnotations(). There doesn’t seem to be a nice way to fix that one; it really should be shared. If a term t has an empty list of annotations t’, then what are the annotations of list t’?

Perhaps ParentTermFactory and OriginTermFactory should just handle EMPTY_LIST as a special case, and create a new, unshared object for it? Which also wouldn’t be very nice…

Wouldn’t it also make sense to just generate new non-shared term whenever an attachment is registered to a shared term. Then you have lazy duplication of terms and the existing code is untouched.

Apart from that I agree that the uses of EMPTY_LIST need to be reconsidered.

For parent terms it would be prohibitively expensive to clone terms in the general case. Trees are constructed in a bottom-up fashion, that would be a lot of recursive cloning operations of increasingly larger trees. The parent term factory is only used inside the parser, or if you happen to use the SSL_EXT_clone_and_set_parents primitive, so mutably setting parents should be safe as the subterms can never escape before the tree is fully constructed.

Origins are a different story: in Spoofax, the OriginTermFactory is always active. For the OriginTermFactory I don’t think it would be a problem to clone small subtrees like [] though, so perhaps that factory should just have a getStorageType() test to see if it should clone a tree before adding the attachment.