-
Notifications
You must be signed in to change notification settings - Fork 11.9k
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
[Clang][Parser] Build up QualifiedTemplateName for typo correction #108148
Conversation
@llvm/pr-subscribers-clang Author: Younan Zhang (zyn0217) ChangesSince #93433, we have switched to No release note because I think we should backport it. Fixes #107887 Full diff: https://github.com/llvm/llvm-project/pull/108148.diff 2 Files Affected:
diff --git a/clang/lib/Sema/SemaTemplate.cpp b/clang/lib/Sema/SemaTemplate.cpp
index 513f83146fb59e..e58cd7959b9d77 100644
--- a/clang/lib/Sema/SemaTemplate.cpp
+++ b/clang/lib/Sema/SemaTemplate.cpp
@@ -3567,7 +3567,10 @@ bool Sema::resolveAssumedTemplateNameAsType(Scope *S, TemplateName &Name,
if (Corrected && Corrected.getFoundDecl()) {
diagnoseTypo(Corrected, PDiag(diag::err_no_template_suggest)
<< ATN->getDeclName());
- Name = TemplateName(Corrected.getCorrectionDeclAs<TemplateDecl>());
+ Name = Context.getQualifiedTemplateName(
+ /*NNS=*/nullptr, /*TemplateKeyword=*/false,
+ TemplateName(
+ TemplateName(Corrected.getCorrectionDeclAs<TemplateDecl>())));
return false;
}
diff --git a/clang/test/Parser/cxx1z-class-template-argument-deduction.cpp b/clang/test/Parser/cxx1z-class-template-argument-deduction.cpp
index 2dd61baac31b3c..a1594333abae73 100644
--- a/clang/test/Parser/cxx1z-class-template-argument-deduction.cpp
+++ b/clang/test/Parser/cxx1z-class-template-argument-deduction.cpp
@@ -255,3 +255,15 @@ void f() {
GH57495::vector.d; // expected-error {{cannot use dot operator on a type}}
}
}
+
+namespace GH107887 {
+
+namespace a {
+template <class> struct pair; // expected-note 3{{declared here}}
+}
+template <class T2> pair() -> pair<T2>; // expected-error 2{{no template named 'pair'}} \
+ // expected-error {{deduction guide must be declared in the same scope}} \
+ // expected-error {{cannot be deduced}} \
+ // expected-note {{non-deducible template parameter 'T2'}}
+
+}
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it makes sense but
- I'd like @mizvekov to get a change to look at it
- Even if I think the change is fairly low risk, I'm not sure a crash-on-invalid is critical enough to backport (we are at ~1 week of the release)
I added more people for additional feedback
I understand the point, but my thought is that we'd want as less regressions as possible; OTOH, the crash-on-invalid could also affect clangd, when the user happens to be typing something invalid. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this change is sensible/innocuous enough. While the clangd justification is somewhat motivating, it would depend on where we want to backport it to.
it seems to me that a x.0.0 release would probably be acceptable for backporting, anything else I'd probably not be willing over a crash-on-invalid.
Given how late in the cycle it is, I would say we shouldn't backport now but if we do another point release of 19.x, then we should backport then. |
Offline, Aaron and I discussed the changes this release cycle, and I think I agree with this. We're too close to the current release, but as soon as the gates are open for the next point-release, I'd be fine with this. |
Thank you folks for the opinions about the backporting. Then I'll move forward with an additional release note and target it to Clang 20 - it's possible that I forget to issue the backport a couple of weeks later :P |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Name = Context.getQualifiedTemplateName( | ||
/*NNS=*/nullptr, /*TemplateKeyword=*/false, | ||
TemplateName(Corrected.getCorrectionDeclAs<TemplateDecl>())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would probably make sense to compute a nested name specifier here, but it doesn't need to happen in this patch.
Since #93433, we have switched to
QualifiedTemplateName
s in more situations to preserve sugars in diagnostics. However, there is one missed case in typo correction that might not meet the expectation inCheckDeductionGuideDeclarator()
.Fixes #107887