Stop name mangling at an imported package name

[dwblaikie]

To ensure the outer package name is not mangled into imported names.

With this change I can successfully link/run a two-file example:

package Mod;
fn HelloWorld() {
  Core.Print(42);
}

import Mod;
fn Run() -> i32 {
  Mod.HelloWorld();
  return 0;
}

$ carbon compile mod.carbon main.carbon
$ carbon link mod.o main.o --output=a.out
$ ./a.out
42

\o/

[jonmeow]

LG. :)

[jonmeow]

Suggested change

	if (!name_scope.is_closed_import) {
	if (!name_scope.is_closed_import || name_scope.parent_scope_id != NameScopeId::Package) {

Per #toolchain comment

[zygoloid]

Can we add a name_scope.is_package() for this? I think this is a little subtle to be writing out by hand here.

[dwblaikie]

#toolchain comment in question: https://discord.com/channels/655572317891461132/655578254970716160/1283200462782464030

[4:00 PM]zygoloid: Does is_closed_import also return true for namespaces in imported packages?
[4:01 PM]JonMeow: Mayyybe yes
[4:01 PM]JonMeow: I guess maybe it's actually something like is_closed_import && parent_scope_id == NameScopeId::Package

Ah, I think I see - thanks for the details.

Hmm, I thought what you were discussing would mean that calling a function in a namespace in an imported package. But when I tried that the is_closed_import (without the Package test added) it seemed to work, mangled as _CHelloWorld.NameSpace.Mod on both call and definition.

What's the case you had in mind where the code as-is (without the Package test) would misbehave?

[jonmeow]

That case. Even if we aren't setting is_closed_import it's possible we should be.

[jonmeow]

If you want to avoid testing the package, I'll suggest also fixing is_closed_import to be set correctly on imported namespaces for other packages.

[dwblaikie]

Hmm - think I have a prototype patch that adds is_closed_import on imported namespaces (mostly a change in AddNamespaceFromOtherPackage) - though I don't know how to test that change in isolation. Should SemIR be dumping this flag somewhere so we can test it? Is there some ramification of it being closed that can be observed/tested? I guess closed import is only an optimization in some way, so presumably it's not observable if it's not dumped?

But yes, once fixed I can observe the failure in a hand-crafted test case (though no coverage in the existing suite seems to trip over this case) - guess I can add that to this patch.

Hmm, there should be testable in that this flag seems to power diagnostics - looking at toolchain/check/testdata/packages/no_prelude/cross_package_import.carbon I tried a similar testcase, but where the duplicate is nested inside a namespace in each library within the same package. The diagnostic about duplicates was still emitted with or without the namespace-is_closed_import patch applied...

OK, so adding the is_closed_import to namespaces imported from packages does change the diagnostic experience:

package Other;
namespace Nested;
fn Nested.F() {}

import Other;
namespace Other;
namespace Other.Nested;
fn Run() {
  Other.Nested.F();
}

From this:

reopen.carbon:2:1: ERROR: Duplicate name being declared in the same scope.
namespace Other;
^~~~~~~~~~~~~~~~
reopen.carbon:1:1: Name is previously declared here.
import Other;
^~~~~~

To this:

reopen.carbon:2:1: ERROR: Duplicate name being declared in the same scope.
namespace Other;
^~~~~~~~~~~~~~~~
reopen.carbon:1:1: Name is previously declared here.
import Other;
^~~~~~

reopen.carbon:3:1: ERROR: Duplicate name being declared in the same scope.
namespace Other.Nested;
^~~~~~~~~~~~~~~~~~~~~~~
reopen.carbon:1:1: In import.
import Other;
^~~~~~
open.carbon:2:1: Name is previously declared here.
namespace Nested;
^~~~~~~~~~~~~~~~~

But I'm not sure this is an improvement - it's not important that the nested namespaces are duplicate - they should never be an issue if the outer namespace is made non-duplicate.

So... I'm wondering if this is even something we want to do? If the is_closed_import is only important for diagnostics, I don't think it's helpful to have diagnostics for these nested cases and maybe we could change the name/purpose of the flag to be narrower and address just that top-level namespace<>package duplication?

If the intent is that this flag ends up being useful for other things, then I guess it's probably still a case of adding it, but then actively disabling the bonus diagnostics it causes because they seem unhelpful - and then I'm back to "how to test this?", "Should it be dumped somewhere?", and "what's the purpose of adding it?".

[jonmeow]

I think the case it might offer a clearer improvement from a diagnostic perspective is:

package Other;
namespace Nested;

import Other;
namespace Other;
fn Other.Nested.F();

Here, the Nested namespace isn't redeclared, but is actually used from the other package. In this case, is_closed would lead to a diagnostic; as-is, I expect this won't diagnose again.

In particular, once the name conflict is resolved, then it will diagnose. My thought here is that renaming Other (either one) shouldn't lead to a new diagnostic.

Note though, we've also considered the meaning of aliases in declarations. These are currently rejected, but I don't know if that'll change. Here, the relevant case would be alias NS = Other.Nested; fn NS.F();. Right now that fails because NS is a non-scoped entity (which is a little misleading, and should maybe explicitly complain about alias). But it's possible we'd go the other direction, allowing aliases in out-of-line declarations (I don't recall if there's a strong reason to disallow it, other than discouraging namespaces). If we did that, then is_closed would be required; having the safeguard now might head off edge-case bugs later.

Overall, really, my gut is that adding names to a nested namespace that conflicts with a nested namespace in another package will be a rare developer mistake. I thus lean towards is_closed to head off bugs.

That said, I can also see the argument that is_closed should just be renamed, maybe in a separate PR.

I don't really have a strong sense here of the best choice, between making the field work as commented, or just changing it. Thoughts?

[dwblaikie]

Here, the Nested namespace isn't redeclared, but is actually used from the other package. In this case, is_closed would lead to a diagnostic;

Yeah, with the patch applied:

b.carbon:2:1: ERROR: Duplicate name being declared in the same scope.
namespace Other;
^~~~~~~~~~~~~~~~
b.carbon:1:1: Name is previously declared here.
import Other;
^~~~~~

b.carbon:3:10: ERROR: Imported packages cannot be used for declarations.
fn Other.Nested.F();
         ^~~~~~
b.carbon:1:1: In import.
import Other;
^~~~~~
a.carbon:2:1: Package imported here.
namespace Nested;
^~~~~~~~~~~~~~~~~

as-is, I expect this won't diagnose again.

Can confirm, yeah - it diagnoses the Other duplicate, but no error for using Nested.

In particular, once the name conflict is resolved, then it will diagnose. My thought here is that renaming Other (either one) shouldn't lead to a new diagnostic.

I've got mixed feelings there - the tradeoff compared to the example I provided where it produces an extra diagnostic that isn't needed. Neither's a great situation, to be sure.

I like your test case though, so I'll add that and send out a PR. #4312

[josh11b]

Now that #4312 is merged, can we progress this PR?

[jonmeow]

@josh11b FYI this PR still needs to be updated. The comment is unchanged.