-
Notifications
You must be signed in to change notification settings - Fork 11.8k
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] Add frontend flag to enable support for broken external resugarers #102510
[clang] Add frontend flag to enable support for broken external resugarers #102510
Conversation
…arers There are some external projects that can't rely on our own sugar propagation for templated entities, because they need to resugar types which only exist within their framework, and so are entirely invisible to our internal tooling. This new flag is meant to prevent our transforms from removing any Subst* nodes. For this, this is wired only to template type alias subsititutions. Note that our AST does represent enough information to correctly resugar template type alias, so any users of this are either defective or working under extremely limited scenarios.
@llvm/pr-subscribers-clang Author: Matheus Izvekov (mizvekov) ChangesThere are some external projects that can't rely on our own sugar propagation for templated entities, because they need to resugar types which only exist within their framework, and so are entirely invisible to our internal tooling. This new flag is meant to prevent our transforms from removing any Subst* nodes. For this patch, it's wired only to template type alias substitutions. Note that our AST does represent enough information to correctly resugar template type alias, so any users of this are either defective or working under limited scenarios. See discussion at #101858 for motivation. Full diff: https://github.com/llvm/llvm-project/pull/102510.diff 4 Files Affected:
diff --git a/clang/include/clang/Basic/LangOptions.def b/clang/include/clang/Basic/LangOptions.def
index 54e689a7a42213..a4b323a922a6da 100644
--- a/clang/include/clang/Basic/LangOptions.def
+++ b/clang/include/clang/Basic/LangOptions.def
@@ -162,6 +162,7 @@ LANGOPT(CoroAlignedAllocation, 1, 0, "prefer Aligned Allocation according to P20
LANGOPT(DllExportInlines , 1, 1, "dllexported classes dllexport inline methods")
LANGOPT(RelaxedTemplateTemplateArgs, 1, 1, "C++17 relaxed matching of template template arguments")
LANGOPT(ExperimentalLibrary, 1, 0, "enable unstable and experimental library features")
+LANGOPT(SupportBrokenExternalResugarers, 1, 0, "Suppress transforms which would impede broken external resugarers")
LANGOPT(PointerAuthIntrinsics, 1, 0, "pointer authentication intrinsics")
LANGOPT(PointerAuthCalls , 1, 0, "function pointer authentication")
diff --git a/clang/include/clang/Driver/Options.td b/clang/include/clang/Driver/Options.td
index e196c3dc5cb3be..551f975f2b15b3 100644
--- a/clang/include/clang/Driver/Options.td
+++ b/clang/include/clang/Driver/Options.td
@@ -3449,6 +3449,11 @@ defm relaxed_template_template_args : BoolFOption<"relaxed-template-template-arg
PosFlag<SetTrue, [], [], "Enable">,
NegFlag<SetFalse, [], [CC1Option], "Disable">,
BothFlags<[], [ClangOption], " C++17 relaxed template template argument matching">>;
+defm support_broken_external_resugarers : BoolFOption<"support-broken-external-resugarers",
+ LangOpts<"SupportBrokenExternalResugarers">, DefaultFalse,
+ PosFlag<SetTrue, [], [CC1Option], "Enable">,
+ NegFlag<SetFalse, [], [], "Disable">,
+ BothFlags<[], [], " support for broken external resugarers">>;
defm sized_deallocation : BoolFOption<"sized-deallocation",
LangOpts<"SizedDeallocation">, Default<cpp14.KeyPath>,
PosFlag<SetTrue, [], [], "Enable C++14 sized global deallocation functions">,
diff --git a/clang/lib/Sema/SemaTemplate.cpp b/clang/lib/Sema/SemaTemplate.cpp
index cd3ee31fcca610..7224d7cd892f30 100644
--- a/clang/lib/Sema/SemaTemplate.cpp
+++ b/clang/lib/Sema/SemaTemplate.cpp
@@ -3333,9 +3333,16 @@ QualType Sema::CheckTemplateIdType(TemplateName Name,
return QualType();
// Only substitute for the innermost template argument list.
+ // NOTE: Some external resugarers rely on leaving a Subst* node here.
+ // Make the substituion non-final in that case.
+ // Note that these external resugarers are essentially broken
+ // for depending on this, because we don't provide
+ // enough context in the Subst* nodes in order
+ // to tell different template type alias specializations apart.
MultiLevelTemplateArgumentList TemplateArgLists;
- TemplateArgLists.addOuterTemplateArguments(Template, SugaredConverted,
- /*Final=*/true);
+ TemplateArgLists.addOuterTemplateArguments(
+ Template, SugaredConverted,
+ /*Final=*/!getLangOpts().SupportBrokenExternalResugarers);
TemplateArgLists.addOuterRetainedLevels(
AliasTemplate->getTemplateParameters()->getDepth());
diff --git a/clang/test/AST/ast-dump-support-broken-external-resugarers.cpp b/clang/test/AST/ast-dump-support-broken-external-resugarers.cpp
new file mode 100644
index 00000000000000..2f2e4ce16bcdd1
--- /dev/null
+++ b/clang/test/AST/ast-dump-support-broken-external-resugarers.cpp
@@ -0,0 +1,18 @@
+// RUN: %clang_cc1 -fsyntax-only -fsupport-broken-external-resugarers -ast-dump -ast-dump-filter=dump %s | FileCheck -strict-whitespace %s
+
+namespace t1 {
+template<class T> using X = T;
+using dump = X<int>;
+
+// CHECK-LABEL: Dumping t1::dump:
+// CHECK-NEXT: TypeAliasDecl
+// CHECK-NEXT: `-ElaboratedType
+// CHECK-NEXT: `-TemplateSpecializationType
+// CHECK-NEXT: |-name: 'X':'t1::X' qualified
+// CHECK-NEXT: | `-TypeAliasTemplateDecl
+// CHECK-NEXT: |-TemplateArgument
+// CHECK-NEXT: | `-BuiltinType {{.+}} 'int'
+// CHECK-NEXT: `-SubstTemplateTypeParmType 0x{{[0-9]+}} 'int' sugar class depth 0 index 0 T
+// CHECK-NEXT: |-TypeAliasTemplate {{.+}} 'X'
+// CHECK-NEXT: `-BuiltinType {{.+}} 'int'
+} // namespace t1
|
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.
Thank you! This change fixes and unblocks our nullability and lifetime tooling. Please merge at your convenience.
The precommit CI failures are related to your changes and need to be addressed. |
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 using "broken" in the name of the diagnostic and the internal option is a bit problematic -- that's too much of a value judgement, IMO. After all, the previous functionality worked well enough for those folks.
Would it make more sense to name it something along the lines of "retain substitution ast nodes"?
Gentle ping. It's critical that this land so we can unbreak our system. Thanks! |
I've posted a fix as #103219 |
…arers (#103219) Forked from #102510 by [mizvekov](https://github.com/mizvekov). Changes are captured as a fixup commit. There are some external projects that can't rely on our own sugar propagation for templated entities, because they need to resugar types which only exist within their framework, and so are entirely invisible to our internal tooling. This new flag is meant to prevent our transforms from removing any Subst* nodes. For this, this is wired only to template type alias subsititutions. Note that our AST does represent enough information to correctly resugar template type alias, so any users of this are limited in their capacity to reconstruct the parameter substitutions fully. --------- Co-authored-by: Matheus Izvekov <mizvekov@gmail.com>
Thanks. My intention was mostly to discourage users from trying it, without at least looking sideways. Though I am fine with the name change. Thanks for fixing the commit up. |
Closed as completed elsewhere. |
…arers (llvm#103219) Forked from llvm#102510 by [mizvekov](https://github.com/mizvekov). Changes are captured as a fixup commit. There are some external projects that can't rely on our own sugar propagation for templated entities, because they need to resugar types which only exist within their framework, and so are entirely invisible to our internal tooling. This new flag is meant to prevent our transforms from removing any Subst* nodes. For this, this is wired only to template type alias subsititutions. Note that our AST does represent enough information to correctly resugar template type alias, so any users of this are limited in their capacity to reconstruct the parameter substitutions fully. --------- Co-authored-by: Matheus Izvekov <mizvekov@gmail.com>
There are some external projects that can't rely on our own sugar propagation for templated entities, because they need to resugar types which only exist within their framework, and so are entirely invisible to our internal tooling.
This new flag is meant to prevent our transforms from removing any Subst* nodes.
For this patch, it's wired only to template type alias substitutions.
Note that our AST does not represent enough information to correctly resugar template type alias, so any users of this are either defective or working under limited scenarios.
See discussion at #101858 for motivation.