icerpc / icerpc-csharp

A C# RPC framework built for QUIC, with bidirectional streaming, first-class async/await, and Protobuf support.
https://docs.icerpc.dev
Apache License 2.0
108 stars 13 forks source link

slice2cs crash when an include contains a single letter top level nested module #150

Closed externl closed 3 years ago

externl commented 3 years ago

A.ice

#include <B.ice>
module AModule
{
    interface Foo
    {
        void func();
    }
}

B.ice

module M::N
{
    struct Smn
    {
        string s;
    }
}
D:\IceRPC\icerpc-csharp>cpp\bin\x64\Debug\slice2cs.exe -I. A.ice
Assertion failed: _containerStack.size() == 1, file D:\IceRPC\icerpc-csharp\cpp\src\Slice\Parser.cpp, line 5024

Note: A { module B {} } works fine, it's just A::B that crashes.

InsertCreativityHere commented 3 years ago

It was a mistake with how we popped nested modules off the container stack after parsing them... https://github.com/zeroc-ice/icerpc-csharp/blob/9263520e0a2538ab07c4ca415c935ff57d38ffd2/cpp/src/Slice/Grammar.y#L488-L495 We iterate through the nested identifier M::N and every time we see a ::, we pop a module off the stack of containers we're in, and we pop an extra module off at the end (the total number of modules is equal to the number of :: + 1).

We search for the next :: 2 characters ahead of the last position (to skip the last :: we hit). But when we're at the start of the string (position 0), this means we accidentally skip the first 2 characters! And when the identifier is only 1 character long then we miss the first ::!

M::N
  ^
Where we start searching for `::` from, but we only see a single `:` and miss it.

Long::Name
  ^
Not an issue when the identifier is at least 2 characters long.

So for M::N we should pop off 2 modules when we're done parsing them both, but we were only popping one off, because of this bug. Being stuck in the scope of module M when it should of been in global scope thrashed the rest of the parsing.

Changing the start position from 0 to -2 fixes it. Because then we'll start searching at (-2 + 2 = 0) the start of the string like we should!

externl commented 3 years ago

I think we should add a comment and a test also.

pepone commented 3 years ago

I reverted the fix as it breaks Windows build

InsertCreativityHere commented 3 years ago

Fixed in 2ba622f973c4de1b580aae1afe376a39791699cd I also added a comment and testing for this into the Slice/parser test.