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

[C++20] [Modules] The lambda in the requires expression prevents us to merge concepts across modules #63544

Closed
ChuanqiXu9 opened this issue Jun 27, 2023 · 2 comments
Assignees
Labels
clang:modules C++20 modules and Clang Header Modules

Comments

@ChuanqiXu9
Copy link
Member

ChuanqiXu9 commented Jun 27, 2023

This comes from #63530. I file this to track the issue in the compiler side.

Reproducer:

// RUN: rm -rf %t
// RUN: mkdir -p %t
// RUN: split-file %s %t
//
// RUN: %clang_cc1 -std=c++23 %t/a.cppm -emit-module-interface -o %t/m-a.pcm
// RUN: %clang_cc1 -std=c++23 %t/b.cppm -emit-module-interface -o %t/m-b.pcm
// RUN: %clang_cc1 -std=c++23 %t/m.cppm -emit-module-interface -o %t/m.pcm \
// RUN:     -fprebuilt-module-path=%t
// RUN: %clang_cc1 -std=c++23 %t/use.cpp -fprebuilt-module-path=%t -fsyntax-only -verify

//--- foo.h

namespace std {
struct strong_ordering {
  int n;
  constexpr operator int() const { return n; }
  static const strong_ordering equal, greater, less;
};
constexpr strong_ordering strong_ordering::equal = {0};
constexpr strong_ordering strong_ordering::greater = {1};
constexpr strong_ordering strong_ordering::less = {-1};
} // namespace std

namespace std {
template <typename _Tp>
class optional {
private:
    using value_type = _Tp;
    value_type __val_;
    bool __engaged_;
public:
    constexpr bool has_value() const noexcept
    {
        return this->__engaged_;
    }

    constexpr const value_type& operator*() const& noexcept
    {
        return __val_;
    }

    optional(_Tp v) : __val_(v) {
        __engaged_ = true;
    }
};

template <class _Tp>
concept __is_derived_from_optional = requires(const _Tp& __t) { []<class __Up>(const optional<__Up>&) {}(__t); };

template <class _Tp, class _Up>
    requires(!__is_derived_from_optional<_Up>)
constexpr strong_ordering
operator<=>(const optional<_Tp>& __x, const _Up& __v) {
    return __x.has_value() ? *__x <=> __v : strong_ordering::less;
}
} // namespace std

//--- a.cppm
module;
#include "foo.h"
export module m:a;
export namespace std {
    using std::optional;
    using std::operator<=>;
}

//--- b.cppm
module;
#include "foo.h"
export module m:b;
export namespace std {
    using std::optional;
    using std::operator<=>;
}

//--- m.cppm
export module m;
export import :a;
export import :b;

//--- use.cpp
// expected-no-diagnostics
import m;
int use() {
    std::optional<int> a(43);
    int t{3};
    return a<=>t;
}

The root cause is that we treat the lambda expressions in __is_derived_from_optional as different expressions in module m:a and m:b. This may be related to #57222

@ChuanqiXu9 ChuanqiXu9 added the clang:modules C++20 modules and Clang Header Modules label Jun 27, 2023
@ChuanqiXu9 ChuanqiXu9 self-assigned this Jun 27, 2023
@llvmbot
Copy link
Member

llvmbot commented Jun 27, 2023

@llvm/issue-subscribers-clang-modules

@ChuanqiXu9
Copy link
Member Author

veselypeta pushed a commit to veselypeta/cherillvm that referenced this issue Sep 4, 2024
…tionally

Close llvm/llvm-project#63544.

Background: We landed std modules in libcxx recently but we haven't
landed the corresponding in-tree tests. According to @mordante, there
are only 1% libcxx tests failing with std modules. And the major
blocking issue is the lambda expression in the require clauses.

The root cause of the issue is that previously we never consider any
lambda expression as the same. Per [temp.over.link]p5:
> Two lambda-expressions are never considered equivalent.

I thought this is an oversight at first but @rsmith explains that in the
wording, the program is as if there is only a single definition, and a
single lambda-expression. So we don't need worry about this in the spec.
The explanation makes sense. But it didn't reflect to the implementation
directly.

Here is a cycle in the implementation. If we want to merge two
definitions, we need to make sure its implementation are the same. But
according to the explanation above, we need to judge if two
lambda-expression are the same by looking at its parent definitions. So
here is the problem.

To solve the problem, I think we have to profile the lambda expressions
actually to get the accurate information. But we can't do this
universally. So in this patch I tried to modify the interface of
`Stmt::Profile` and only profile the lambda expression during the
process of merging the constraint expressions.

Differential Revision: https://reviews.llvm.org/D153957
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:modules C++20 modules and Clang Header Modules
Projects
None yet
Development

No branches or pull requests

2 participants