[RFC] [C++20] [Modules] Relax the ODR check for decls in global module fragment

[ChuanqiXu9]

The false positive ODR violation diagnostic for modules have been an annoying problem for a long time. We've received a lot of issue reports about it and I rarely see the reports saying the mechanism find real ODR violations. (a suspicious one may be #61643).

Then in a private chat with MSVC developer, he told me MSVC don't check ODR violations in the global module fragment. This is a surprise to me and I never thought we can proceed by this. I feel this may be a regression at first. But I feel it might make sense due to:

• Like I said before, there are a lot of false positive ODR violation diagnostic while the true positive diagnostics are much less.
• On the other hand, given that the things in the global module fragment are mostly from headers, it won't be a regression compared with the era before modules.
• It is really hard for the compiler to get ODR Hash correct in the GMF due to the complexity of headers and C++. An example may be [C++20] [Modules] False Positive ODR violation diagnostic due to using inconsistent qualified but the same type #78850. While all of us love standard conforming implementation, sometimes it is really hard to get things right.

So I'd like to take the same strategy with MSVC to not check ODR violations in the global module fragment.

Given the fact that this is actually a regression on the standard conformance, I am not sure if people have different opinions.

[llvmbot]

@llvm/issue-subscribers-clang-modules

Author: Chuanqi Xu (ChuanqiXu9)

The false positive ODR violation diagnostic for modules have been an annoying problem for a long time. We've received a lot of issue reports about it and I rarely see the reports saying the mechanism find real ODR violations. (a suspicious one may be https://github.com//issues/61643).

Then in a private chat with MSVC developer, he told me MSVC don't check ODR violations in the global module fragment. This is a surprise to me and I never thought we can proceed by this. I feel this may be a regression at first. But I feel it might make sense due to:

• Like I said before, there are a lot of false positive ODR violation diagnostic while the true positive diagnostics are much less.
• On the other hand, given that the things in the global module fragment are mostly from headers, it won't be a regression compared with the era before modules.
• It is really hard for the compiler to get ODR Hash correct in the GMF due to the complexity of headers and C++. An example may be [C++20] [Modules] False Positive ODR violation diagnostic due to using inconsistent qualified but the same type #78850. While all of us love standard conforming implementation, sometimes it is really to get things right.

So I'd like to take the same strategy with MSVC to not check ODR violations in the global module fragment.

Given the fact that this is actually a regression on the standard conformance, I am not sure if people have different opinions.

[mizvekov]

This is hiding real bugs, I feel like this is going too far for people who are striving for correctness here.

With this change, this behavior is not even controlled by a flag, so it feels like we are putting this under the rug indefinitely.

[cor3ntin]

Putting the behavior behind a flag does sound like a worthy compromise to me.

[dwblaikie]

Is anyone signing up to use that flag today, on a significant codebase on an ongoing basis, today? Otherwise I'd be inclined to leave it out until there's such a user signing up. I'd rather not add it speculatively.

[dwblaikie]

& might be worth discussing adding the flag in a separate issue, or a PR?

[mizvekov]

There are two kinds of bugs / issues relevant here:

1. Clang bugs that this change hides
   Here we can add a Frontend flag that disables the GMF ODR check, just so we can keep tracking, testing and fixing these issues.
   The Driver would just always pass that flag.
   We could add that flag in this current issue.
2. Bugs in user code:
   I don't think it's worth adding a corresponding Driver flag for controlling the above Frontend flag, since we intend it's behavior to become default as we fix the problems, and users interested in testing the more strict behavior can just use the Frontend flag directly.