This is an archive of the discontinued LLVM Phabricator instance.

[Index] [clangd] Support for concept declarations and requires expressions
ClosedPublic

Authored by ilya-biryukov on Apr 26 2022, 2:12 AM.

Details

Summary

Add support for concepts and requires expression in the clang index.
Genarate USRs for concepts.

Also how RecursiveASTVisitor handles return type requirement in
requires expressions. The new code unpacks the synthetic template parameter
list used for storing the actual expression. This simplifies
implementation of the indexing. No code seems to depend on the original
traversal anyway and the synthesized template parameter list is easily
accessible from inside the requires expression if needed.

Add tests in the clangd codebase.

Fixes https://github.com/clangd/clangd/issues/1103.

Diff Detail

Event Timeline

ilya-biryukov created this revision.Apr 26 2022, 2:12 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 26 2022, 2:12 AM
ilya-biryukov requested review of this revision.Apr 26 2022, 2:12 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptApr 26 2022, 2:12 AM
Herald added a subscriber: MaskRay. · View Herald Transcript
sammccall accepted this revision.Apr 26 2022, 2:46 AM

This looks great!

Only thing I'm uncomfortable with is the lack of test coverage outside clangd.
I think the most valuable thing would be to have some direct testing of how RecursiveASTVisitor behaves.
Do you think you could add something to unittests/Tooling/RecursiveASTVisitorTests/Concept.cpp at least for your change and if there are any other bits that seem critical & missing?

clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
3577

maybe also a constrained auto usage?

3584

I think it'd be useful to assert the presence/absence of template brackets/placeholders here, even if the behavior is not ideal today (in which case, comment to that effect).

Similarly it's probably worth having a concept B with two type params as these are different cases for template brackets when used as type (even if the behavior is the same)

clang-tools-extra/clangd/unittests/XRefsTests.cpp
1851

nit: does anything break if we just set c++20 unconditionally?

2079

maybe = 'c' and use char below? Don't make me think :-)

This revision is now accepted and ready to land.Apr 26 2022, 2:46 AM

This looks great!

Only thing I'm uncomfortable with is the lack of test coverage outside clangd.
I think the most valuable thing would be to have some direct testing of how RecursiveASTVisitor behaves.

Just to be clear, I feel it is not RecursiveASTVisitor that's at fault here. It's rather design of the Index library that splits work into multiple visitors.
Requires expression is produces a template parameter list that suddenly appears out of nowhere in the BodyVisitor, whereas the code to handle it is in the DeclVisitor.

Do you think you could add something to unittests/Tooling/RecursiveASTVisitorTests/Concept.cpp at least for your change and if there are any other bits that seem critical & missing?

I actually think neither way to do the traversal works in all cases, so I'm reluctant to add a test, which feels like picking which behavior is better.
The change I have avoids code duplication in the index library and is probably more convenient for source tooling in general.
However, any code that needs to examine all elements of the AST for whatever reason (e.g. dumps or serialization) would probably be better off walking the synthesized template parameter list.
The idea is: it's not important how exactly we traverse the nodes there as long as all interested visitors get callbacks for nodes they're looking into.

The proper refactoring here would be to add Traverse and Visit methods for concept::Requirement and one for ExprRequirement::ReturnRequirement. Overriding VisitReturnRequirement in BodyVisitor would be enough and we can still visit the template parameter list like before.
I'd like to do this in a separate change, would that work for you?

ilya-biryukov marked 4 inline comments as done.
  • Check 'Concept auto' completion,
  • set -std=c++20 unconditionally,
  • use char instead of int.
clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
3584

The problem is that we currently add the template brackets everywhere.
I forgot to mention that I didn't solve this issue yet, it requires adding a new completion context and seems like a good independent change.

I've been collecting missing cases in the github issue, will make a note there.

clang-tools-extra/clangd/unittests/XRefsTests.cpp
1851

Yeah, good point. Other tests don't depend on it.

2079

Done. Just to make sure we're safe against new architectures for the next 250 years of C++ history.

  • clang-format the code
This revision was landed with ongoing or failed builds.Apr 26 2022, 6:50 AM
This revision was automatically updated to reflect the committed changes.