Properly constrain non-`canWrite` files to not change (remaining cases)

[mattmccutchen-cci]

Per discussion with John, we intended that if 3C is not allowed to write a file F per its canWrite check (based on the -base-dir option), it should constrain the annotations in the file not to change. Otherwise, 3C may make changes to other files that depend on changes it generated for F that are silently discarded, and the other files may be uncompilable as a result. But the constraining isn't actually implemented; I thought this code was doing it, but it actually serves an unrelated purpose of generating some statistics. This issue is to implement the constraining.

Update: The most common cases have been fixed in #391. This issue is left open for the tail of unhandled cases and is tentatively labeled "unusual c code - low priority"; we'll remove that label (or file a more specific issue) if someone's actual work is affected by an unhandled case.

A related issue that I'll include here: If this test fails, meaning that we have a modified rewrite buffer for a file we weren't supposed to change, then something is wrong. I hope to be able to make this an assertion error, although conceivably there could be cases where 3C makes a non-substantive change to the file and we would have to try to find a way to assert that the change was non-substantive before discarding it. Update: This was made an error diagnostic, not an assertion error, because of the known unhandled cases.

[mattmccutchen-cci]

There seems to be some fundamental confusion (I'm not yet sure on whose part) about the nature of this issue. Here's an example of what I meant by the issue. It may seem contrived; I was struggling to stop 3C from doing various clever things that got in the way of what I wanted to demonstrate. 🙂

example1.c:

inline void foo(int **p) {}

void bar(int *p) {
  foo(&p);
}

After 3c -output-postfix=checked example1.c, I get the following example1.checked.c:

inline void foo(_Ptr<_Ptr<int>> p) {}

void bar(_Ptr<int> p) {
  foo(&p);
}

And all is well.

Now:
/home/matt/foo_decl.h:

inline void foo(int **p) {}

example2.c:

#include "/home/matt/foo_decl.h"

void bar(int *p) {
  foo(&p);
}

After 3c -output-postfix=checked example2.c, I get the following example2.checked.c:

#include "/home/matt/foo_decl.h"

void bar(_Ptr<int> p) {
  foo(&p);
}

/home/matt/foo_decl.checked.h is not created because it is outside the base dir (which defaults to the working directory). Now clang -fcheckedc-extension example2.checked.c fails with error: passing '_Ptr<int> *' to parameter of incompatible type 'int **'.

The behavior I expected was to leave example2.c unchanged (or perhaps something more clever that also compiles).

So, the presence of the definition of foo in a file that 3C is not allowed to write needs to influence the new annotations of bar. The only way I see to achieve that is to apply a constraint to foo that affects the solution for bar.

Mike and Kyle said they thought I shouldn't modify the constraints, and Kyle further said he thought the second example should be a 3C error. To attempt to generalize Kyle's statement about the behavior: we should solve for changes to all transitively included files but then error if we try to actually change any file outside the base dir? I guess this comes down to: what are the real-world workflows in which 3C would try to change a file outside the base dir (or, how is the -base-dir option intended to be used), and what does the user expect? In any case, I'm pretty sure the current behavior of solving for all files and silently discarding changes outside the base dir (producing uncompilable output) is wrong. @mwhicks1 @kyleheadley Can you clarify what I'm supposed to do?

Note that in stdout mode (#328), we decided (or at least I thought we decided) on a more complex behavior: solve only for changes to files inside the base dir, but assert that no file other than the one specified on the command line (the new version of which is printed to stdout) actually changed.

[mwhicks1]

The behavior I would want is to reject this program and not rewrite it. You have specified example2.c on the command line, and it's within the basedir, but it's necessary for foo_decl.h also to change, as a result of running 3c, but it's not within the basedir. So the whole attempt should be rejected. You should be able to know this because the definition of foo lives in an an ineligible file, and yet that definition is deemed by 3c to be one that should be rewritten.

I interpret what you wrote in the last paragraph as consistent with the above, except that instead of "solve only for ..." it should "solve everything, but reject rewriting all files if any that would change are ineligible."

[mattmccutchen-cci]

Update: We determined at Friday's meeting that my interpretation was correct.

[kyleheadley]

In particular, we decided to relax the assumption that the user intends to convert all code available to their project. Previously, if unchangable code needed updates, we wanted 3c to produced an error. In the new interpretation, the user has the option of having a library outside the changable code, and 3c will leave it as unsafe, with warnings about the cause being external code. The user will need to handle this safety concern manually.

[mattmccutchen-cci]

with warnings about the cause being external code.

To clarify: This is not a new kind of warning. The inability to change external code is just a new root cause of wildness that works with 3C's existing options for reporting causes of wildness.

The user will need to handle this safety concern manually.

Right. AFAICT, the safety risk from code that the user has but chooses not to convert is no different than that from code that the user doesn't have (e.g., system libraries for which we don't have checked headers).

Another reason we needed this feature is that it's not always easy to tell what code should be considered "available" for conversion. Currently, 3C assumes that a function with no definition shouldn't be converted, but it was happy to convert inline functions, structs, and typedefs in system headers, and we were oblivious that this was happening until I added the new error check in #391.