btc-ag / service-idl

Xtext-based Service IDL (Interface Definition Language) and Code Generators for Protobuf, C++, Java and .NET
Eclipse Public License 2.0
8 stars 8 forks source link

Name conflicts between IDL identifiers and generated code lead to build failure #122

Open huttenlocher opened 6 years ago

huttenlocher commented 6 years ago

I discovered this issue recently by pure occasion. I designed an IDL where I used an output parameter with the name "result". Code was generated successfully, but when I tried to compile the resulting code, I received a build failure due to variable redefinition. Reason: in the generated code there is also a variable called "result" (at least in C++ dispatcher, probably also in other places). Here is the simple IDL to reproduce the behavior:

module Demo
{
    interface Foo [ guid = 8B08E141-4DB2-4AD2-BACF-FA4257C42481 ]
    {
        DummyMethod(out sequence<uuid> result) returns boolean;
    };
}

Here is an extract from the problematic generator output:

// prepare [out] parameters
BTC::Commons::CoreExtras::InsertableTraits< BTC::Commons::CoreExtras::UUID >::AutoPtrType result(      // <----------- #1 !!!
   BTC::Commons::FutureUtil::GetOrCreateDefaultInsertable(BTC::Commons::CoreExtras::InsertableTraits< 
   BTC::Commons::CoreExtras::UUID >::MakeEmptyInsertablePtr()) );
             auto resultFuture = result->GetFuture();

// call actual method
auto result( GetDispatchee().DummyMethod(*result).Get() );   // <----------- #2 !!!

One solution may be to use something like prefix "_" for all "internal" variables to avoid such collisions.

sigiesec commented 6 years ago

Using some prefix/suffix for generated variables might be a solution, but then using this prefix/suffix must be forbidden within the IDL file, otherwise such conflicts still might occur.

sigiesec commented 6 years ago

What behaviour would you like to see? I can imagine the following options:

huttenlocher commented 6 years ago

I would do it in the simplest way either by adding internally some prefix or suffix (like one or more underscores - or both ways at the beginning and the end like "internalVar"), which are not very likely to be expected in a real-world IDL) and use a simple validation rule to immediately point the IDL designer that such names are reserved and not allowed in the IDL. This can also be enforced by the grammar, but the validation in the more preferred way. We could then use a utility method to wrap all internal variables and ensure this naming (e.g. "makevar("abc") will make my internal variable "abc" named in this way; here we could also guarantee some other consistent style rules like "always lowercase" etc). Or we could add a set of variable names to the "black list", which already exists (see "com.btc.serviceidl.validation.KeywordValidator.xtend"). By the way, this black list was introduced as one guy actually named his IDL variables "base", "fixed" and "internal" which are reserved keywords in C# and break the compilation. But this is not a flexible and error-prone solution, I would prefer the first one, as it is easy to do, reliable and the error is immediately visible to the IDL designer. I also think, this is a reasonable trade-off, as almost nobody would name his public interface variable like "abc__"), so there is almost no real restriction. But I am open to other proposals, if you have some favorite approach.

sigiesec commented 6 years ago

There is a related issue with the module names. E.g. using "Test" as a module name leads to some ambiguity in namespace resolution in the generated .NET code.