Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Generator cannot detect namespace conflicts when a sub-namespace is declared by an external type that isn't included in externals #97

Closed
mpiroc opened this issue Jul 23, 2018 · 5 comments

Comments

@mpiroc
Copy link
Contributor

mpiroc commented Jul 23, 2018

In .NET, a fully-qualified type name must not collide with a fully-qualified namespace name. The generator attempts to detect these collisions (and rename the type to avoid them). To do this, it needs to know every possible namespace that could cause a conflict.

For root namespaces, this is simple: just inspect the targets property of the assembly itself as well as each of its dependencies (recursively). However, a type can also declare a sub-namespace by including the sub-namespace in its fqn. So we also search types and externals for any sub-namespaces.

The problem arises when a type in my-dependency declares a sub-namespace, but that type is not referenced from my-assembly (and therefore does not appear in externals). The generator has no way to be aware of this sub-namespace, so it can't detect conflicts.

When the generated .NET code for my-assembly is compiled, it takes a reference on my-dependency. So every namespace in my-dependency (even those not referenced from my-assembly) is available to the compiler, and can cause conflicts.

Previously this wasn't a problem, because all external types were present in dependencies. Now that we've switched to externals, this is no longer the case.

@mpiroc
Copy link
Contributor Author

mpiroc commented Jul 23, 2018

One way to fix this is to require that a PackageVersion declare all of the sub-namespaces that it defines, in addition to the root namespace.

@RomainMuller
Copy link
Contributor

We haven't "switched" to "externals" - they were already there. We renamed the field from "externalTypes" to "externals", that's all.

It feels impractical to copy the whole union of types from all dependencies in all assemblies (it makes them up to 100x larger)...

Regarding your issue - I'm having a hard time framing how one makes this happen... would you mind coming up with a minimal repro case so I can understand better? I'm wondering if simply ensuring none of the .Net namespaces configured in the closure overlap would be sufficient? That's something jsii could easily check for I think...

@mpiroc
Copy link
Contributor Author

mpiroc commented Jul 23, 2018

Simple repro:

{
    ...
    "name": "my-dependency",
    "targets": {
        "dotnet": {
            "namespace": "Foo.Bar"
        },
    },
    "types": {
        "my-dependency.Sub1.Type1": { ... },
        "my-dependency.Sub2.Type2": { ... },
    },
    ...
}

...

{
    ...
    "name": "my-assembly",
    "targets": {
        "dotnet": {
            "namespace": "Foo"
        },
    },
    "dependencies": {
        "my-dependency": {
            "targets": {
                "dotnet": {
                    "namespace": "Foo.Bar"
                },
            },
        }
    },
    "types": {
        "my-assembly.Bar.Sub1": { ... },
    },
    "externals": {
        "my-dependency.Sub2.Type2": { ... },
    }
    ...
}

The generated code for my-dependency will contain a namespace Foo.Bar.Sub1, while the generated code for my-assembly will contain a type with the same name (Foo.Bar.Sub1). We can't detect this conflict because my-assembly has no idea that Foo.Bar.Sub1.Type1 exists in my-dependency.

@mpiroc
Copy link
Contributor Author

mpiroc commented Jul 23, 2018

I'm not suggesting that we copy the whole union of types. Rather, something like this:

{
    ...
    "name": "my-assembly",
    "targets": {
        "dotnet": {
            "namespace": "Foo"
        },
    },
    "dependencies": {
        "my-dependency": {
            "targets": {
                "dotnet": {
                    "namespace": "Foo.Bar",
                    "childNamespaces": [
                        "Sub1",
                        "Sub2"
                    ],
                },
            },
        }
    },
    "types": {
        "my-assembly.Bar.Sub1": { ... },
    },
    "externals": {
        "my-dependency.Sub2.Type2": { ... },
    }
    ...
}

@mpiroc
Copy link
Contributor Author

mpiroc commented Aug 11, 2018

This was fixed by #129.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants