[Lldb-commits] [clang] [clang-tools-extra] [lldb] [clang] Reland: Instantiate alias templates with sugar (PR #101858)

[Matheus Izvekov via lldb-commits]

mizvekov wrote:

> I don't know, but it would be a lot of work, and would likely still mean preserving the same information as `SubstTemplateTypeParmType` nodes currently provide.
> 
> Our scope is also larger than what Clang's resugarer aims to do, because for our purposes sometimes there isn't even one correct sugar to be permanently recorded in the AST. For example, we also want to propagate our annotations through _instantiated function template bodies_ so that we can analyze function template instantiations in a context-sensitive way. For example:
> 

I wouldn't say that's outside the scope of clang's resugarer. I just don't think that can be something we can always enable,
as that would make some examples of source code take exponentially more time and memory to compile.

Right now the priority is to implement the stuff that won't take a big hit on perf. But that is just priority.
Clang's resugarer can still get there first, even if that's lower relative priority.

> Yes, and we consider those cases to be bugs, we just didn't get to them yet...

I don't think, from looking at the resugarer implemented in `type_nullability.cc`, it's anywhere close to being able
to resugar through a complex type like an STL vector, as shown in the example.

I think the first thing you would find you need is to get those default arguments resugared, as for example both libc++ and libstdc++ vector implementations derive the element type not through the first template argument, but through the allocator instead.

> > As for my first question, can you think of an alternative way we can go about this? What would you need to be upstreamed in order for our own transforms to handle this for you?
> 
> PTAL at https://github.com/google/crubit/blob/main/nullability/type_nullability.cc, especially the `NullabilityWalker` and `Rebuilder` abstractions to get a rough idea of what we're doing. While the code is complex, it is reflecting the nature of the problem.

I see the implementation of the resugarer in `type_nullability.cc`, and it's much more incomplete that the one I am proposing to upstream. So I think if you get it to work through upstream transforms, you will get there faster.

> However if upstream Clang provided base classes that only did this, without exposing the underlying data, we would lose out on flexibility. I'm also not sure that the resugaring algorithms are exactly the same between nullability and lifetime analysis.
> 
> The alternatives I can think of are all much worse than leaving `SubstTemplateTypeParmType` nodes and admitting that they are public API, and these alternatives don't necessarily allow us to remove `SubstTemplateTypeParmType` or some other equivalent marker anyway. For example, adding a more general API for pluggable type system extensions; such facility is quite nebulous and would either compromise on flexibility (e.g., would only propagate C++11-style attributes on a best-effort basis), or would be flexible but cost too much complexity in Clang's core for the benefit of only a few out-of-tree users.
> 

Can you explain in more concrete terms how you think the sugared substitution as implemented in this patch creates impediment relative to what you need in your project?

As far as I can see, you don't need to keep the Subst* nodes after resugaring if you implement:
* AST Type sugar nodes which represent nullability and lifetime annotations.
* Teach getCommonSugar type how to fold these annotations. This will implement what should happen when for example different lifetimes appear in each arm of a ternary operator, or each return statement of a function with deduced return type.

So then our transforms should still do the right things you need for them.

You would still need to implement the semantic analysis for these annotations, even if externally. But that still should give a lot of flexibility.

> Note that we are hoping to eventually upstream some of our work (see, for example, our RFC thread on lifetimes [ [RFC] Lifetime annotations for C++](https://discourse.llvm.org/t/rfc-lifetime-annotations-for-c/61377)), but confidently proposing a complex type system extension like this requires having solid implementation and usage experience, which in our case would come from Google-internal deployment. If the extension points for out-of-tree usage get removed and we are forced to move our code upstream before we actually understand what a good design would be, arguably everyone would be worse off.

I don't think the bar is that high for starting to upstream the work. As long as this is marked as an experimental extension that must be manually enabled, you can still treat this as a Greenfield thing, and do your experiments.

I was aware of the lifetime annotations thread, I even commented on it.
I informed about my resugaring work, which google helped finance by the way.

https://github.com/llvm/llvm-project/pull/101858