Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Missing FoundDecl in ConceptReference. #82628

Closed
hokein opened this issue Feb 22, 2024 · 7 comments · Fixed by #85032
Closed

Missing FoundDecl in ConceptReference. #82628

hokein opened this issue Feb 22, 2024 · 7 comments · Fixed by #85032
Labels
c++20 clang:frontend Language frontend issues, e.g. anything involving "Sema" concepts C++20 concepts

Comments

@hokein
Copy link
Collaborator

hokein commented Feb 22, 2024

Given the following code, the FoundDecl is null in the Foo's AST node (ConceptReference).

namespace ns {
template<typename T> concept Foo = true;
}

using ns::Foo;
template<typename T>concept Bar = Foo<T>;  // reference `Foo` points to the underlying of ns::Foo, rather than the `using` decl.

This breaks some clang tools: include-cleaner will report the file of the underlying decl, rather than the one for using decl; clangd's go-to-def on Foo jumps to the underlying decl.

@hokein hokein added the clang Clang issues not falling into any other category label Feb 22, 2024
@EugeneZelenko EugeneZelenko added clang:frontend Language frontend issues, e.g. anything involving "Sema" and removed clang Clang issues not falling into any other category labels Feb 22, 2024
@llvmbot
Copy link
Collaborator

llvmbot commented Feb 22, 2024

@llvm/issue-subscribers-clang-frontend

Author: Haojian Wu (hokein)

Given the following code, the `FoundDecl` is null in the `Foo`'s AST node (`ConceptReference`).
namespace ns {
template&lt;typename T&gt; concept Foo = true;
}

using ns::Foo;
template&lt;typename T&gt;concept Bar = Foo&lt;T&gt;;  // reference `Foo` points to the underlying of ns::Foo, rather than the `using` decl.

This breaks some clang tools: include-cleaner will report the file of the underlying decl, rather than the one for using decl; clangd's go-to-def on Foo jumps to the underlying decl.

@zyn0217 zyn0217 added the concepts C++20 concepts label Feb 23, 2024
@zyn0217
Copy link
Contributor

zyn0217 commented Feb 25, 2024

I think the reason is that LookupResult::getFoundDecl() returns the underlying Decl of the target UsingShadowDecl.

NamedDecl *getFoundDecl() const {
assert(getResultKind() == Found
&& "getFoundDecl called on non-unique result");
return (*begin())->getUnderlyingDecl();
}

if (R.getAsSingle<ConceptDecl>()) {
return CheckConceptTemplateId(SS, TemplateKWLoc, R.getLookupNameInfo(),
R.getFoundDecl(),
R.getAsSingle<ConceptDecl>(), TemplateArgs);
}

Perhaps it should have been R.getRepresentativeDecl() based on the intent of ConceptReference::FoundDecl.

@zyn0217
Copy link
Contributor

zyn0217 commented Feb 25, 2024

And applying that modification gives us the AST:

`-ConceptDecl 0x7fd5249c13c8 <line:6:1, col:40> col:29 Bar
  |-TemplateTypeParmDecl 0x7fd5249c1230 <col:10, col:19> col:19 referenced typename depth 0 index 0 T
  `-ConceptSpecializationExpr 0x7fd5249c1398 <col:35, col:40> 'bool' UsingShadow 0x7fd5249c11b0 'Foo'

compared to that before:

`-ConceptDecl 0x7fe2b920b298 <line:6:1, col:40> col:29 Bar
  |-TemplateTypeParmDecl 0x7fe2b920b100 <col:10, col:19> col:19 referenced typename depth 0 index 0 T
  `-ConceptSpecializationExpr 0x7fe2b920b268 <col:35, col:40> 'bool' Concept 0x7fe2b920afb0 'Foo'

@llvmbot
Copy link
Collaborator

llvmbot commented Feb 25, 2024

@llvm/issue-subscribers-c-20

Author: Haojian Wu (hokein)

Given the following code, the `FoundDecl` is null in the `Foo`'s AST node (`ConceptReference`).
namespace ns {
template&lt;typename T&gt; concept Foo = true;
}

using ns::Foo;
template&lt;typename T&gt;concept Bar = Foo&lt;T&gt;;  // reference `Foo` points to the underlying of ns::Foo, rather than the `using` decl.

This breaks some clang tools: include-cleaner will report the file of the underlying decl, rather than the one for using decl; clangd's go-to-def on Foo jumps to the underlying decl.

@hokein
Copy link
Collaborator Author

hokein commented Feb 29, 2024

@zyn0217 thanks for the investigation, it makes sense, do you mind sending out a fix?

@zyn0217
Copy link
Contributor

zyn0217 commented Mar 2, 2024

Hmm, this is more complicated than I expected: for the type constraint cases, e.g.

namespace ns {
template<typename T> concept Foo = true;
}

using ns::Foo;

template <Foo T> // <-- Note here
void foo();

The FoundDecl thereof still doesn't point to the UsingShadowDecl that way, seemingly because the annotated token (which points to the UsingShadowDecl) gets desuagared implicitly within TemplateName::getAsTemplateDecl.

TemplateDecl *TemplateName::getAsTemplateDecl() const {
if (Decl *TemplateOrUsing = Storage.dyn_cast<Decl *>()) {
if (UsingShadowDecl *USD = dyn_cast<UsingShadowDecl>(TemplateOrUsing))
return cast<TemplateDecl>(USD->getTargetDecl());
assert(isa<TemplateDecl>(TemplateOrUsing));
return cast<TemplateDecl>(TemplateOrUsing);
}

bool Sema::BuildTypeConstraint(const CXXScopeSpec &SS,
TemplateIdAnnotation *TypeConstr,
TemplateTypeParmDecl *ConstrainedParameter,
SourceLocation EllipsisLoc,
bool AllowUnexpandedPack) {
if (CheckTypeConstraint(TypeConstr))
return true;
TemplateName TN = TypeConstr->Template.get();
ConceptDecl *CD = cast<ConceptDecl>(TN.getAsTemplateDecl());
DeclarationNameInfo ConceptName(DeclarationName(TypeConstr->Name),
TypeConstr->TemplateNameLoc);
TemplateArgumentListInfo TemplateArgs;
if (TypeConstr->LAngleLoc.isValid()) {
TemplateArgs =
makeTemplateArgumentListInfo(*this, *TypeConstr);
if (EllipsisLoc.isInvalid() && !AllowUnexpandedPack) {
for (TemplateArgumentLoc Arg : TemplateArgs.arguments()) {
if (DiagnoseUnexpandedParameterPack(Arg, UPPC_TypeConstraint))
return true;
}
}
}
return AttachTypeConstraint(
SS.isSet() ? SS.getWithLocInContext(Context) : NestedNameSpecifierLoc(),
ConceptName, CD,
TypeConstr->LAngleLoc.isValid() ? &TemplateArgs : nullptr,
ConstrainedParameter, EllipsisLoc);
}

@zyn0217
Copy link
Contributor

zyn0217 commented Mar 2, 2024

The type constraint cases are not that hard to tackle, but constraints on AutoTypes e.g. C auto probably need some extra work.

(I hope I can figure them all out, but it takes some time.)

zyn0217 added a commit that referenced this issue Mar 14, 2024
The `ConceptReference`'s `FoundDecl` claims it "can differ from
`NamedConcept` when, for example, the concept was found through a
`UsingShadowDecl`", but such the contract was not previously respected.

Fixes #82628
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++20 clang:frontend Language frontend issues, e.g. anything involving "Sema" concepts C++20 concepts
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

4 participants