To clarify: When you say “in tree”, you mean development and experimentation will happen on the version of Clang-Tidy at https://github.com/llvm/llvm-project?
This is our plan. We have up until this point done our experimentation in a non-public codebase, but we think we’ve answered enough questions that now is a good point to start upstreaming what we have, continue developing in upstream Clang-Tidy, and get feedback from the community. Also, we already have partners who are interested in evaluating the checks but would need them to be in upstream Clang-Tidy to do so.
Our plan is to clearly identify the two proposed Clang-Tidy checks as experimental in the documentation. We will caution users that we expect to make breaking changes to syntax and semantics and that they should therefore use the checks only for testing and experimentation but not for production use. We will work with our early adopters to help them across breaking changes while the Clang-Tidy check is still experimental. Once we consider the design to be sufficiently stabilized, we will remove this experimental qualification from the checks.
We think this approach makes sense because it allows us to get feedback on the design and implementation incrementally (the feedback on this RFC already shows the value of this), rather than in one “big bang”, as would be the case if we did the development out of tree.
Okay, if you’re this far along – do you plan to use the annotate_type spelling directly for exposing the attributes, or do you have a plan to start with more concrete syntax (dedicated attributes, keywords, etc)? This is my primary area of concern regarding breaking users with design changes, so I’d like to either nail down those details (which can be done as part of the patch review, I don’t think we need to nail them down in the RFC) or have a plan for how to help users avoid breakage if we’re not ready to pick the syntax yet (like hiding everything behind macros and expecting the user to use the macro spelling, as an example).
There’s some natural tension here – people integrate clang-tidy into their CI pipelines fairly regularly, and so being experimental with checks in clang-tidy can be highly disruptive to folks who enable those checks (either without knowing they’re experimental or accidentally by enabling all checks in a module), so we tend to push back against significant experimenting in tree. It’s a production tool, so experimentation is somewhat unreasonable for it, but not impossible. At the same time, I agree about getting feedback and the benefits of incremental implementation. We should be aware of this when doing design work here and try to get as many of the important details correct up front as we can in terms of things like syntax; it’s more understandable to vary the semantic behavior than the syntax because users will be happier with different (better) diagnostics than they will be with differ syntax.
That said, the choice of clang-tidy seems incorrect to me. clang-tidy checks are based off AST pattern matching, so they’re typically not control-flow sensitive (and I don’t think we have any that are data flow sensitive). The Clang Static Analyzer seems like a more natural fit for lifetime checking because all of the checks are able to do data and control flow analysis. The architecture of checks is sufficiently different between clang-tidy and the static analyzer that I don’t think it’s particularly valuable to experiment in one with the intention of porting to the other, so I’d encourage you to explore a static analysis-based check. [There is a third option that may be useful for lifetime checks that do not require inter-procedural analysis (if you’re able to identify some): you can do a Clang analysis-based warning, which means Clang itself triggers the diagnostic rather than requiring the user to run a separate tool. However, I suspect the limitation to only checking CFGs within the function will be onerous and that’s why I recommend looking at the static analyzer first.]
CSA is not a good fit because its core is driving a path-sensitive analysis. CSA can solve “may” problems (“try to find a path through the program which leads to a state that violates a predicate P”), but can’t solve “must” problems (“show that no path through the program ends in a state that violates P”).
Inferring and verifying lifetimes is a “must” problem – just like type checking. So CSA is not suitable.
ClangTidy is a general-purpose framework for checks, it is not limited to pattern-matching. ClangTidy provides the AST to the check, and it is up to the check to decide what to do with it.
We have been working on a dataflow framework that allows to compute an all-paths solution to dataflow problems. This framework allows to develop dataflow-based checks in ClangTidy (or in any other system that provides access to the Clang AST).
We are upstreaming our dataflow-based ClangTidy check that verifies that std::optional values are known to have a value before it is unwrapped, on all paths through the program. You can find the tests for it here: https://github.com/llvm/llvm-project/blob/main/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp . The core modeling for std::optional is in clang/Analysis/FlowSensitive because it is a reusable building block for building many other checks, not just the optional unwrapping check. Unfortunately the ClangTidy part is not upstream yet, but we will be mailing it out soon.
While technically it would be possible to add checks to CSA that are not using the path-sensitive part (e.g., it already has some syntactic checks), I think there is another strong reason to not use it at this moment. Currently, none of the checks in CSA will emit fixits. As a result, I think almost none of the IDE integrations would provide the intended user experience when it comes to the annotation inference check. On the other hand, Tidy is using fixits extensively and many integrations already handle them well.
I assume the annotation inference would be an opt-in check. While there are definitely people out there using wild cards to enable checks that configuration is always fragile. Every time we add a new check to Clang Tidy those users has the potential to break their CI.
If this is a concern, I think it should be relatively easy to introduce the concept of an experimental check that cannot be turned on using wild cards, only when the check is explicitly mentioned.
That’s a great point – using the new dataflow facilities would be a useful approach to consider.
It’s a reasonably common approach to use wildcards like bugprone-*, and while that has the potential to break CI because it introduces new diagnostics, users tend to like (and expect) that behavior, in my experience. The issue here isn’t that adding the check will surprise users, it’s that modifying the check behavior significantly (due to the check being experimental) between releases can be disruptive. e.g., the developer has clang-tidy N and the CI has clang-tidy N+1 and they give conflicting diagnostics because of experimental changes. (The issue isn’t unique to clang-tidy; I see the same issues with clang-format version mismatches – but the more experimental checks we add, the more likely it is for users to hit these kinds of issues unintentionally.)
That’s a neat idea and would address my concerns!
That’s why I said “typically” – we do some flow-sensitive checks. However, I’m not convinced this scales well. Each clang-tidy check is run separately from other checks, so there’s no shared flow graph between the checks. This means we need to construct the flow graph for each check, which I understand to be expensive (as is walking it repeatedly in each check). I am very curious how the dataflow framework changes the landscape (I don’t know how expensive it is to produce or traverse compared to say a CFG).
I understand that the wildcard feature has been present for a long time, but at Google we have actually found it to do more harm than good for anything but experimentation. There are multiple reasons, including:
Incomplete, not yet ready checks, or checks that provide low-quality results (at least in our codebase). We use Clang cut and qualified from the main branch ~weekly, so we do pick up ClangTidy checks that are in the process of being landed.
Checks that go against the style guide. When someone enables an open set of checks with a wildcard, there is no guarantee that they provide advice consistent with our style guide. Even if one validates the set of checks that the wildcard enables today, this set will be quietly expanded in future when more checks are added.
Checks that provide conflicting advice. Just like it may be impossible to make a piece of code warning-free with -Weverything, there can be conflicting ClangTidy checks.
Yeah, I think different organizations have different levels of risk they’re willing to take on. FWIW, I personally prefer not using wildcards. But I’ve definitely seen plenty of projects that use wildcards and we shouldn’t ignore that use case.
Thanks for the link! What I meant was mostly that we either are taking the AST and converting it to another graph form (as we do with CFG construction) to walk over, which is expensive, or we are taking the AST and walking it directly in a flow-sensitive manner, which means we’re walking the AST at least twice (once for the tidy check and once for the flow sensitive analysis after the original AST match). Either way, it seems like it wouldn’t scale well in an “imagine if every check did this” situation. However, I’m going to reserve judgement until I actually see performance in practice – right now I’m in FUD territory (sorry for that).
I’ll retract my “maybe we don’t want it in clang-tidy” and instead say: let’s be cognizant of the performance impacts from the checks. Additionally, in terms of experimentation, let’s try to ensure that any surprising breaking changes are kept to a minimum and communicated clearly with users as best we can. Checks change over time and I’m not worried about that – the breakage I am mostly worried about is more of the syntactic variety.
CFG can definitely be built at most once per function but I too am curious about data flow fixpoint iteration done once across all checkers and how well does it scale in this scenario.
Note that data flow fixpoint iteration definitely doesn’t mean going through all AST; it’s usually confined to an individual function under analysis, which typically doesn’t include system headers, which is the majority of the AST. On the other hand, fixpoint iteration may involve a lot more than an extra traversal.
Note that the same problem will arise regardless of where it’s implemented - the fixpoint iteration reuse will have to be implemented mostly from scratch and it’ll have the same performance complexity; the static analyzer’s advantage here is in minor technicalities at best.
I’m also curious whether y’all consider implementing it as a compiler proper warning. IIRC the dataflow framework was designed to be performant enough for this, and these checks also don’t deal with anything API-specific, just a pure language feature.
Two potential concerns with adding it to the compiler itself: it should be an opt-in warning like other analysis based warnings due to the extra overhead (we’ve required this for other such diagnostics that are flow sensitive, such as -Wfallback and -Wthread-safety, due to overhead), and the false positive rate (Clang expects a no/very low false positive rate for new diagnostics, but the static analyzer and clang-tidy have more wiggle room for false positives generally speaking.) But if there’s no issue making it an opt-in warning and we expect it to have a reasonable false positive rate, the compiler seems worth considering to me.
it should be an opt-in warning like other analysis based warnings
Hmm, would these warnings fire at all if there are no lifetime attributes anywhere in the program? If not, would this alone count as opt-in?
Clang expects a no/very low false positive rate for new diagnostics
If the compiler correctly identifies that a lifetime contract is violated but it doesn’t lead to an immediate use-after-free in the program, do we count it as a false positive here?
What would be the lifetime assigned to a at its declaration?
And another question would be how are lifetimes handled at joins (since the proposal says there is no subtyping constraints between lifetimes). In the example above, what’s the lifetime of a at the line cout << a?
While there is no subtyping relationship between lifetimes (AFAIU), there seems to be some notion of “shorter lifetimes" as shown in example: push_back_into_short_lived_vector. This example talks of a shorter lifetime being a supertype of a longer lifetime. I would like to understand how this is enforced.
There are actually multiple warnings proposed. One is for inferring lifetime annotations. That would be triggered for code without attributes.
I think classically we avoided putting warnings that are specific to certain libraries or coding guidelines in the compiler. Following a programming model that can pass the lifetime analysis feels a bit like following a guideline to me. Regardless of this, I don’t have a strong opinion on where to put it.
The subtyping relationship is there and it would handle the cases you specified. The limitations section is talking about the fact that those subtyping relationships cannot be specified explicitly in the code using some annotations. But in your example, the analysis should be able to discover those relationships without any help from the code author.
That’s great, thank you for clarifying. I have one follow-up question: how are lifetimes inferred (or what are the default lifetimes assigned at declaration) for locals?
We perform a pointer analysis to determine what each pointer can be pointing to. For local variables, we don’t necessarily need to assign any explicit lifetime – we simply track all of the different objects that that local variable might be pointing to.
If the local variable ends up being returned from the function (either through the return value or an output parameter) and that local variable is pointing at objects with different lifetimes, then at that point we will need to “collapse” those two lifetimes, i.e. make them the same.
We will be providing design docs that go into much more detail along with the implementation.
Once we have proven that the approach works well on large real-world codebases, we would certainly be interested in integrating the check into the compiler itself. However, we don’t think it would be a good idea to do this now, while the scheme is still experimental. If it doesn’t work as well as we hope, we would have to tear a significant amount of code back out of the compiler; we would like to avoid the risk of having to do that.
@gribozavr and I have discussed this offline, and we’d like to propose creating a new experimental category of Clang-Tidy checks that would contain the lifetime checks and potentially other checks that still need to “mature”. This would have the following advantages:
If people are using wildcards to select a whole cateogry of checks (e.g. bugprone-*), experimental checks would automatically be excluded. (Question: How common is it for people to specify *, i.e. “give me all the checks”? If people do this, we should discuss whether experimental should be excluded from * and should only be included if people specify it explicitly, i.e. *,experimental-*
The name of the check itself would indicate clearly that the check is experimental.
LIkewise, the implementation for all of the experimental changes would live in a separate directory, again making their status clear.
The main disadvantage would be that there would be a bit of churn when a check is promoted from experimental to non-experimental – but this churn would be limited:
Clang-Tidy already has functionality for referring to a check by several different names. We could therefore keep the old experimental name, so that people wouldn’t need to update existing configurations.
Apart from that, the main churn would be moving the implementation (not a big deal) and updating documentation to use the new name (should be easy to do with search-and-replace).
Getting back to a question that I hadn’t yet gotten round to answering.
There’s been a lot of feedback on this thread (thank you!), so we may have overlooked other questions people had. If someone had a question or comment that we haven’t responded to yet, please let us know!
Yes, we will recommend that people use macros. This makes sense not just to protect against breaking syntax changes but also as a shorthand. The raw annotate_type macros are pretty verbose and kind of drown out the rest of the function signature (see the first code sample in the “Example” section). A shorthand syntax (e.g. $a or a more typical macro name such as LT_A) helps make the signatures much more readable.
At some point, once we have validated the approach, we do intend to introduce dedicated attributes and possibly keywords for lifetimes, but we’d like to avoid introducing something now that we’ll have to change later, possibly multiple times, or even tear out again completely.
So I think experimental probably should be excluded by default from *.
I think this is reasonable. Personally, I sort of wonder if we want to explicitly disallow (with assertions) someone from making an alias into an experimental check. If the primary implementation of the check is experimental, it seems ill-advised to make an alias to it. And if it’s no longer experimental, I think we want the primary interface to move out of the experimental directory. (It seems more reasonable to add an alias from a non-experimental check into the experimental module, both to ease this transition for users but also because there may be times when we want to add experimental options to an already-stable check to see how well they work in practice.)
I think using macros is a good approach (it’s the one I’d recommend), but we should also try our very best to make sure that the macro interface will continue to work even if we decide later to switch to dedicated attributes or keywords. e.g., if the macro is function-like or object-like, it needs to stay that way; be aware of syntactic locations where the user is writing the macro and be sure that whatever it expands to remains valid there, etc. Hopefully experimentation doesn’t change the design such that the macro interface can’t be used seamlessly between experiments (so far, I don’t see any reasons why it would, so this is more just “please keep it in mind” feedback).
Thanks for researching this – I agree, in this case experimental should not be included in *.
Agree – I think it makes sense to enforce this through assertions.
This is a good point.
I’m expecting most of the experimentation / churn to be around the semantics, rather than the syntax of the annotations.
In terms of syntax, the fundamentals seem mostly settled. It seems clear we want to be able to identify lifetimes by name in annotations, and that these annotations should be type attributes of some form. This means that macros that a user defines for these attributes can be adapted if we change the specific name of the attribute later on.
Semantics aren’t as settled yet, and we may need to make changes to these, for example which places lifetime parameters can be applied to. It seems likely that the rules will become more rather than less permissive over time, so existing code should continue to work. However, if we implemented the annotations as dedicated attributes in Clang today, the rules for where they are allowed to go should also be implemented in Clang, and any change to those rules would necessitate a change in Clang itself.
But your point is well taken – we should try to minimize any changes to the annotations that would invalidate annotations users have already made, even if those annotations are only for experimentation so far.