[C#] Using 'global::' as qualifying_start_ within BaseGenerator

[thansen24]

I recently came across an issue with a namespace inference error. Given I have these two files:

Color.fbs

namespace MyGame.Sample;

enum Color:byte { Red, Green, Blue }

Monster.fbs

include "Color.fbs";

namespace MyCompany.MyGame.Sample;

table Monster {
  name:string;
  color:MyGame.Sample.Color;
}

root_type Monster;

The generated C# code will not compile because it will say

The type or namespace name 'Color' does not exist in the namespace 'MyCompany.MyGame.Sample' (are you missing an assembly reference?)

I realize that my namespaces in this contrived example could just be rearranged to make it work, but in my real world case I can't. I think this this can be resolved by adding "global::" as the qualifyingstart within BaseGenerator class whereas right now it's just the empty string. Are you open to such a change?

[aardappel]

I guess FlatBuffers namespace lookup is more permissive than C#, that is unfortunate.

Yes, we should probably fix this, even though it is kinda niche? @dbaileychess who may have opinions :)

[aardappel]

What happens if in MyCompany.MyGame.Sample there is also a definition for Color ? which does it pick? If it picks the local one, does the C# code have the right namespace for that case?

[thansen24]

Your comment about if there is another definition for Color in that namespace highlights a similar problem that all stems from the same underlying issue. Given I have these three files:

Color.fbs

namespace MyGame.Sample;

enum Color:byte { Red, Green, Blue }

AnotherColor.fbs

namespace MyCompany.MyGame.Sample;

enum Color:byte { Purple }

Monster.fbs

include "Color.fbs";

namespace MyCompany.MyGame.Sample;

table Monster {
  name:string;
  color:MyGame.Sample.Color = Blue;
}

root_type Monster;

In this case you will get a different compile error saying that "Blue" doesn't exist in the enum Color, because the generated code is picking the wrong Color enum due to the namespace inference. Prepending global:: removes all this ambiguity and inference and will make the autogenerated code work in both examples I've come up with as you would expect it to.

[thansen24]

This is actually very similar to the example provided in the documentation for the global:: alias:

https://docs.microsoft.com/en-us/dotnet/csharp/language-reference/operators/namespace-alias-qualifier

The example they provide is this:

namespace MyCompany.MyProduct.System
{
    class Program
    {
        static void Main() => global::System.Console.WriteLine("Using global alias");
    }

    class Console
    {
        string Suggestion => "Consider renaming this class";
    }
}

Where you have a Console class in a namespace MyCompany.MyProduct.System which conflicts with the built in .net Console in the System namespace and without the global:: it will pick the wrong one.

[aardappel]

Ugh, I wish languages would have a universal namespace lookup semantics.. but I guess it's too late for that now :)

[TYoungSL]

The generated C# code will not compile because it will say

The type or namespace name 'Color' does not exist in the namespace 'MyCompany.MyGame.Sample' (are you missing an assembly reference?)

The generated code refers to Color or to MyGame.Sample.Color? The error message you describe occurs due to a lack of namespace entirely.

I don't think global:: actually fixes that issue, just using the full namespace does.

[thansen24]

The generated code refers to MyGame.Sample.Color. The problem is that it's ambiguous because there is a MyGame.Sample.Color and a MyCompany.MyGame.Sample.Color. The generated code is already using the full namespace, but the full namespace is ambiguous, that's why global:: fixes the issue to disambiguate between those two namespaces.

[TYoungSL]

Sorry, something doesn't make sense... Could you please show the few lines of code that where error CS0246 is thrown?

An ambiguous symbol present in identical namespaces can be resolved by assembly namespace aliasing, but an ambiguous symbol between separate namespaces should be resolved by simply using the full namespace.

Example: https://dotnetfiddle.net/fuT6RV

CS0104: https://dotnetfiddle.net/XFJXNS

Where you have a Console class in a namespace MyCompany.MyProduct.System which conflicts with the built in .net Console in the System namespace and without the global:: it will pick the wrong one.

This could be the case, but could you demonstrate a proof of concept that gives CS0246?

Also, introducing the use of global:: would break versioning schemes that may (in future or in niche cases) rely on namespace aliasing (as assembly namespace aliasing was intended to be used).

[thansen24]

I pushed up an example pull request that demonstrates what I'm talking about and will fail the C# test run because it won't compile. Hopefully that makes it more clear.

https://github.com/google/flatbuffers/pull/6799

Also, introducing the use of global:: would break versioning schemes that may (in future or in niche cases) rely on namespace aliasing (as assembly namespace aliasing was intended to be used).

I'm unclear about that feature, so I can't really speak to that.

[TYoungSL]

I see the following;

C:\projects\flatbuffers\tests\namespace_test\NamespaceA\NamespaceB\ColorTestTable.cs(26,49): error CS0117: 'Color' does not contain a definition for 'Blue' [C:\projects\flatbuffers\tests\FlatBuffers.Test\FlatBuffers.Test.csproj]
C:\projects\flatbuffers\tests\namespace_test\NamespaceA\NamespaceB\ColorTestTable.cs(26,24): error CS1750: A value of type '?' cannot be used as a default parameter because there are no standard conversions to type 'Color' [C:\projects\flatbuffers\tests\FlatBuffers.Test\FlatBuffers.Test.csproj]

I don't see CS0246.

I extracted NamespaceA\NamespaceB\ColorTestTable.cs and created a minimal example https://dotnetfiddle.net/FT8xUM

[TYoungSL]

Example using the global:: extern alias without the errors;

https://dotnetfiddle.net/jNa635

Example using namespace aliasing to resolve between the namespaces:

https://dotnetfiddle.net/Rp3dSn

Example with escaping the not-unique-in-scope nested namespace by appending an underscore:

https://dotnetfiddle.net/ghmEdX

[TYoungSL]

I pushed up an example pull request that demonstrates what I'm talking about and will fail the C# test run because it won't compile. Hopefully that makes it more clear.

6799

Also, introducing the use of global:: would break versioning schemes that may (in future or in niche cases) rely on namespace aliasing (as assembly namespace aliasing was intended to be used).

I'm unclear about that feature, so I can't really speak to that.

https://theburningmonk.com/2014/02/c-extern-alias-and-ilmerged-assemblies/

This article examines one scenario where an assembly is being aliased.

The scenarios generally involve versioning; 2 versions of the same library with identical symbols.

Generally one would want to convert between the symbols. In the case of flatbuffers, one simply copies offsets in the cases where they are identical or at least compatible in structure. This is more common in libraries that are built from collections of models e.g. of the such that flatc generates. One would be global::SomeNamespace.Color and the others would be v1::SomeNamespace.Color, v2::SomeNamespace.Color, etc. In the scenario where they have already been compiled, the global:: alias for referring to the current version is fine.

Using the global:: alias would break that scenarios that depend on compiling against alternative aliases.

I don't think either namespace aliasing or using global:: are the best approach here.

The use of global:: to resolve this scenario should be expressed behind a compiler flag instead of enabled by default.

Logically speaking, alternative naming of namespaces to satisfy the resolution scope should be employed by the user instead.

I realize that my namespaces in this contrived example could just be rearranged to make it work, but in my real world case I can't.

What is the real world reason you can't?

[thansen24]

I've never personally used ilmerged assemblies and aliasing in that way, so can't speak confidently about how this would affect that use case. Although I would think that use case is even more niche than mine, but I could be wrong about that if this is a widely used approach.

As an additional reference point, protobuffers always prepends "global::", without a feature switch to opt out of it, similar to what I've proposed.

https://github.com/protocolbuffers/protobuf/blob/master/src/google/protobuf/compiler/csharp/csharp_helpers.cc#L323-L347

Is there a reason ilmerged assemblies are in more widespread use within the flatbuffer community than protobuffers? Maybe there is some way to make it work anyway that I'm not aware of?

A command line switch to enable/disable disable this would also be fine, I personally think the default should be on, but someone with broader knowledge of how it's used in the wild could make that judgement call.

Logically speaking, alternative naming of namespaces to satisfy the resolution scope should be employed by the user instead.

Yep, if that were an option I agree with you. In this case I'm referencing two .fbs files that I don't own or have influence over how they are created so I cannot.

[TYoungSL]

Yep, if that were an option I agree with you. In this case I'm referencing two .fbs files that I don't own or have influence over how they are created so I cannot.

That sounds like a bigger external problem, (maybe bad) policy or something?

Some people use flatbuffers specifically because it's different from protobufs, so not an apt comparison, kind of invokes the argumentum ad populum fallacy.

A command line switch and default to current behavior would be convenient. Introducing the flag as default on I think should be a new major version.

In one scenario @thansen24 you have a preference for renaming symbols in the case of escaping keywords (by appending an underscore) over keeping the symbol names the same, yet at the same time you want to avoid renaming namespaces. These are not directly conflicting, but they point to a rather specific scenario motivation that involves (potentially bad) policy around symbol names.

[TYoungSL]

I should note that the ILMerge scenario just clearly demonstrates the use of extern aliases other than global::, and is not the only use case.

[TYoungSL]

I'd rather have cake and eat it too;

• Default to no prefix, allowing errors to be identified, (current behavior)
• An option for using global:: prefix, (keeping symbol names)
• another for escaping nested namespaces by _ suffix,
• default to _ suffix escaping keywords, (maybe should be an option instead of default?)
• another option for @ prefix escaping keywords. (keeping symbol names)

[thansen24]

Some people use flatbuffers specifically because it's different from protobufs, so not an apt comparison, kind of invokes the argumentum ad populum fallacy.

I agree that people use it specifically because it's different, that's why I'm here! :)

In one scenario @thansen24 you have a preference for renaming symbols in the case of escaping keywords (by appending an underscore) over keeping the symbol names the same, yet at the same time you want to avoid renaming namespaces.

As I replied in your previous comment about that issue, I was following the convention already in place for other languages but otherwise was indifferent to which way the problem was solved.

I guess fundamentally I come from the perspective that if it's a valid .fbs file(s), every effort should be made to produce code in each language will compile out of the box.

I'd rather have cake and eat it too;

• Default to no prefix, allowing errors to be identified, (current behavior)
• An option for using global:: prefix, (keeping symbol names)
• another for escaping nested namespaces by _ suffix,
• default to _ suffix escaping keywords, (maybe should be an option instead of default?)
• another option for @ prefix escaping keywords. (keeping symbol names)

My only concern about any of that would be with feature switch bloat, but if other people don't share that concern I wouldn't have a problem with any/all of those suggestions.

[TYoungSL]

To condense the feature switch bloat, it's purely a decision between permitting improper naming versus escaping improper names versus allowing improper names to result in errors.

If I see that a namespace issue is occurring, given my own control over the schema, I'd want the parser or code generator to warn me or throw an error rather than have it generate any code. I'd rename the symbols if that occurs.

When the perspective is that any valid schema should generate valid code (assuming it's not errata in schema documentation and parsing to allow the creation conflicting naming situations like these), then obviously you have to decide between escaping or preserving symbol names.

Could condense it down to one feature switch with 3 settings; e.g. --improper-naming=; error, escape, and preserve.

[thansen24]

Perhaps @dbaileychess has some thoughts on all this?

[dbaileychess]

Lets leave the keyword escaping for #6792 and focus only on namespacing.

Oddly enough, if you do:

using NamespaceB = NamespaceB;

You can get around the issues by using the :: notation in the namespace naming. Thus

NamespaceB::Color refers to NamespaceB.Color and not NamespaceA.NamespaceB.Color

I took @TYoungSL minimal example and changed it https://dotnetfiddle.net/cU0bVN and it compiles.

So we could change the generator to put all the aliases at the top like that, and just the usages to use :: everywhere (as long as its fully qualified.

In this example (https://dotnetfiddle.net/o0Lea5) the ColorTestTableT class is using NamespaceB.Color and it has access to Purple.

[thansen24]

@dbaileychess, I think your alias idea only works nicely if the namespace in question is just one word.

using NamespaceA.NamespaceB = NamespaceA.NamespaceB; is not allowed. So you'd probably have to do something like:

using NamespaceA_NamespaceB = NamespaceA.NamespaceB; if you wanted to go the alias route over just using global::

Here's an example of what I mean: https://dotnetfiddle.net/tTFxgi

[dbaileychess]

I see, that's unfortunate.

I think the best path forward then is to make it a feature option to prepend global:: to all namespaces and default it to false. @thansen24 could you amend #6767 with this?

[thansen24]

yep, will do

[thansen24]

@dbaileychess, PR #6767 has been updated to include a feature option defaulted to false as you had suggested.