Fix intrinsic lookup with namespaces

[llvm-beanz]

This change fixes issues with intrinsic lookup caused by not correctly respecting the using declaration(s) that impact unqualified lookups.

This probably isn't a perfect solution because I'm sure there's some nuance of unqualified lookups in C++ that I'm not handling, but this does respect scoped using directives and allows us to get things working.

Additionally this change disables emitting some "declared here" notes when the source location referred to is invalid.

Fixes #7495

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[tex3d]

Should the did you mean 'MaybeReorderThread' part say did you mean 'dx::MaybeReorderThread' instead? Is there any way to get the message to say that?

[llvm-beanz]

There is something wrong with the way we're generating the ASTs for these types, and I'm not entirely sure how to fix it. I can carve out some time to look at it, but if it proves to be too time consuming I'd like to suggest tracking that as a follow up?

Does that sound reasonable to you?

[tex3d]

Yeah, that sounds reasonable.

[llvm-beanz]

I have a follow-up change ready that makes this less misleading by removing the correction text. That falls out naturally from fixing how SemaHLSL constructs ASTs to properly put namespaced functions under the namespace instead of the translation unit declaration.

If you're curious to see that change it is here.

[tex3d]

It seems unfortunate to have to look for all using directives for each call lookup. Is there any way to make that better? Shouldn't there be a better way to recognize the namespaces available in the current lookup scope based on using statements?

[tex3d]

Ok, so I guess that it copies the UnqualUsingDirectiveSet implementation from SemaLookup.cpp.

I see we are duplicating this because the ExternalSemaSource mechanism seems to only support unqualified name lookups, as opposed to being hooked in under CppNamespaceLookup/LookupDirect (I think).

Do you think it would be a better approach to hook into the existing C++ lookup process differently?

[llvm-beanz]

Switching to use the C++ lookup process would require us to pre-populate all the impacted APIs in the AST and now allow them to be lazily added (at least not without additional complicated changes). That might not have too significant of a performance impact because we don't have many intrinsics that are in namespaces today.

One drawbacks here is that the candidate selection and overload resolution are pretty tightly coupled in the codebase, so we'd really be opting in a subset of functions to the C++ overload resolution behavior. We do this today for subscript operators, which is fine because HLSL and C++ overload resolution for functions that take a single argument are almost indistinguishable in most cases. Extending this across a wider array of functions could be more problematic in the long run. It is probably fine for the current set of functions, but would be an odd edge case we could trip over.

I think these concerns make me think it is probably better to teach the HLSL-specific code to handle unqualified lookups. WDYT @tex3d?

[tex3d]

So I was thinking of some way of hooking into the same code path that already collects the needed using namespaces as for other lookups, and exposing that namespace set to our lookup somehow. Like by moving our lookup to CppNamespaceLookup instead of under LookupName. It seems like the ExternalSource mechanism was only designed to deal with unqualified C names, as opposed to things that reside in C++ namespaces.

I wasn't thinking of adding the prototypes in advance so that the normal lookups find those.

However, that does bring up another issue. We currently pre-populate dx and vk namespaces using AddIntrinsicFunctionsToNamespace, then we go through the usual path to construct a template/specialization in the usual way in AddOverloadedCallCandidates for those functions later. This pre-population path was originally added for vk::RawBuffer* intrinsics, and has specific limitations for the template types it recognizes from the intrinsic table as a result (see CreateTemplateTypeParmDeclsForIntrinsicFunction). Shouldn't this path be unnecessary with this change?

[llvm-beanz]

I'm not sure I understand what you're saying here. Sema::LookupName calls CppLookupName, which calls CppNamespaceLookup. The external sema source gets called in all cases where LookupName fails, so it should always be invoked for both C and C++ names.

[llvm-beanz]

So... looking at this more. Part of the problem is that we're using the ExternalSemaSource wrong.

DXC added a new interface to the ExternalSemaSource to populate overload candidates, but we should instead probably have handled it all in lookupUnqualified, which is really the right place to be able to perform a lazy lookup. We really only depend on lookupUnqualified for a few simple cases for builtin types, but it could do so much more.

I'm not really sure it is worth fixing this all in DXC.

[tex3d]

I would think we need to visit each (non-transparent) scope on the way up to add the available namespaces, like:

DirectXShaderCompiler/tools/clang/lib/Sema/SemaLookup.cpp

Lines 1054 to 1059 in 94abfe9

	for (DeclContext *UCtx = Ctx; UCtx; UCtx = UCtx->getParent()) {
	if (UCtx->isTransparentContext())
	continue;
	UDirs.visit(UCtx, UCtx);
	}

[tex3d]

Ok, I see now that visitScopeChain should add these, and this just gets the InnermostFileScope for that call.

I think it would have been clearer to declare a new Scope *InnermostFileScope = S;, which would be changed by this loop, instead of changing the original passed-in S.

[tex3d]

I'm also a little confused. The code I pointed to in SemaLookup.cpp is followed by the equivalent code to this. So why does the code in SemaLookup.cpp explicitly visit each context up the parent chain before finding InnermostFileScope calling visitScopeChain?

It appears necessary, since InnermostFileScope will stop at the first global/namespace parent scope, while the prior loop will catch all scopes that could have using statements that impact the current lookup.

[llvm-beanz]

Oh... so that's interesting. This is needed for a C++-ism that I hadn't really thought about.

The visitScopeChain call hits the contexts up the scope chain, but it doesn't guarantee that it is walking up the declaration contexts for the enclosing scope. The commit that adds that code is here, its test case demonstrates the issue:

namespace Other {
  void other_foo();
}

namespace M2 {
  using namespace Other;

  namespace MInner {
    class Bar { 
      void bar();
    };
  }
}

void M2::MInner::Bar::bar() {
  other_foo();
}

The issue is that the scope chain for M2::MInner::Bar::bar() doesn't actually include the using namespace Other declaration, you have to walk the declaration contexts for the enclosing decl.

Because namespaces aren't sealed, we could encounter this, so I'll add a declaration context walk to my PR as well.