[cling] Always add primary NamespaceDecl to fNSSet

[jase231]

This Pull request:

Marks the primary NamespaceDecl as having external visible storage, ie. so that clang will query ROOT for any name it can't find in that namespace, and adds it into the fNSFromRootmaps/fNSSet alongside any potential non-primary redeclared context, so ROOT correctly reports any rootmap-redeclared namespace as an autoload candidate, even when the primary declaration comes from the PCH.

From the clang manual: Clang Internals Manual

Example:

1. ROOT is built with gminimal=ON and without runtime modules. That build's PCH does not declare e.g. ROOT::RDataFrame, but it does declare the ROOT:: namespace. That NamespaceDecl in the PCH is designated the primary, and since it is sourced directly from the PCH it will never be inserted into fNSFromRootmaps.
2. Then, dataframe components are installed, e.g. via wheel. These components can be autoparsed and their .rootmap(s) redeclare the ROOT:: namespace, which contains e.g. ROOT::RDataFrame. The NamespaceDecl representing this redeclaration is added to fNSFromRootmaps.
3. The user tries to access ROOT::RDataFrame, clang's lookupImpl tries to find the external decl but does so by querying TCling with the primary NamespaceDecl for the ROOT:: namespace. TCling (through a chain of calls) checks for the primary in fNSFromRootmaps, but fails, because only dataframe's NamespaceDecl is in the set.
4. TCling reports failure, and the user sees error: no member named 'RDataFrame' in namespace 'ROOT' even though RDataFrame is present and could have been autoparsed.

Checklist:

• tested changes locally
• updated the docs (if necessary)

Bug discovered by LLM

[pcanal]

Why do we need to record both?

[jase231]

As in, "why do we need to record both rather than only recording the primary"? I don't think we do; if you think so too I will change to always recording the primary. I kept both because I didn't want to potentially break anything by dropping the non-primary in case that was used elsewhere, but I haven't found any other such case. I'll investigate the test failures this week. I'll also try to reproduce the original issue in a standard ROOT build, since I initially ran into this issue while working on wheels.

Also, based on getPrimaryContext's definition I think the return should always be non-null i.e. I can just cast, if you agree.

[pcanal]

I am confused. I read:

"why do we need to record both rather than only recording the primary"? I don't think we do
I don't think we do;
I kept both because ....

Where the last 2 statement seem contradictory ...

[jase231]

I wanted to be conservative in my change since I'm inexperienced in this part of the codebase, so rather than completely stop inserting non-primary decls, I decided to record both to avoid risking

to potentially break anything by dropping the non-primary in case that was used elsewhere

So recording both was out of abundance of caution. If you think recording non-primaries is pointless, then I will stop doing so, but I wasn't confident enough in my understanding to be sure that recording non-primaries is pointless.

[pcanal]

I do not know for sure whether both are needed. The comment however is pretty assertive: "When cling ... queries ... it always does so using the primary DeclContext", so I assumed the non-primary was unnecessary.

Either way, storing both if it is not needed would both cost 'unnecessary memory use' and increase the challenge in understanding the code (eg make wrong assumption of what is needed or not).

I.e. I strongly recommend that we investigate whether or not they are both needed (ideally both via testing and understanding the usage of the collection).

Thanks.

[github-actions]

Test Results

    22 files      22 suites   3d 4h 3m 44s ⏱️
 3 834 tests  3 785 ✅ 0 💤  49 ❌
67 803 runs  67 678 ✅ 0 💤 125 ❌

For more details on these failures, see this check.

Results for commit cba31ae.