The problem is that in general, the way we understand move in C++ is that, once moved an object is valid (it can be destroyed), but all the invariants may or may not be upheld depending on what the library decides to do.
In general there is an expectation that a moved-from objects can be assigned-to.
Smart pointers have that property that they are set to nullptr after move, it’s the exception rather than the rule.
Maybe what this is trying to get to is ”do the preconditions of a given operation hold after move”. Or rather “do that function has any precondition?” for example, calling vector.clear() on a moved-from vector is fine, calling vector.operator[] is not.
By that logic either we’d want a
You can call any function on a moved-from object of that type
You can call this specific function on a moved-from object of that type. this gets tricky because of non member functions and template.
But “You can call any function on a moved-from object of that type” may or may not be what you want. you can call vector.push_back on a moved from-vector, it will not crash. But adding an element to a container whose content is completely unspecified is probably incorrect logically even if the standard says it’s “valid”.
So I think we get back to the point @erichkeane made that the behavior of unique_ptr might be too… unique (see what I did there?) to justify something more elaborate than a hard coded list.
I don’t think that will solve the underlying concern because (e.g.) plugins could look for particular annotation strings and modify how lowering to LLVM IR happens, etc. I think [[clang::annotate]] likely is the correct underlying mechanism for this, but it would need some sort of schema for the argument to the attribute to help avoid conflicts with other uses of the attribute.
But taking a step back, I don’t think I understand the idea well enough to judge its value. “This is basically a that” kind of attribute hides the important detail that “basically” is load-bearing. Is a std::wstring basically a std::string? Well, that depends – if you’re just looking at these as containers of a sequence of code points, then string, wstring, std::vector, and StringRef are all basically the same thing. But if you look at them for the semantics conveyed, a string and a wstring are quite different from each other, and both are different than a StringRef.
I completely agree, I don’t think this was a rational argument. It’s more of a “marketing” issue I’ve ran into in the past. I personally don’t see this as a problem and I have no stakes in this right now.
Yes absolutely. Just like every other use of [[clang::annotate]].
Yes, we still need a cleverer schema to avoid conflicts and misunderstandings, just like with annotate. As in, “a std::wstring is basically a std::string for the purposes of bounds-checking operator[]according to my new clang-tidy bounds checker” or something of that nature. FWIW I don’t see any benefits to this over simply using [[clang::annotate]]other than being narrower for being-narrower sake.
I’m completely on board with simply starting to use [[clang::annotate]]as the universal answer to every request for a new attribute that isn’t used by compiler itself.
I do think so. It’s one of the reasons to propose the attribute. Annotation types that behave like smart pointers would make a difference eliminating false positives in bugprone-use-after-move.
@martinboehme also mentioned to me this alternative, and the concern. As far as I can tell if we use [[clang::annotate]] to annotate decl types, as [[clang::null_after_move]] would do, it does not propagates to the LLVM IR, does it? If someone knows better than me, please feel free to educate me.
I guess there is still the residual risk that someone misuses the attribute, resulting on the annotation being propagated to the LLVM IR. Is there a way to mitigate the risk? conditional compilation?
Quite frankly, I’m not entirely sure of all the implications of having annotations propagating to the IR, therefore not sure if it’s the correct thing to do universally at this point. The potential impact on optimizations discussed in [RFC] New attribute: annotate_decl sounds inconvenient to me. Although, afaiu from the conversation the interference with optimizations would not be expected and would be a bug. But I’ve a very limited view of the problem, frankly.
But for the purpose or this RFC, clang::anotate may be a solution.
Is there preferred/standard schema for defining annotations with clang::annotate? I couldn’t find documentation, please feel free to tell if I missed it.
Maybe something like: [[clang::annotate("bugprone-use-after-move", "null_after_move")]], where the user of the annotation and the intention are expressed?
Thoughts?
ping @martinboehme, @erichkeane ^^^ Does clang::annotate sound like a path forward if the attribute in this RFC is found too specific to be added as a new attribute to clang? If so, is it my understanding that is the responsability of bugprone-use-after-move to define and document how clang::annotate is used by the check?
Based on the problems with annotate, I dont’ think it is THE solution, but I think it is neighbor-to-the-solution. Its annoying, because ‘annotate’ is a GREAT name for what you want.
Having thought about this direction for a grand total of like… 10 minutes, I wonder if there is value in some sort of inform_my_static_analysis_tool attribute. I WOULD like to see if we could make that eminently flexible though. It should be able to support instantiation, travelling across multiple instantiations/etc (AND we should decide on its behavior for moving to partial specializations/etc).
ONE Problem I was thinking about with the null_after_move attribute is: What if I ONLY make it null if the type is trivially destructible? Or vice versa? We would want to be able to set the attribute based on substitution.
Another final thought: What about C++ Standard Annotations? Could/should we just use those instead?
I think we should mention clang-tidy explicitly eg. ["clang-tidy", "bugprone-use-after-move", "null_after_move"]. We cannot demand developers of other tools to consult the list of clang-tidy checkers in order to check their own attribute for potential conflicts. But we can expect them to avoid naming their entire tool “clang-tidy”.
Other than that, yes, sounds perfect to me, this is exactly how I imagine it.
As much as I want to give an answer here, quite frankly I don’t think I’m well equipped to give a solution for a flexible enough framework that can be used by all static analyzers out there. Although if something flexible enough is proposed I’d happily use it.
That said, and since I’m here, I’ve the feeling that any flexible enough framework, expressed as attributes, may become quite similar to [[clang::annotate]] (but that is kept in the AST). And at that point the discussion (or part of it) will kind of overlap with [RFC] New attribute: annotate_decl.
I’d love to hear what’s the opinion of people more familiar with static analysis though.
I’m not sure I follow you in this question, sorry. May I ask if you’re talking about some annotation you already have in mind or you think is worth considering?
Yes, I do think you are right about this. thx for the feedback.
This paper has been accepted for C++26, has a partial implementation in Clang (IIRC? maybe still under review?), and is likely to be exposed in other modes if we can. Something like this is perhaps more useful?
Thank you for sharing this. It clarifies for me what you had in mind. I took a first look today. I was not familiar with this proposal/new feature, so don’t hesitate to educate me.
My initial thoughts are:
We’ll have to wait until C++ 26 is widely adopted. Personally, I’d like a solution I could use in the short term.
For the purpose of static analysis, the annotations seem to share some disadvantages with attributes:
If individual tidy checks (or static analysis tools) want to recognize annotations, the annotations somehow need to be defined and exposed so that user code can use them. Similar to tool-specific attributes requiring to be registered. Even more. I’d say with annotations user-code will need to include the code that defines the annotation.
You may define generic annotations. Afaiu, similar to having [[clang::annotate*]] attributes, you would still need to agree in some kind of schema.
I do admit these annotations seem to provide flexibility when facing situations like you mentioned:
While I don’t see that as a problem for recognizing smart-pointer-like types in bugprone-use-after-move, if it was an scenario we’d like to support afaiu we could:
Support also [[clang::annotate_type]], probably useful for scenarios with very few exceptions, kind of inconvenient otherwise(??)
Use AST parsing. Afaiu, if we traverse the AST using the “AsIs” mode template, instantiations are visible and should be possible to query if the type in one instantiation is trivially destructible.
I, personally, think we can improve bugprone-use-after-move without waiting for these annotations. But looking forward to your thoughts and others. Also, kindly let me know if I misunderstood something.
>We’ll have to wait until C++ 26 is widely adopted. Personally, I’d like a solution I could use in the short term.
Probably less time than a new feature TBH. And I suspect many compilers will just expose these in older modes. Clang either already has them, or will very soon.
>If individual tidy checks (or static analysis tools) want to recognize annotations, the annotations somehow need to be defined and exposed so that user code can use them. Similar to tool-specific attributes requiring to be registered. Even more. I’d say with annotations user-code will need to include the code that defines the annotation
>You may define generic annotations. Afaiu, similar to having [[clang::annotate*]] attributes, you would still need to agree in some kind of schema.
Yep, same as clang::annotate in all of that. The difference at least is that they are ‘standards compliant’ so most parsers will get them by default/the rules are fixed as to what goes into them.
Just to comment on this and other discussion of [[clang::annotate]] above.
I believe from explorations I did a while ago that a [[clang::annotate]] on a class declaration doesn’t have any impact on optimizations. However, I don’t understand everything the compiler does with attribute well enough to be sure.
Agreed, my experience is the same. Afaict annotations on class declaration types are not emitted to the IR.
I played with code and, afaict, if we wanted to do such check in bugprone-use-after-move (again, I don’t think so at this moment), it’s something we could indeed do with AST parsing.
@erichkeane is the above enough to make [[clang::annotate]] suitable for bugprone-use-after-move for the purpose intended with this RFC? Or do you strongly feel that this is not a valid use of clang::annotate?
Others: afaict there is no consensus on adding a new attribute (clang::null_after_move or clang::specified_after_move). But there seem to be consensus about using clang::annotate instead (with an schema to help avoid conflicts with other uses of the attribute).
But please, if someone else thinks clang::annotate is not a valid solution feel free to tell. I’m not trying to dismiss anybody.
I don’t feel strongly about using annotate for it. If folks haven’t noticed problems with opt on it, it seems reasonable. And if it ends up BEING problematic, perhaps we can re-visit this.
This feels like way too specific a use. I sympathize with the amount of work involved but I would much prefer the NullAfterMoveTypes solution or just annotating code using NOLINT which honestly feels like the correct approach.
Adding a new feature is not always the right trade-off for things that are a lot of work.
Sorry, which solution are you pointing as the NullAfterMoveTypes solution? Several ideas have been discussed in the thread, so I want to be sure I’m in the same page
The motivation for this proposal is that we have bugprone-use-after-move enabled in our codebase. And some users are not happy with having to annotate with NOLINT false positives on valid uses of smart-pointer-like types (when they did not have to do nothing when using unique_ptr for instance). And quite frankly I sympathize with them. I think they have a valid point.
Since [[clang::annotate]] already exists, I don’t think modifying the bugprone-use-after-move tidy check to recognize type declarations annotated as described in this thread can qualify as a lot of work. I definitely agree that there is some coding work though. But imho is worth the trade-off.
Therefore, I plan to send pull request to update the bugprone-use-after-move clang-tidy check to, using the above mentioned annotations, ignore uses of smart-pointer-like types that don’t dereference the pointer (similar to what the plugin already does with standard smart pointers).
I’m posting the update as a comment, as I don’t think I can edit the topic anymore? I may be missing the option though. I’m not a regular discourse user.
Kindly let me know If I should seek/wait for a more kind of formal approval before I open any Pull Request. It’s my first time going through the RFC review process.
It will take me a few days to work in the code anyway, so I’ll read any update/new comment that may be posted in this topic in the meantime
I just opened a PR to extend the use-after-move check to recognize user defined smart-pointer-like types as discussed in this topic:
Sorry that I didn’t add anybody that participated in the RFC discussion to the pr. I think I lack permissions to assign people or reviewers. So I’m pinging this topic instead to invite anybody interested
