[clang] concepts: perform parameter mapping substitution in correct context #101745
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
llvmbot
added
clang
|
@llvm/pr-subscribers-clang Author: Matheus Izvekov (mizvekov) ChangesPrior to this patch, during constraint normalization we could forget from which declaration an atomic constraint was normalized from. Subsequently when performing parameter mapping substitution for that atomic constraint with an incorrect context, we couldn't correctly recognize which declarations are supposed to be visible. Fixes #60336 Full diff: https://github.com/llvm/llvm-project/pull/101745.diff 5 Files Affected:
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 25f5bd37bbe94..e8973002b3059 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -173,6 +173,9 @@ Bug Fixes to C++ Support
- Clang now correctly parses potentially declarative nested-name-specifiers in pointer-to-member declarators.
- Fix a crash when checking the initialzier of an object that was initialized
with a string literal. (#GH82167)
+- Clang now correctly recognizes the correct context for parameter
+ suibstitutions in concepts, so it doesn't incorrectly complain of missing
+ module imports in those situations. (#GH60336)
Bug Fixes to AST Handling
^^^^^^^^^^^^^^^^^^^^^^^^^
diff --git a/clang/include/clang/Sema/SemaConcept.h b/clang/include/clang/Sema/SemaConcept.h
index 03791962b2dc0..4b1abccb7741a 100644
--- a/clang/include/clang/Sema/SemaConcept.h
+++ b/clang/include/clang/Sema/SemaConcept.h
@@ -30,10 +30,11 @@ enum { ConstraintAlignment = 8 };
struct alignas(ConstraintAlignment) AtomicConstraint {
const Expr *ConstraintExpr;
+ NamedDecl *ConstraintDecl;
std::optional<ArrayRef<TemplateArgumentLoc>> ParameterMapping;
- AtomicConstraint(Sema &S, const Expr *ConstraintExpr) :
- ConstraintExpr(ConstraintExpr) { };
+ AtomicConstraint(const Expr *ConstraintExpr, NamedDecl *ConstraintDecl)
+ : ConstraintExpr(ConstraintExpr), ConstraintDecl(ConstraintDecl) {};
bool hasMatchingParameterMapping(ASTContext &C,
const AtomicConstraint &Other) const {
diff --git a/clang/lib/Sema/SemaConcept.cpp b/clang/lib/Sema/SemaConcept.cpp
index 9e16b67284be4..7d7a94e9fd637 100644
--- a/clang/lib/Sema/SemaConcept.cpp
+++ b/clang/lib/Sema/SemaConcept.cpp
@@ -1457,8 +1457,8 @@ substituteParameterMappings(Sema &S, NormalizedConstraint &N,
: ArgsAsWritten->arguments().front().getSourceRange().getEnd();
Sema::InstantiatingTemplate Inst(
S, InstLocBegin,
- Sema::InstantiatingTemplate::ParameterMappingSubstitution{}, Concept,
- {InstLocBegin, InstLocEnd});
+ Sema::InstantiatingTemplate::ParameterMappingSubstitution{},
+ Atomic.ConstraintDecl, {InstLocBegin, InstLocEnd});
if (Inst.isInvalid())
return true;
if (S.SubstTemplateArguments(*Atomic.ParameterMapping, MLTAL, SubstArgs))
@@ -1632,7 +1632,7 @@ NormalizedConstraint::fromConstraintExpr(Sema &S, NamedDecl *D, const Expr *E) {
Kind, std::move(*Sub), FE->getPattern()}};
}
- return NormalizedConstraint{new (S.Context) AtomicConstraint(S, E)};
+ return NormalizedConstraint{new (S.Context) AtomicConstraint(E, D)};
}
bool FoldExpandedConstraint::AreCompatibleForSubsumption(
diff --git a/clang/test/Modules/GH60336-2.cpp b/clang/test/Modules/GH60336-2.cpp
new file mode 100644
index 0000000000000..9740c744b7b7b
--- /dev/null
+++ b/clang/test/Modules/GH60336-2.cpp
@@ -0,0 +1,44 @@
+// RUN: rm -rf %t
+// RUN: %clang_cc1 -x c++ -std=c++20 %s -verify -fmodules -fmodules-cache-path=%t
+// expected-no-diagnostics
+
+#pragma clang module build std
+module std {
+ module concepts {}
+ module functional {}
+}
+#pragma clang module contents
+#pragma clang module begin std
+
+template <class _Tp> struct common_reference {
+ using type = _Tp;
+};
+
+#pragma clang module end
+#pragma clang module begin std.concepts
+#pragma clang module import std
+
+template <class _Tp>
+concept same_as = __is_same(_Tp, _Tp);
+
+template <class _Tp>
+concept common_reference_with =
+ same_as<typename common_reference<_Tp>::type>;
+
+#pragma clang module end
+#pragma clang module begin std.functional
+#pragma clang module import std.concepts
+
+template <class, class _Ip>
+concept sentinel_for = common_reference_with<_Ip>;
+
+constexpr bool ntsf_subsumes_sf(sentinel_for<char *> auto)
+ requires true
+{
+ return true;
+}
+bool ntsf_subsumes_sf(sentinel_for<char *> auto);
+static_assert(ntsf_subsumes_sf(""));
+
+#pragma clang module end
+#pragma clang module endbuild
diff --git a/clang/test/Modules/GH60336.cpp b/clang/test/Modules/GH60336.cpp
index fefbd37b7926c..e181c24904217 100644
--- a/clang/test/Modules/GH60336.cpp
+++ b/clang/test/Modules/GH60336.cpp
@@ -1,5 +1,7 @@
// RUN: rm -rf %t
// RUN: %clang_cc1 -x c++ -std=c++20 %s -verify -fmodules -fmodules-cache-path=%t
+// expected-no-diagnostics
+
#pragma clang module build std
module std [system] {
module concepts [system] {
@@ -65,14 +67,3 @@ constexpr bool ntsf_subsumes_sf(std::nothrow_sentinel_for<char*> auto) requires
}
constexpr bool ntsf_subsumes_sf(std::sentinel_for<char*> auto);
static_assert(ntsf_subsumes_sf("foo"));
-
-// Note: Doing diagnostics verify lines in the individual modules isn't
-// permitted, and using 'bookmarks' in a module also doesn't work, so we're
-// forced to diagnose this by line-number.
-//
-// Check to ensure that this error happens, prior to a revert of a concepts
-// sugaring patch, this diagnostic didn't happen correctly.
-
-// expected-error@* {{partial specialization of 'common_reference<_Tp, _Up>' must be imported from module 'std.type_traits' before it is required}}
-// expected-note@63 {{while substituting into concept arguments here}}
-// expected-note@*{{partial specialization declared here is not reachable}}
|
|
@llvm/pr-subscribers-clang-modules Author: Matheus Izvekov (mizvekov) ChangesPrior to this patch, during constraint normalization we could forget from which declaration an atomic constraint was normalized from. Subsequently when performing parameter mapping substitution for that atomic constraint with an incorrect context, we couldn't correctly recognize which declarations are supposed to be visible. Fixes #60336 Full diff: https://github.com/llvm/llvm-project/pull/101745.diff 5 Files Affected:
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 25f5bd37bbe94..e8973002b3059 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -173,6 +173,9 @@ Bug Fixes to C++ Support
- Clang now correctly parses potentially declarative nested-name-specifiers in pointer-to-member declarators.
- Fix a crash when checking the initialzier of an object that was initialized
with a string literal. (#GH82167)
+- Clang now correctly recognizes the correct context for parameter
+ suibstitutions in concepts, so it doesn't incorrectly complain of missing
+ module imports in those situations. (#GH60336)
Bug Fixes to AST Handling
^^^^^^^^^^^^^^^^^^^^^^^^^
diff --git a/clang/include/clang/Sema/SemaConcept.h b/clang/include/clang/Sema/SemaConcept.h
index 03791962b2dc0..4b1abccb7741a 100644
--- a/clang/include/clang/Sema/SemaConcept.h
+++ b/clang/include/clang/Sema/SemaConcept.h
@@ -30,10 +30,11 @@ enum { ConstraintAlignment = 8 };
struct alignas(ConstraintAlignment) AtomicConstraint {
const Expr *ConstraintExpr;
+ NamedDecl *ConstraintDecl;
std::optional<ArrayRef<TemplateArgumentLoc>> ParameterMapping;
- AtomicConstraint(Sema &S, const Expr *ConstraintExpr) :
- ConstraintExpr(ConstraintExpr) { };
+ AtomicConstraint(const Expr *ConstraintExpr, NamedDecl *ConstraintDecl)
+ : ConstraintExpr(ConstraintExpr), ConstraintDecl(ConstraintDecl) {};
bool hasMatchingParameterMapping(ASTContext &C,
const AtomicConstraint &Other) const {
diff --git a/clang/lib/Sema/SemaConcept.cpp b/clang/lib/Sema/SemaConcept.cpp
index 9e16b67284be4..7d7a94e9fd637 100644
--- a/clang/lib/Sema/SemaConcept.cpp
+++ b/clang/lib/Sema/SemaConcept.cpp
@@ -1457,8 +1457,8 @@ substituteParameterMappings(Sema &S, NormalizedConstraint &N,
: ArgsAsWritten->arguments().front().getSourceRange().getEnd();
Sema::InstantiatingTemplate Inst(
S, InstLocBegin,
- Sema::InstantiatingTemplate::ParameterMappingSubstitution{}, Concept,
- {InstLocBegin, InstLocEnd});
+ Sema::InstantiatingTemplate::ParameterMappingSubstitution{},
+ Atomic.ConstraintDecl, {InstLocBegin, InstLocEnd});
if (Inst.isInvalid())
return true;
if (S.SubstTemplateArguments(*Atomic.ParameterMapping, MLTAL, SubstArgs))
@@ -1632,7 +1632,7 @@ NormalizedConstraint::fromConstraintExpr(Sema &S, NamedDecl *D, const Expr *E) {
Kind, std::move(*Sub), FE->getPattern()}};
}
- return NormalizedConstraint{new (S.Context) AtomicConstraint(S, E)};
+ return NormalizedConstraint{new (S.Context) AtomicConstraint(E, D)};
}
bool FoldExpandedConstraint::AreCompatibleForSubsumption(
diff --git a/clang/test/Modules/GH60336-2.cpp b/clang/test/Modules/GH60336-2.cpp
new file mode 100644
index 0000000000000..9740c744b7b7b
--- /dev/null
+++ b/clang/test/Modules/GH60336-2.cpp
@@ -0,0 +1,44 @@
+// RUN: rm -rf %t
+// RUN: %clang_cc1 -x c++ -std=c++20 %s -verify -fmodules -fmodules-cache-path=%t
+// expected-no-diagnostics
+
+#pragma clang module build std
+module std {
+ module concepts {}
+ module functional {}
+}
+#pragma clang module contents
+#pragma clang module begin std
+
+template <class _Tp> struct common_reference {
+ using type = _Tp;
+};
+
+#pragma clang module end
+#pragma clang module begin std.concepts
+#pragma clang module import std
+
+template <class _Tp>
+concept same_as = __is_same(_Tp, _Tp);
+
+template <class _Tp>
+concept common_reference_with =
+ same_as<typename common_reference<_Tp>::type>;
+
+#pragma clang module end
+#pragma clang module begin std.functional
+#pragma clang module import std.concepts
+
+template <class, class _Ip>
+concept sentinel_for = common_reference_with<_Ip>;
+
+constexpr bool ntsf_subsumes_sf(sentinel_for<char *> auto)
+ requires true
+{
+ return true;
+}
+bool ntsf_subsumes_sf(sentinel_for<char *> auto);
+static_assert(ntsf_subsumes_sf(""));
+
+#pragma clang module end
+#pragma clang module endbuild
diff --git a/clang/test/Modules/GH60336.cpp b/clang/test/Modules/GH60336.cpp
index fefbd37b7926c..e181c24904217 100644
--- a/clang/test/Modules/GH60336.cpp
+++ b/clang/test/Modules/GH60336.cpp
@@ -1,5 +1,7 @@
// RUN: rm -rf %t
// RUN: %clang_cc1 -x c++ -std=c++20 %s -verify -fmodules -fmodules-cache-path=%t
+// expected-no-diagnostics
+
#pragma clang module build std
module std [system] {
module concepts [system] {
@@ -65,14 +67,3 @@ constexpr bool ntsf_subsumes_sf(std::nothrow_sentinel_for<char*> auto) requires
}
constexpr bool ntsf_subsumes_sf(std::sentinel_for<char*> auto);
static_assert(ntsf_subsumes_sf("foo"));
-
-// Note: Doing diagnostics verify lines in the individual modules isn't
-// permitted, and using 'bookmarks' in a module also doesn't work, so we're
-// forced to diagnose this by line-number.
-//
-// Check to ensure that this error happens, prior to a revert of a concepts
-// sugaring patch, this diagnostic didn't happen correctly.
-
-// expected-error@* {{partial specialization of 'common_reference<_Tp, _Up>' must be imported from module 'std.type_traits' before it is required}}
-// expected-note@63 {{while substituting into concept arguments here}}
-// expected-note@*{{partial specialization declared here is not reachable}}
|
mizvekov
force-pushed
the
users/mizvekov/clang-fix-GH60336
branch
from
50d8da6 to
5fa1cc6
Compare
mizvekov
changed the title
cor3ntin
approved these changes
Aug 4, 2024
|
I think this is simple enough that we could backport. |
…ntext Prior to this patch, during constraint normalization we could forget from which declaration an atomic constraint was normalized from. Subsequently when performing parameter mapping substitution for that atomic constraint with an incorrect context, we couldn't correctly recognize which declarations are supposed to be visible. Fixes #60336
mizvekov
force-pushed
the
users/mizvekov/clang-fix-GH60336
branch
from
5fa1cc6 to
56c022e
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Prior to this patch, during constraint normalization we could forget from which declaration an atomic constraint was normalized from.
Subsequently when performing parameter mapping substitution for that atomic constraint with an incorrect context, we couldn't correctly recognize which declarations are supposed to be visible.
Fixes #60336