Improve names of ConstraintVariable, etc.

[mattmccutchen-cci]

I previously wrote:

Based on my experience developing the canWrite constraints PR (#391) as a newcomer, I'm starting to think some things in our code are very poorly named. For example, I would expect the term ConstraintVariable to refer to what we actually call a VarAtom. Perhaps what we now call a ConstraintVariable should be renamed to something like AnnotatableObject, since it may contain any number of VarAtoms and constraining the variables is just a means to the end of applying Checked C annotations to the object. Then PointerVariableConstraint (which is neither a variable nor a constraint) would become AnnotatablePointerType, and FunctionVariableConstraint would become AnnotatableFunctionSignature. (Probably some of the less prominent things should be renamed too; I haven't gone through all of them yet.)

(Addendum: My original proposal was ConstrainableObject, etc.; AnnotatableObject was a new idea this morning. Then I realized that one way in which ConstrainableObject is superior is that there are some things we annotate that don't have their own constraint variables, such as checked regions. There might be a compromise such as SolvableObject, or maybe that's too unwieldy and we should go with ConstrainableObject.)

John added:

Also, I've been thinking we should split those classes out into separate files. ConstraintsVariable.cpp is almost 2k lines which makes it a pain to edit.

I plan to work on this after our upcoming PR to Microsoft. I'm starting this issue thread so we can discuss the new names we want before I start a draft PR; it seems awkward to start the draft PR and then discuss the names there.

[kyleheadley]

If the only change is a refactor from PointerVariableConstraint to ConstrainablePointerType and a couple other similar ones, I think it would be more harm than good. Names don't have to be perfectly descriptive if they are common enough, consider how easy it was for you to understand and mentally rename what's there. And there are other confounding factors, like:

• typedefs like PVConstraint that increase the number of names to change
• Variables with acronyms in their names, like ExtCV that means "ExternalConstraintVariable"
• Subtle differences in definition that might be just as annoying to others as these were, like when you changed the above from "Annotatable" to "Constrainable".

Personally, I'd prefer not to use typedefs for renaming, especially since CLion doesn't search across them. And I prefer variable names whose lengths are correlated with both their scope and rarity. Here "medium" like "ConsideredPointer". But the biggest problem I see is the 2nd, that there are a lot of variables with names that reference the current type names. "PCV" becomes meaningless if its type is "ConstrainableObject". It's a bad variable name, but it's a lot more work to change everything like that.

What's the proposal for dealing with variables whose names reference the current class names?

[mattmccutchen-cci]

You have some valid points. I think PointerVariableConstraint is a really poor name and if we were starting from scratch, I feel sure we could come up with something we could all agree is better, though due to the "subtle differences" you mention, we'd want to talk it over to find the new name we collectively feel best about. Of course, we're not starting from scratch: most of us are familiar with the old names and would have to learn the new ones, and there would be merge conflicts with other pending work. Plus, if we do this properly and change all the other related names (variable names, etc.) as you mentioned, it becomes even more disruptive. On the other hand, if we do get all of this done, it will pay at least some dividends over time (particularly for new engineers), but I don't know how large the benefit will be or how long it will remain relevant before something else changes.

I think the issue of the typedefs is largely separable: we could inline them now, or we could rename them to shortened forms of the new names with the option to inline them later.

At this point, I don't think I know enough to conclude either that I want to go ahead with the change as soon as I have time or that it will never be worthwhile to make this change, so I'm just going to leave this issue open. Further comments would be welcome.