On Fri, Jul 26, 2024 at 01:17:57PM -0400, Jason Merrill wrote:
> On 7/26/24 12:52 AM, Nathaniel Shead wrote:
> > On Tue, Jul 23, 2024 at 04:17:22PM -0400, Jason Merrill wrote:
> > > On 6/15/24 10:29 PM, Nathaniel Shead wrote:
> > > > Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk?
> > > >
> > > > This probably isn't the most efficient approach, since we need to do
> > > > name lookup to find deduction guides for a type which will also
> > > > potentially do a bunch of pointless lazy loading from imported modules,
> > > > but I wasn't able to work out a better approach without completely
> > > > reworking how deduction guides are stored and represented.
> > >
> > > Indeed. We likely want to find them more directly from the template; it's
> > > not clear to me that DECL_INITIAL is used for TEMPLATE_DECL, or we could
> > > put
> > > them in an internal attribute or a separate hash table.
> > >
> > > > -- >8 --
> > > >
> > > > Deduction guides are represented as 'normal' functions currently, and
> > > > have no special handling in modules. However, this causes some issues;
> > > > by [temp.deduct.guide] a deduction guide is not found by normal name
> > > > lookup and instead all reachable deduction guides for a class template
> > > > should be considered, but this does not happen currently.
> > > >
> > > > To solve this, this patch ensures that all deduction guides are
> > > > considered exported to ensure that they are always visible to importers
> > > > if they are reachable. Another alternative here would be to add a new
> > > > kind of "all reachable" flag to name lookup, but that is complicated by
> > > > some difficulties in handling GM entities; this may be a better way to
> > > > go if more kinds of entities end up needing this handling, however.
> > > >
> > > > Another issue here is that because deduction guides are "unrelated"
> > > > functions, they will usually get discarded from the GMF, so this patch
> > > > ensures that when finding dependencies, GMF deduction guides will also
> > > > have bindings created. We do this in find_dependencies so that we don't
> > > > unnecessarily create bindings for GMF deduction guides that are never
> > > > reached; for consistency we do this for *all* deduction guides, not just
> > > > GM ones.
> > >
> > > If you fixed the dependency calculation, why do they also need to be
> > > exported?
> >
> > Deduction guides aren't found using normal name lookup, but any
> > reachable deduction guide must be considered. This means that even if
> > the module interface exports no declarations whatsoever, a deduction
> > guide declared in the module purview must still be considered by
> > importers.
>
> Ah, I was missing the name lookup issue.
>
> > The other option I've considered is adding a new "ANY_REACHABLE" flag to
> > name lookup which would also consider non-exported reachable decls. On
> > further consideration I might actually go this way; I've been thinking
> > about how to resolve some issues adjacent to supporting textual
> > redefinitions that I believe this will be necessary for anyway, and we
> > can probably use this in tsubst_friend_class as well rather than the
> > current relatively ad-hoc solution.
>
> There's also my hack in lookup_elaborated_type for ABI namespace types.
>
> I'm not sure that it should be necessary to do this for redefinitions,
> though; what's the advantage over merging in check_module_override (apart
> from that needing to be fixed)?
>
My thought had been because currently type redefinitions don't go
through 'check_module_override' at all, if the existing type name is
visible, and so I was thinking to fix it at that end instead. This was
also to fix things that aren't strictly redefinitions but also fail
currently, e.g.
module;
template <typename T> struct S;
export module M;
export S<int> foo(); // declares a specialisation of S
// ...
import M;
template <typename T> struct S {}; // definition of template
int main() {
foo(); // error: invalid use of incomplete type, because
// we don't ever go through redeclare_class_template in xref_tag
}
But on further thought maybe it would be more consistent to ensure that
check_module_override handles this, since I suppose we still need to
handle things like the following anyway:
export module M;
struct S {}; // not exported
// ...
import M;
template <typename T> struct S {};
Though the names correspond ([basic.scope.scope] p4), they don't declare
the same entity ([basic.link] p8) since they have different attachment,
and they don't potentially conflict ([basic.scope.scope] p6) because S@M
doesn't precede the template decl because it's not visible to name
lookup. So an "any reachable" flag would be incorrect here regardless.
(Personally, I now even wonder whether it'd be make sense to be more
consistent here, by type declarations always being a new type that is
then merged with an existing one where necessary, rather than xref_tag
always returning an existing type if it finds one, which would help with
compiler errors in the presence of using-decls and help avoid the
duplication of checks for redefining existing names spread across
various places in parser.cc, decl.cc, and name-lookup.cc?)
> > That said, I've realised that this patch isn't completely sufficient
> > anyway; consider:
> >
> > // m.cpp
> > module;
> > template <typename T> struct S;
> > export module M;
> > S(int) -> S<double>;
> >
> > // x.cpp
> > template <typename T> struct S { S(int); };
> > import M;
> > int main() {
> > S s(10); // should be S<double> s;
> > }
> >
> > This patch doesn't correctly handle this case yet, we need to also
> > consider cases where only the deduction guide is in purview.
>
> Indeed.
>
> Jason
>
