Stop name mangling at an imported package name #4294

dwblaikie · 2024-09-10T22:43:44Z

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 · 2024-09-10T23:04:46Z

toolchain/lower/mangler.cpp

@@ -89,8 +89,10 @@ auto Mangler::MangleInverseQualifiedNameScope(llvm::raw_ostream& os,
        CARBON_FATAL() << "Attempting to mangle unsupported SemIR.";
        break;
    }
-    names_to_render.push_back(
-        {.name_scope_id = name_scope.parent_scope_id, .prefix = '.'});
+    if (!name_scope.is_closed_import) {


Suggested change

if (!name_scope.is_closed_import) {

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

Per #toolchain comment

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

#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?

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

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.

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

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

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

Added test coverage for this problem, added the helper function (but I could only figure out how to make IsImportedPackage - I wasn't sure how to get the NameScopeId from the NameScope so I could check if it was == NameScopeId::Package - happy to change that to something more general (could be a free function that takes a NameScopeId and NameScopeStore and test against Package, then does the lookup (which would be a bit redundant since we've already done the lookup in the mangler) and other checks) folks might suggest?)

Note, discussed offline, I think IsImportedPackage is fine as a helper (versus IsPackage)

Spin off from #4294 (comment) --------- Co-authored-by: Richard Smith <[email protected]> Co-authored-by: Carbon Infra Bot <[email protected]>

To ensure the outer package name is not mangled into imported names. With this change I can successfully link/run a two-file example: ``` import Mod; fn Run() -> i32 { Mod.HelloWorld(); return 0; } ``` ``` package Mod; fn HelloWorld() { Core.Print(42); } ``` ``` $ carbon compile mod.carbon main.carbon $ carbon link mod.o main.o --output=a.out $ ./a.out 42 ``` \o/

If the call is to a function in a namespace in a foreign package, it mismangles by stopping at the foreign namespace, missing the foreign package name.

Spin off from carbon-language#4294 (comment) --------- Co-authored-by: Richard Smith <[email protected]> Co-authored-by: Carbon Infra Bot <[email protected]>

toolchain/sem_ir/name_scope.h

jonmeow · 2024-09-18T20:12:24Z

toolchain/lower/mangler.cpp

@@ -89,8 +89,10 @@ auto Mangler::MangleInverseQualifiedNameScope(llvm::raw_ostream& os,
        CARBON_FATAL() << "Attempting to mangle unsupported SemIR.";
        break;
    }
-    names_to_render.push_back(
-        {.name_scope_id = name_scope.parent_scope_id, .prefix = '.'});
+    if (!name_scope.is_closed_import) {


Note, discussed offline, I think IsImportedPackage is fine as a helper (versus IsPackage)

Co-authored-by: Jon Ross-Perkins <[email protected]>

jonmeow

Looks good.

toolchain/sem_ir/name_scope.h

Co-authored-by: Jon Ross-Perkins <[email protected]>

github-actions bot requested a review from josh11b September 10, 2024 22:44

github-actions bot added the toolchain label Sep 10, 2024

jonmeow approved these changes Sep 10, 2024

View reviewed changes

jonmeow added this pull request to the merge queue Sep 10, 2024

josh11b approved these changes Sep 10, 2024

View reviewed changes

jonmeow removed this pull request from the merge queue due to a manual request Sep 10, 2024

jonmeow reviewed Sep 10, 2024

View reviewed changes

dwblaikie mentioned this pull request Sep 13, 2024

Apply is_closed_import to imported namespaces #4312

Merged

github-merge-queue bot pushed a commit that referenced this pull request Sep 17, 2024

Apply is_closed_import to imported namespaces (#4312)

ff1cc43

Spin off from #4294 (comment) --------- Co-authored-by: Richard Smith <[email protected]> Co-authored-by: Carbon Infra Bot <[email protected]>

dwblaikie added 4 commits September 18, 2024 19:11

Add test demonstrating mismangling for cross-package calls

25618f6

If the call is to a function in a namespace in a foreign package, it mismangles by stopping at the foreign namespace, missing the foreign package name.

Check the enclosing name scope is a package before stopping

d5ee110

Add IsImportedPackage helper

3600196

dwblaikie force-pushed the mangle_imported_name branch from b7dec12 to 3600196 Compare September 18, 2024 19:14

jonmeow reviewed Sep 18, 2024

View reviewed changes

Correct function comment

1ba213f

Co-authored-by: Jon Ross-Perkins <[email protected]>

jonmeow approved these changes Sep 18, 2024

View reviewed changes

toolchain/sem_ir/name_scope.h Outdated Show resolved Hide resolved

Update spelling for trivial function

8073c78

Co-authored-by: Jon Ross-Perkins <[email protected]>

dwblaikie force-pushed the mangle_imported_name branch from ffe2afd to 8073c78 Compare September 18, 2024 20:37

dwblaikie enabled auto-merge September 18, 2024 20:41

dwblaikie added this pull request to the merge queue Sep 18, 2024

Merged via the queue into carbon-language:trunk with commit 4631767 Sep 18, 2024
8 checks passed

dwblaikie deleted the mangle_imported_name branch September 18, 2024 23:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Stop name mangling at an imported package name #4294

Stop name mangling at an imported package name #4294

dwblaikie commented Sep 10, 2024

jonmeow left a comment

jonmeow Sep 10, 2024 •

edited

Loading

zygoloid Sep 10, 2024

dwblaikie Sep 11, 2024

jonmeow Sep 11, 2024

jonmeow Sep 11, 2024

dwblaikie Sep 13, 2024

josh11b Sep 18, 2024

jonmeow Sep 18, 2024

dwblaikie Sep 18, 2024

jonmeow Sep 18, 2024

jonmeow Sep 18, 2024

jonmeow left a comment

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

Stop name mangling at an imported package name #4294

Stop name mangling at an imported package name #4294

Conversation

dwblaikie commented Sep 10, 2024

jonmeow left a comment

Choose a reason for hiding this comment

jonmeow Sep 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonmeow left a comment

Choose a reason for hiding this comment

jonmeow Sep 10, 2024 •

edited

Loading