Use qualified name-lookup in module.

[DanielaE]

And allow lookup of non-exported names from local classes in function templates.

Using declarations link these names into the body and make them available for name-lookup when the local classes are instantiated together with the function bodies that contain them.

[vitaut]

This should be unnecessary because we are in the detail namespace.

[DanielaE]

My first impulse as well.
This is similar to #2260 and the outcome the same: the name-lookup of detail::auto_id fails with "auto_id is not a member of detail".

For more on that see below.

[vitaut]

I suggest moving namespace qualification to the only place where auto_id is used below.

[DanielaE]

The visibility of non-exported names used in (not only) function templates, and the lack of it during implicit template instantiation either directly or indirectly triggered by a call from importers of the module is a common theme.

The situation in #2260 is simple. Here in #2324 it's more difficult because in this case there's a dependent local class involved which uses a non-exported name detail::auto_id as a tag parameter in the overload set of some of its member functions.

The million dollar question is: when is this name looked up and bound?

1. At template compilation time (i.e. generation of the BMI) with the result stored in the BMI and made reachable?
2. Or at template instantiation time in client code?
   Either answer might be correct. The name detail::auto_id is non-dependent so it should be resolved at template compilation time (i.e. answer 1). Or P1815R2: Translation-unit-local entities (or something along this spirit) applies and makes the code ill-formed when compiled in a module interface unit (answer 2). The behaviour suggests that msvc's implementation is along answer 2.

There are even worse examples present in the code (format.h:2640):

template <typename Char>
void vformat_to(buffer<Char>& buf, basic_string_view<Char> fmt,
                basic_format_args<buffer_context<type_identity_t<Char>>> args,
                locale_ref loc)

has a dependent local class that requires 12 entities from detail to be visible during instantiation. I need 12 using-declarations in the function body to make this happen.

And there is FMT_STRING_IMPL (format.h:2138) with its lambda and even-more-local nested class in there.

I need to make a deep dive into the standard to figure out what's going on here and if this is the indended outcome. From my conversation with Cameron I get the impression that the observed behaviour is correct according to Microsoft's reading of the standard. Unfortunately I cannot cross-check with f.e. gcc (even though the modules branch is mostly there). May be I need to discuss this further or even bring this to SG2 or Core.

[vitaut]

Thanks for the detailed explanation. So what are the observable effects of not doing the namespace qualification of auto_id? What code breaks and what are the errors?

[DanielaE]

The non-char API overloads use separate code paths and instantiate more/other templates. These break e.g. like this:

[build] FAILED: test/CMakeFiles/module-test.dir/module-test.cc.obj 
[build] cl.exe  /nologo /TP -DGTEST_HAS_STD_WSTRING=1 -D_SILENCE_TR1_NAMESPACE_DEPRECATION_WARNING=1 -I..\include -I..\test\gtest\. /DWIN32 /D_WINDOWS /GR /EHsc /O2 /Ob2 /DNDEBUG -MD /utf-8 /reference fmt=E:/fmt-module/build/test/test-module.ifc -std:c++latest /showIncludes /Fotest\CMakeFiles\module-test.dir\module-test.cc.obj /Fdtest\CMakeFiles\module-test.dir\ /FS -c ..\test\module-test.cc
[build] E:\fmt-module\include\fmt\core.h(2176): error C3861: 'auto_id': identifier not found
[build] E:\fmt-module\include\fmt\core.h(2281): note: see reference to function template instantiation 'const Char *fmt::v7::detail::parse_width<Char,fmt::v7::detail::specs_checker<fmt::v7::detail::specs_handler<Char>>&>(const Char *,const Char *,Handler)' being compiled
[build]         with
[build]         [
[build]             Char=wchar_t,
[build]             Handler=fmt::v7::detail::specs_checker<fmt::v7::detail::specs_handler<wchar_t>> &
[build]         ]
[build] E:\fmt-module\include\fmt\format.h(2702): note: see reference to function template instantiation 'const Char *fmt::v7::detail::parse_format_specs<Char,fmt::v7::detail::specs_checker<fmt::v7::detail::specs_handler<Char>>&>(const Char *,const Char *,SpecHandler)' being compiled
[build]         with
[build]         [
[build]             Char=wchar_t,
[build]             SpecHandler=fmt::v7::detail::specs_checker<fmt::v7::detail::specs_handler<wchar_t>> &
[build]         ]
[build] E:\fmt-module\include\fmt\xchar.h(118): note: see reference to function template instantiation 'void fmt::v7::detail::vformat_to<T>(fmt::v7::detail::buffer<T> &,fmt::v7::basic_string_view<wchar_t>,fmt::v7::basic_format_args<fmt::v7::basic_format_context<std::back_insert_iterator<fmt::v7::detail::buffer<T>>,wchar_t>>,fmt::v7::detail::locale_ref)' being compiled
[build]         with
[build]         [
[build]             T=wchar_t
[build]         ]
[build] ..\test\module-test.cc(133): note: see reference to function template instantiation 'std::back_insert_iterator<fmt::v7::detail::buffer<T>> fmt::v7::format_to<wchar_t[3],int,wchar_t,500,std::allocator<wchar_t>,0>(fmt::v7::basic_memory_buffer<T,500,std::allocator<wchar_t>> &,const S (&),int &&)' being compiled
[build]         with
[build]         [
[build]             T=wchar_t,
[build]             S=wchar_t [3]
[build]         ]
[build] E:\fmt-module\include\fmt\core.h(2206): error C3861: 'auto_id': identifier not found
[build] E:\fmt-module\include\fmt\core.h(2286): note: see reference to function template instantiation 'const Char *fmt::v7::detail::parse_precision<Char,fmt::v7::detail::specs_checker<fmt::v7::detail::specs_handler<Char>>&>(const Char *,const Char *,Handler)' being compiled
[build]         with
[build]         [
[build]             Char=wchar_t,
[build]             Handler=fmt::v7::detail::specs_checker<fmt::v7::detail::specs_handler<wchar_t>> &
[build]         ]

I ran a quick check with an alternative solution: by pulling all of these local dependent classes out of the function body into class templates at namespace scope, then none of the using declarations are required and everything works. This applies to

• core.h detail::parse_width and detail::parse_precision
• format.h detail::vformat_to

In the latter case this saves 12 seemingly unnecessary using declarations. And almost all non-char overloads of the API become available.

For example: pull class width_adapter out of parse_width

namespace detail {
 ...
// pulled out of  'parse_width'
template <typename Char, typename Handler>
struct width_adapter {
  Handler& handler;

  FMT_CONSTEXPR void operator()() { handler.on_dynamic_width(auto_id()); }
  FMT_CONSTEXPR void operator()(int id) { handler.on_dynamic_width(id); }
  FMT_CONSTEXPR void operator()(basic_string_view<Char> id) {
    handler.on_dynamic_width(id);
  }
  FMT_CONSTEXPR void on_error(const char* message) {
    handler.on_error(message);
  }
};

template <typename Char, typename Handler>
FMT_CONSTEXPR auto parse_width(const Char* begin, const Char* end,
                               Handler&& handler) -> const Char* {
  FMT_ASSERT(begin != end, "");
  if ('0' <= *begin && *begin <= '9') {
    handler.on_width(parse_nonnegative_int(begin, end, handler));
  } else if (*begin == '{') {
    ++begin;
    if (begin != end)
      begin = parse_arg_id(begin, end, width_adapter<Char, Handler>{handler});
    if (begin == end || *begin != '}')
      return handler.on_error("invalid format string"), begin;
    ++begin;
  }
  return begin;
}
 ...
} // end namespace detail

I can modify the PR with this alternative approach.

[vitaut]

I can modify the PR with this alternative approach.

No need to, some of these will become lambdas in the future so adding qualification is better.

[vitaut]

same here

[DanielaE]

Namespace qualification doesn't help, that's the crux of the matter: at the point of name-lookup of detail::auto_id, i.e. at template instantiation time while compiling user code consuming the API imported from the module, nothing from namespace detail is visible anymore!

At present, there are two options available:

1. incorporate all affected names from namespace detail into the function bodies by introducing them into the function scope with using declarations and make this scope their declarative region. In effect, this establishes a link to the actual definition and makes the definition necessarily reachable.
2. pull the function-local classes (actually class-templates in disguise) out into namespace scope. This way they become plain vanilla class-templates with 1st-pass name binding of non-dependent names at template compilation time.

[vitaut]

Not a fan of this module "feature", hopefully they'll fix it somehow.

[DanielaE]

Me too! I will discuss this with people more in the know as I said before. I really want Cameron's opinion on that, and hopefully Gaby's too. Let's see if I can make them interested into this discussion here.

[vitaut]

Thank you!