Page MenuHomePhabricator

[Sema] Merge C++20 concept definitions from different modules in same TU
ClosedPublic

Authored by ilya-biryukov on Jun 30 2022, 8:24 AM.

Details

Summary

Currently the C++20 concepts are only merged in ASTReader, i.e. when
coming from different TU. This can causes ambiguious reference errors when
trying to access the same concept that should otherwise be merged.

Please see the added test for an example.

Note that we currently use ASTContext::isSameEntity to check for ODR
violations. However, it will not check that concept requirements match.
The same issue holds for mering concepts from different TUs, I added a
FIXME and filed a GH issue to track this:
https://github.com/llvm/llvm-project/issues/56310

Diff Detail

Event Timeline

ilya-biryukov created this revision.Jun 30 2022, 8:24 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 30 2022, 8:24 AM
ilya-biryukov requested review of this revision.Jun 30 2022, 8:24 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 30 2022, 8:24 AM

I don't see anything to be concerned about here, but I'd like a modules person to take a look. @ChuanqiXu any chance you can confirm here? (or @iains ?)

I don't see anything to be concerned about here, but I'd like a modules person to take a look. @ChuanqiXu any chance you can confirm here? (or @iains ?)

Also, would like to see a release note on this too!

@erichkeane please let me know if you're the right person to review this.

clang/lib/Sema/SemaTemplate.cpp
8714–8715

Not sure if reporting the redefinition if isSameEntity returns false is the right approach here.
This leads to errors on definitions of something like:

// foo.h
template <class T> concept A = false;

// bar.h
template <class T, class U> concept A = true;

#include "foo.h"
#include "bar.h" // <-- redefinition error shown here

The alternative is the "ambiguous reference" error on the use of the concept. I will check what happens for variables and typedefs in that case and follow the same pattern.

  • Update code to match how typedefs behave

I haven't seen your reply before posting my initial comments. Many thanks for a very quick turnaroud on this!
Will wait for module folks to approve.

clang/lib/Sema/SemaTemplate.cpp
8714–8715

Clang tends to do "ambiguous reference" instead of the behavior we had in this patch.
I have updated it to be consistent with what typedefs and other symbols produce.

  • remove leftover test from previous version

Thanks for filing that issue. I wanted to add that myself before. It should be better to change isVisible and hasVisibleDefinition to isReachable and hasReachableDefinition. The definition of reachability comes from C++20 Modules and it shouldn't affect Clang Modules.

And would you like to add an example for C++20 Modules? It should be something like:

// RUN: rm -rf %t
// RUN: mkdir %t
// RUN: split-file %s %t
//
// RUN: %clang_cc1 -std=c++20 -emit-module-interface %t/same_as.cppm -o %t/same_as.pcm
// RUN: %clang_cc1 -std=c++20 -emit-module-interface -fprebuilt-module-path=%t %t/concepts.cppm -o %t/concepts.pcm
// RUN: %clang_cc1 -std=c++20 -emit-module-interface -fprebuilt-module-path=%t %t/format.cppm -o %t/format.pcm
// RUN: %clang_cc1 -std=c++20 -emit-module-interface %t/conflicting.cppm -o %t/conflicting.pcm
// RUN: %clang_cc1 -std=c++20 -fprebuilt-module-path=%t %t/Use.cppm -fsyntax-only -verify

//--- same_as.cppm
export module same_as;
export template <class T, class U>
concept same_as = __is_same(T, U);

//--- concepts.cppm
export module concepts;
export import same_as;

export template <class T>
concept ConflictingConcept = true;

//--- format.cppm

export module format;
export import concepts;
export import same_as;

export template <class T> void foo()
  requires same_as<T, int>
{}

//--- conflicting.cppm
export module conflicting;
export template <class T, class U = int>
concept ConflictingConcept = true;

//--- Use.cppm
import format;
import conflicting;

template <class T> void foo()
  requires same_as<T, T>
{}
ConflictingConcept auto x = 10; // expected-error {{reference to 'ConflictingConcept' is ambiguous}}
                                // expected-note@* 2 {{candidate}}

Also I feel it would be better to rewrite the current tests into the above style by split-file.

clang/lib/Sema/SemaTemplate.cpp
8732
8742
ilya-biryukov marked 2 inline comments as done.
  • Update code to match how typedefs behave
  • remove leftover test from previous version
  • Add test for C++20 modules, rewrite original test with split-file
  • use isReachable, do not call makeMergedDefinitionVisible

Many thanks! I didn't know about split-file, it's much nicer indeed!

Also incorporating feedback from @rsmith and removing the call to makeMergedDefinitionVisible. Keeping just setPrimaryMergedDecl is enough here.
Richard's reply from the email exchange:

I don't think that makeMergedDefinitionVisible is quite right; that's intended to solve a different problem. Specifically, when we have a repeated class definition in a single parse:

// in module X
class A { void f(); };

// in module Y
class A { void f(); };

Here, we first see and parse the class definition of A in module X. That then becomes "the" definition of A, for the purposes of this compilation. Then when we see another definition of the class, we choose to skip it because we already have a definition, and Clang wants there to only be one definition of each class. So we end up behaving as if we parsed this:

// in module X
class A { void f(); };

// in module Y
class A;

... but we still need to track that class A has a complete definition if only Y is imported, so that A::f can be used for example. And that's what makeMergedDefinitionVisible does: it says that A should be considered as having a complete definition when Y is imported, even though the definition actually lives in X.

For concepts I think the situation is different: we're not going to skip the "body" of the concept and treat

// module X
template<typename T> concept bool C = true;

// module Y
template<typename T> concept bool C = true;

as if it were


// module X
template<typename T> concept bool C = true;

// module Y
template<typename T> concept bool C; // forward declaration?

... because there's no such thing as a forward declaration of a concept. Instead, I think we should just be calling ASTContext::setPrimaryMergedDecl to say that the C in module Y is a redeclaration of the C in module X, so that a name lookup that finds both is not ambiguous. It shouldn't matter which one we actually pick, because they are the same.

PS I will make sure to look at the patches you sent my way this week.
Wanted to do it earlier, but have been having some personal emergencies I needed to take care of.

  • Always call PushToScopeChains. This is the right behavior after we stopped calling makeMergedDefinitionVisible
rsmith added inline comments.Jul 6 2022, 2:49 PM
clang/lib/Sema/SemaTemplate.cpp
8732

I don't think that's right: the C++20 modules rule is that at most one definition is permitted per translation unit, and the Clang header modules rule is that a definition is only permissible if no definition is visible (Clang header modules effectively behave as if they're part of the same translation unit for the purposes of this check). For now, checking for a visible definition seems better than checking for a reachable one, until we implement the C++20 rule in general.

8748–8750

It'd be nice to have a third diagnostic for the case where there's a known declaration that doesn't match. That is:

  • If the name is used for something that's not a concept, diagnose with err_redefinition_different_kind.
  • If the name is used for a visible and matching concept, diagnose with err_redefinition.
  • If the name is used for a non-matching concept, regardless of whether it's visible or even reachable, produce some new mismatch error.
  • Otherwise, if the name is used for a matching concept that's not visible, merge.
ChuanqiXu added inline comments.Jul 6 2022, 10:54 PM
clang/lib/Sema/SemaTemplate.cpp
8732

For clang modules, the reachability should be the visibility: https://github.com/llvm/llvm-project/blob/9cb00e7133ea3da6a49ade95e3708240c2aaae39/clang/lib/Sema/SemaLookup.cpp#L1903-L1905. So I think it is better to check for a reachable definition here. Otherwise we might not be able to handle the following case:

import M;
export template <class T>
concept ConflictingConcept = true; // There is another reachable but not visible definition for ConflictingConcept.
ChuanqiXu accepted this revision.Jul 14 2022, 3:22 AM

LGTM if the comments about diagnostic get addressed.

This revision is now accepted and ready to land.Jul 14 2022, 3:22 AM
  • Update diagnostics per Richard's suggestions, add tests
ilya-biryukov marked 3 inline comments as done.Fri, Jul 22, 10:13 AM

Sorry, was out again due to an emergency. So I wanted to give a chance for any last comments despite having an LGTM
I plan to land this on Monday.

clang/lib/Sema/SemaTemplate.cpp
8732

Keeping the reachability for now. Happy to change if we run into trouble later.

8748–8750

Done. PTAL at the wording for the new error.

regardless of whether it's visible or even reachable, produce some new mismatch error.

I have removed the check visibility of conflicting declarations completely.
I assume it does not make sense to distinguish between symbols of the same kind or different kinds as both are ODR violations, right?

There is a corner case where we don't report a conflict now: lookup with >1 result and a matching non-visible concept as a representative decl. I did not find a good error for this case, and there are other symbols that have this problem, e.g. namespaces.

It's not easy to trigger this, but I bet it's possible.

ilya-biryukov marked 2 inline comments as done.Mon, Jul 25, 3:45 AM
This revision was landed with ongoing or failed builds.Mon, Jul 25, 5:53 AM
This revision was automatically updated to reflect the committed changes.