Simplify interning #7714
Simplify interning #7714
Conversation
Use a single generic entrypoint for obtaining interned terms regardless of type. Signed-off-by: Anders Eknert <[email protected]>
anderseknert
force-pushed
the
better-interning
branch
from
c5736c4 to
fa3ceb1
Compare
| return booleanTrueTerm | ||
| // Interned returns a possibly interned term for the given scalar value. | ||
| // If the value is not interned, a new term is created for that value. | ||
| func InternedTerm[T internable](v T) *Term { |
There was a problem hiding this comment.
💭 Makes you wonder why we shouldn't define
func Term(v Value) *Term {
return InternedTerm(v)
}...and make non-interned terms the exceptional case.
There was a problem hiding this comment.
That is tempting indeed. But it'd require even more aggressive disclaimers, as it'd be too easy to reach for Term in places where that term later is mutated, typically by having a Location added at some later point in time.
My original plan was to have an interned package mirroring the relevant functions of the ast one, which would make the difference very clear between e.g. an ast.StringTerm and an interned.StringTerm — and the ideal "design" for the current ast.InternedTerm would IMHO be interned.Term. But the circular dependency check sadly prevents that as long as we want to use interned terms also in the ast package :/
I think there could be some ways around that worth exploring — and given our existing disclaimers about not using interning outside of the OPA project, I wouldn't feel bad about making further changes here later. This seems like a pretty good middle-ground for the time being though.
Use a single generic entrypoint for obtaining interned terms regardless of type.