[PATCH] D113545: [C++20] [Module] Support reachable definition initially/partially
Richard Smith - zygoloid via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Tue Jun 28 13:51:45 PDT 2022
rsmith accepted this revision.
rsmith added a comment.
This revision is now accepted and ready to land.
A few comments, but they're all minor things or FIXMEs. I'm happy for this to land once they're addressed.
================
Comment at: clang/include/clang/AST/DeclBase.h:229
+ /// This declaration has an owning module, and is visible to lookups
+ /// that occurs within that module. And it is reachable to other module
+ /// when the owning module is transitively imported.
----------------
================
Comment at: clang/include/clang/Sema/Lookup.h:360-361
+ /// Determine whether this lookup is permitted to see the declaration.
+ /// Note that the reachable but not visible declaration inhabit at namespace
+ /// is not allowed to be seen during name lookup.
+ ///
----------------
================
Comment at: clang/include/clang/Sema/Lookup.h:373-374
+ /// // Not valid. We couldn't see reachable here.
+ /// // So isAvailableForLookup would return false when we looks
+ /// 'reachable' here.
+ /// // return reachable(43).v;
----------------
================
Comment at: clang/include/clang/Sema/Lookup.h:377
+ /// // Valid. The field name 'v' is allowed during name lookup.
+ /// // So isAvailableForLookup would return true when we looks 'v' here.
+ /// return func().v;
----------------
================
Comment at: clang/lib/Sema/SemaLookup.cpp:1634-1658
bool Sema::hasVisibleDefaultArgument(const NamedDecl *D,
llvm::SmallVectorImpl<Module *> *Modules) {
if (auto *P = dyn_cast<TemplateTypeParmDecl>(D))
- return ::hasVisibleDefaultArgument(*this, P, Modules);
+ return ::hasAcceptableDefaultArgument(*this, P, Modules,
+ Sema::AcceptableKind::Visible);
if (auto *P = dyn_cast<NonTypeTemplateParmDecl>(D))
+ return ::hasAcceptableDefaultArgument(*this, P, Modules,
----------------
Consider adding a `Sema::hasAcceptableDefaultArgument` to remove the duplicated dispatch here.
================
Comment at: clang/lib/Sema/SemaLookup.cpp:1796
// set of declarations for tags in prototype scope.
- bool VisibleWithinParent;
+ bool AcceptableWithParent;
if (D->isTemplateParameter()) {
----------------
Please rename `With` back to `Within`.
================
Comment at: clang/lib/Sema/SemaLookup.cpp:1895
-bool Sema::isVisibleSlow(const NamedDecl *D) {
- return LookupResult::isVisible(*this, const_cast<NamedDecl*>(D));
+bool LookupResult::isReachableSlow(Sema &SemaRef, NamedDecl *D) {
+ assert(!isVisible(SemaRef, D) && "Shouldn't call the slow case.\n");
----------------
This function is assuming that any declaration in the `ASTContext` is in a translation unit on which we have an interface dependency. That's not going to be true in general (for example, using `-fmodule-file` you can load AST files on which there is no interface dependency, and clang-as-a-library users might even build an `ASTContext` containing the entire program), but seems good enough for now. Please add a FIXME (eg: "FIXME: Return false if we don't have an interface dependency on the translation unit containing D.").
================
Comment at: clang/lib/Sema/SemaLookup.cpp:1898-1907
+ // Filter the use other than C++20 Considering the case we are using clang
+ // module + -std=c++20 combination.
+ if (llvm::any_of(D->redecls(), [](Decl *D) {
+ return D->getOwningModule() &&
+ D->getOwningModule()->isModuleMapModule();
+ }))
+ return false;
----------------
The `any_of` here doesn't seem right to me for two reasons:
1) The rest of this function is answering the question, "is this particular declaration reachable?", not "is any declaration of this entity reachable?"
2) An entity should be reachable if any declaration of it is. Adding a declaration in a module map module should not cause other declarations to become less reachable.
================
Comment at: clang/lib/Sema/SemaLookup.cpp:1926
+ // fragment.
+ if (D->isModulePrivate() || D->isDiscardedInGlobalModuleFragment())
+ return false;
----------------
I don't think we should need the second check here: a declaration that's discarded in the GMF should be module-private, per your comment change on `ModueOwnershipKind`.
================
Comment at: clang/lib/Sema/SemaLookup.cpp:1947
+ // DeclModule if it isn't (transitively) imported.
+ if (DeclModule->getTopLevelModule()->isModuleInterfaceUnit())
+ return true;
----------------
ChuanqiXu wrote:
> iains wrote:
> > ChuanqiXu wrote:
> > > iains wrote:
> > > > ChuanqiXu wrote:
> > > > > iains wrote:
> > > > > > I think isModuleInterfaceUnit needs to include implementation partition units, since they also have a BMI (is `isInterfaceOrPartition()` the right thing to use here?
> > > > > I think we couldn't use `isInterfaceOrPartition()` here. The call for `isModuleInterfaceUnit()` here is sufficient. For example, we could find the example at [[ https://eel.is/c++draft/module.reach#example-1 | [module.reach]example 1 ]], here in Translation unit #5:, the definition of struct B is not reachable since the definition lives in an implementation unit. (We could make it valid by making all additional TU as reachable)
> > > > >
> > > > > Also the module interface unit shouldn't include implementation partition unit according to the wording: [[ https://eel.is/c++draft/module.unit#2 | [module.unit]p2 ]]. I agree that it is confusing since implementation partition unit is importable. But this is not our fault.
> > > >
> > > > OK, perhaps I am missing something - just to clarify,...
> > > >
> > > > In this (which I believe is legal like [module.unit] ex 1 TU 4.
> > > > ```
> > > > module;
> > > > ....
> > > > module Main;
> > > >
> > > > import :ImplUnit; // this has a BMI which could contain reachable definitions, right?
> > > >
> > > > ...
> > > > ```
> > > >
> > > >
> > > >
> > > >
> > > Yeah, I believe this is legal according to [module.reach]p1:
> > > > A translation unit U is necessarily reachable from a point P if U is a module interface unit on which the translation unit containing P has an interface dependency, **or the translation unit containing P imports U**, in either case prior to P.
> > >
> > > Since module Main imports `:ImplUnit` directly, the `:ImplUnit` TU is necessarily reachable.
> > (sorry for multiple iterations - I am trying to see if I missed some point ... )
> >
> > ... it seems to me that in valid code `:ImplUnit` can have `Kind =`
> > `ModulePartitionInterface`
> > or
> > `ModulePartitionImplementation`
> >
> > the second is the special case of an implementation that provides a BMI also.
> >
> >
> Yes, this confused me for a relative long time. It is really confusing that module partition implementation unit is importable but not module interface unit. The standard often emphasize module interface unit. From my implementation and using experience, the implementor and user could treat module partition implementation unit as an implementation partition unit if we treat all additional implementation TU as reachable. (This is what we do internally)
It looks like this change is treating implementation partitions as being reachable only within the same module (see a few lines above: we say anything in the same module is reachable). That's the desirable behavior -- the only reason the standard allows implementation partitions to be reachable anywhere else is because the desirable behavior is harder to implement. So I'm happy with the current approach.
================
Comment at: clang/lib/Sema/SemaLookup.cpp:2000-2004
+ // Class and enumeration member names can be found by name lookup in any
+ // context in which a definition of the type is reachable.
+ if (auto *ECD = dyn_cast<EnumConstantDecl>(ND))
+ return getSema().hasReachableDeclaration(
+ cast<NamedDecl>(ECD->getDeclContext()));
----------------
ChuanqiXu wrote:
> rsmith wrote:
> > I don't think this is quite right. Given:
> >
> > ```
> > export module M {
> > export enum E1 { e1 };
> > enum E2 { e2 };
> > export using E2U = E2;
> > enum E3 { e3 };
> > export E3 f();
> > ```
> >
> > ... the intent is:
> >
> > ```
> > import M;
> > int main() {
> > auto a = E1::e1; // OK, namespace-scope name E1 is visible and e1 is reachable
> > auto b = e1; // OK, namespace-scope name e1 is visible
> > auto c = E2::e2; // error, namespace-scope name E2 is not visible
> > auto d = e2; // error, namespace-scope name e2 is not visible
> > auto e = E2U::e2; // OK, namespace-scope name E2U is visible and E2::e2 is reachable
> > auto f = E3::e3; // error, namespace-scope name E3 is not visible
> > auto g = e3; // error, namespace-scope name e3 is not visible
> > auto h = decltype(f())::e3; // OK, namespace-scope name f is visible and E3::e3 is reachable
> > }
> > ```
> >
> > Instead of doing the check in this function, I think we need to consider the scope in which we're doing a lookup: if that scope is a class or enumeration scope, and the class or enumeration has a reachable definition, then we don't do any visibility checks for its members.
> Oh, your example makes sense. The current implementation would accept `d` and `g` unexpectedly. I spent some time to look into this one. And I find it is not so easy to fix. I left a FIXME below and I would file an issue if this patch landed. Do you think this is OK?
>
> BTW, I feel like the wording of spec need to be adjusted too. From the wording, I feel like `d` and `g` should be accepted.
Yes, I'm fine with having FIXMEs for things that don't work yet -- so long as we don't regress things (particularly, so long as we don't regress module map module support, which some of our users are heavily relying on).
I agree that the wording here is not capturing the rules properly; I've mailed the CWG reflector and it sounds like Davis is going to fix that :)
================
Comment at: clang/lib/Sema/SemaLookup.cpp:3754
+ // reachable definition in the set of associated entities,
+ if (AssociatedClasses.count(RD) && hasReachableDeclaration(D)) {
Visible = true;
----------------
ChuanqiXu wrote:
> rsmith wrote:
> > We should look for a reachable definition here, not a reachable declaration.
> Yeah, from the wording it should be reachable definition. But it would cause many failures if I write hasReachableDefinition/hasVisibleDefinition here. So it looks like a legacy defect. I chose to follow the original style here.
Hm, this might actually be right as-is: because we don't merge lookup results in the decl context lookup set if they come from different modules, we'll consider a friend once for each module that contains a friend declaration, and so we'll consider each class definition that contains a friend declaration here.
I think there's still something subtly wrong here: if there's a merged definition of `D` that is reachable, then the friend declaration should be considered. If you don't want to handle that here (it might be hard to come up with a testcase), please add a FIXME.
================
Comment at: clang/lib/Serialization/ASTReaderDecl.cpp:626-627
+
+ default:
+ llvm_unreachable("Unknown ModuleOwnership kind");
+ }
----------------
Please don't include a `default` here. We want the compiler to tell us if a new enumerator is added and it isn't handled here.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D113545/new/
https://reviews.llvm.org/D113545
More information about the cfe-commits
mailing list