Re-implemented 'slicec' #50

InsertCreativityHere · 2021-11-03T19:53:06Z

I opened this PR mostly so everyone can review my changes to slicec-cs. It's a very rough port, it's basically the bare minimum that compiles everything okay. A future cleanup pass is absolutely in order, so that it can take better advantage of the new slicec API.

slicec has been completely rewritten from the ground up, as such it's diff is basically going to be unusable.

To build, the same old cargo build should work. Let me know if you have any questions, or find anything smelly!

InsertCreativityHere · 2021-11-03T20:05:22Z

Big changes:

The AST doesn't need to be passed around and baked in everywhere now.
I split Member into Parameter and DataMember now.
There is still a Member trait, which both of those implement.
Broke apart some of my traits and introduced a new Commentable and Attributable trait.
You can see them all in 'https://github.com/zeroc-ice/icerpc/pull/50/files#diff-62f344043ca6790f5230493beb1d13fddcb4456332c2a377d6c86df58a27e507'
Node is completely gone because of the new AST design.
Since we still need something for matching on types, I added a Types wrapper for this.

externl

Commented on what I've reviewed so far. Looks much nicer than the old way so far!

externl · 2021-11-03T20:32:07Z

slicec-cs/src/decoding.rs

 ) -> CodeBlock {
    let mut code = CodeBlock::new();

    let (required_members, tagged_members) = get_sorted_members(members);

-    let mut bit_sequence_index = -1;
-    let bit_sequence_size = get_bit_sequence_size(members, ast);
+    let mut bit_sequence_index: i64 = -1;


We should just make this an Option<u32>.

Sure, I wasn't entirely sure what all these checks were doing so I left them alone.
I'm not sure what I changed to cause it, but rustc thought that this was a u32 (probably because of a comparison we do later), and it complained that -1 didn't fit in a u32, so I explicitly set the type.

If the check works with an Option<u32> I argee that would be cleaner.

externl · 2021-11-03T20:40:15Z

slicec-cs/src/decoding.rs

        )
    };

    let (required_members, tagged_members) = get_sorted_members(&non_streamed_members);

-    let mut bit_sequence_index = -1;
-    let bit_sequence_size = get_bit_sequence_size(&non_streamed_members, ast);
+    let mut bit_sequence_index: i64 = -1;


slicec-cs/src/dispatch_visitor.rs

externl · 2021-11-03T20:49:32Z

slicec-cs/src/encoding.rs

    }

    code
 }

 pub fn encode_type(
    type_ref: &TypeRef,
-    bit_sequence_index: &mut i32,
+    bit_sequence_index: &mut i64,


&mut Option<u32>.

externl · 2021-11-03T20:51:10Z

slicec-cs/src/encoding.rs

@@ -354,20 +341,18 @@ pub fn encode_dictionary(

 pub fn encode_as_optional(
    type_ref: &TypeRef,
-    bit_sequence_index: &mut i32,
+    bit_sequence_index: &mut i64,


slicec-cs/src/enum_visitor.rs

externl · 2021-11-03T21:02:13Z

slicec-cs/src/slicec_ext/attribute_ext.rs

-                _ => Some(deprecate.to_vec().join("\n")),
-            }
-        // } else if check_parent {
-        // get_deprecate_reason(named_symbol.parent(), false)


The new compiler does full recursion, the old compiler only checked the parent. Is this new behavior better. Thoughts @pepone?

I don't think this is required here if you deprecate and interface this will add an obsolete attribute to the interface and the operations? I will check the generated code to see if we need to recurse.

slicec-cs/src/slicec_ext/named_symbol_ext.rs

pepone

First round of comments, I will continue review in the morning!

slicec-cs/src/builders.rs

slicec-cs/src/comments.rs

slicec-cs/src/decoding.rs

slicec/src/visitor.rs

pepone · 2021-11-03T22:14:52Z

slicec/src/grammar/wrappers.rs

+// No one in their right mind will keep this wrapper around for actual use.
+// Without passthrough methods, I think we can eliminate the 'Types' wrapper all together and just
+// use Elements. Also, it will be so simple, we can likely drop the macros entirely.
+


I find it difficult to understand what is the purpose of this, too many macros and too little comments :)

Yes, while porting I realized that most of this file is actually useless, hence the comment. I might cull it tonight, or might leave it until later to avoid drastic changes to my PR.

The gist of this file is that we need some way to take a trait object like &dyn Type and cast/match it to a concrete type like Struct. There's 2 enum wrappers in this file that let you do this, Types for matching types, and Elements for matching elements.

In hindsight, this is pretty dumb. One wrapper is enough. I plan to delete Types and just keep Elements.
I also tried to make the wrappers super smart and fancy so you could use a Types like a Type, also pretty dumb. You can just use the concrete type instead.

I'm probably going to cull it tonight or tomorrow afternoon.

Porting the code gave me alot of insight into which parts of my API were over-complicated, or not as useful as I thought they'd be. But since I opened the PR immediately after porting, I haven't had the chance to fix those up yet : v)

The gist of this file is that we need some way to take a trait object like &dyn Type and cast/match it to a concrete type like Struct. There's 2 enum wrappers in this file that let you do this, Types for matching types, and Elements for matching elements.

Isn't this API a bit against the Rust way of doing things, having to cast traits to structs, or is this something common in rust?

I used the word 'cast' to get the idea across, but there is no actual casting. Casting is definitely a bad in Rust.

There's alot to unpack with this. This was a design decision I spent much time thinking about in the very beginning. In rust, there's two ways to get 'polymorphic' like behavior: enums, and traits.

My approach was to use both. There is the Type trait, which individual structs can implement, and there is the Types enum, for when matching is useful. It's a best-of-both-worlds approach I think.

The Rustyish way to do this is to have a Type enum, with a variant for each type, which owns it's contents. No trait. Any 'trait' methods get implemented directly on the enum.

But this doesn't really work for us, the main issue is that enum variants aren't first class types. There's no way to use Types::Class as a type, only Types. So, using an enum would lead to weaker typing in our API. Take how we could store a base class field for instance, the best we could do is:

struct Class { base: Type, }

Instead of

struct Class { base: Type::Class, }

This is clearly lame though, since a base class has to be a class, not just any type. Being stuck with just Type means more unwrapping and matching for the code-gen, and another place for errors to be introduced.

There's also the issue that you can't implement a trait on a specific variant either, only the entire enum. Things like Structs and Enums are very different than Sequences and Primitives. Without a single enum, we're free to implement whatever traits however we want, but with an enum, this becomes impossible. It's all or nothing.

pepone · 2021-11-03T22:15:36Z

slicec/src/grammar/wrappers.rs

+                match self.concrete_element() {
+                    $(Elements::$variant(_) => {
+                        // Clone the TypeRef, but downcast it's pointer to the concrete type.
+                        // TODO cloning this is expensive. There may be a better way to implement.


What is expensive about this clone?

In all honesty, nothing.
I have a very low bar for what counts as 'expensive', I try to avoid clones whenever I can.

This comment only survived because I would still like to revisit it and see if I can I improve it more.
I think I went overboard with wrappers.rs.

slicec/src/parser/comments.pest

pepone · 2021-11-03T22:22:20Z

slicec/src/parser/mod.rs

+// NOTE! it is NOT safe to call any methods on any of the slice entitites during parsing.
+// Slice entities are NOT considered fully constructed until AFTER parsing is finished (including patching).
+// Accessing ANY data, or calling ANY methods before this point may result in panics or undefined behavior.
+


This sounds pretty scary API choice

It isn't actually true : v)
A) I didn't give much attention to parser, since I'm going to drop Pest anyways.
B) The only actually unsafe things to do are try to access a parent, or resolve an underlying type.
Since we need to wait until after the patchers have run.
C) That being said, you really shouldn't be calling functions on elements while you're still parsing anyways.
D) I definitely need to tone down my comments some. Wait until you see OwnedPtr...

pepone · 2021-11-03T22:24:36Z

slicec/src/parser/parent_patcher.rs

+
+struct ParentPatcher;
+
+impl PtrVisitor for ParentPatcher {


What is this patcher about?

Many elements hold a pointer to their parent (an operation can reference it's interface for instance),
but we strictly construct elements from an inner -> outer approach. So we'll create the Operation first, and then create it's parent Interface. So when constructing the Operation there isn't yet a parent to reference.

This patcher runs after we've finished parsing, visits every element, and if it needs a reference to it's parent, it'll patch the reference in then, since everything has already been parsed.

We could do this while we're parsing, so, once we create the interface, we could immediately iterate over it's children and patch, but in my opinion, it seemed cleaner to separate this out into it's own step.

pepone

did you run rust format? I did a minor fix to slice.rs and the whole file got reformatted

slicec-cs/src/proxy_visitor.rs

slicec/src/grammar/slice.rs

pepone

Using an undefined type causes a panic, here MyEnum is not defined the compiler panics instead of reporting an error

struct MyStruct
    {
        int i;
        int j;
        MyEnum en;
    }

slicec-cs/src/slicec_ext/member_ext.rs

pepone

module Demo
{
    interface Operations
    {
        void op();
    }

    interface DerivedOperations : Operations
    {
    }
}

Compiling this Slice causes a panic, not clear why

D:\IceRPC\icerpc-csharp\examples\Hello>..\..\..\icerpc-dev\target\debug\slicec-cs.exe --output-dir Client\generated Hello.ice
thread 'main' panicked at 'called `Option::unwrap()` on a `None` value', slicec\src\ptr_util.rs:122:30
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

D:\IceRPC\icerpc-csharp\examples\Hello>..\..\..\icerpc-dev\target\debug\slicec-cs.exe --output-dir Client\generated Hello.ice
thread 'main' panicked at 'called `Option::unwrap()` on a `None` value', slicec\src\ptr_util.rs:122:30
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

D:\IceRPC\icerpc-csharp\examples\Hello>..\..\..\icerpc-dev\target\debug\slicec-cs.exe --output-dir Client\generated Hello.ice
thread 'main' panicked at 'called `Option::unwrap()` on a `None` value', slicec\src\ptr_util.rs:122:30
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

D:\IceRPC\icerpc-csharp\examples\Hello>set RUST_BACKTRACE=1

D:\IceRPC\icerpc-csharp\examples\Hello>..\..\..\icerpc-dev\target\debug\slicec-cs.exe --output-dir Client\generated Hello.ice
thread 'main' panicked at 'called `Option::unwrap()` on a `None` value', slicec\src\ptr_util.rs:122:30
stack backtrace:
   0: std::panicking::begin_panic_handler
             at /rustc/09c42c45858d5f3aedfa670698275303a3d19afa\/library\std\src\panicking.rs:517
   1: core::panicking::panic_fmt
             at /rustc/09c42c45858d5f3aedfa670698275303a3d19afa\/library\core\src\panicking.rs:101
   2: core::panicking::panic
             at /rustc/09c42c45858d5f3aedfa670698275303a3d19afa\/library\core\src\panicking.rs:50
   3: enum$<core::option::Option<ptr_const$<dyn$<slice::grammar::traits::Type> > >, 1, 18446744073709551615, Some>::unwrap<ptr_const$<dyn$<slice::grammar::traits::Type> > >
             at /rustc/09c42c45858d5f3aedfa670698275303a3d19afa\library\core\src\option.rs:735
   4: slice::ptr_util::WeakPtr<dyn$<slice::grammar::traits::Type> >::borrow<dyn$<slice::grammar::traits::Type> >
             at D:\IceRPC\icerpc-dev\slicec\src\ptr_util.rs:122
   5: slice::grammar::slice::TypeRef<dyn$<slice::grammar::traits::Type> >::definition<dyn$<slice::grammar::traits::Type> >
             at D:\IceRPC\icerpc-dev\slicec\src\grammar\slice.rs:861
   6: slice::grammar::wrappers::impl$0::concrete_element<dyn$<slice::grammar::traits::Type> >
             at D:\IceRPC\icerpc-dev\slicec\src\grammar\wrappers.rs:165
   7: slice::grammar::slice::TypeRef<dyn$<slice::grammar::traits::Type> >::concrete_type_ref<dyn$<slice::grammar::traits::Type> >
             at D:\IceRPC\icerpc-dev\slicec\src\grammar\wrappers.rs:67
   8: slice::parser::slice::SliceParser::interface_start
             at D:\IceRPC\icerpc-dev\slicec\src\parser\slice.rs:313
   9: slice::parser::slice::SliceParser::interface_def
             at D:\IceRPC\icerpc-dev\slicec\src\parser\slice.rs:327
  10: slice::parser::slice::SliceParser::definition
             at D:\IceRPC\icerpc-dev\slicec\src\parser\slice.rs:110
  11: slice::parser::slice::impl$14::module_def::closure$1
             at C:\Users\ppgut\.cargo\registry\src\github.com-1ecc6299db9ec823\pest_consume-1.1.1\src\match_nodes.rs:87
  12: core::iter::adapters::map::map_try_fold::closure$0<pest_consume::node::Node<enum$<slice::parser::slice::Rule>,ref$<core::cell::RefCell<slice::parser::slice::ParserData> > >,enum$<core::result::Result<enum$<slice::grammar::wrappers::Definition>,pest::error
             at /rustc/09c42c45858d5f3aedfa670698275303a3d19afa\library\core\src\iter\adapters\map.rs:91
  13: core::iter::traits::iterator::Iterator::try_fold<pest_consume::node::Nodes<enum$<slice::parser::slice::Rule>,ref$<core::cell::RefCell<slice::parser::slice::ParserData> > >,tuple$<>,core::iter::adapters::map::map_try_fold::closure$0,enum$<core::ops::contro
             at /rustc/09c42c45858d5f3aedfa670698275303a3d19afa\library\core\src\iter\traits\iterator.rs:1994
  14: core::iter::adapters::map::impl$2::try_fold<enum$<core::result::Result<enum$<slice::grammar::wrappers::Definition>,pest::error::Error<enum$<slice::parser::slice::Rule> > > >,pest_consume::node::Nodes<enum$<slice::parser::slice::Rule>,ref$<core::cell::RefC
             at /rustc/09c42c45858d5f3aedfa670698275303a3d19afa\library\core\src\iter\adapters\map.rs:117
  15: core::iter::adapters::impl$0::try_fold<core::iter::adapters::map::Map<pest_consume::node::Nodes<enum$<slice::parser::slice::Rule>,ref$<core::cell::RefCell<slice::parser::slice::ParserData> > >,slice::parser::slice::impl$14::module_def::closure$1>,enum$<sl
             at /rustc/09c42c45858d5f3aedfa670698275303a3d19afa\library\core\src\iter\adapters\mod.rs:177
  16: core::iter::traits::iterator::Iterator::find<core::iter::adapters::ResultShunt<core::iter::adapters::map::Map<pest_consume::node::Nodes<enum$<slice::parser::slice::Rule>,ref$<core::cell::RefCell<slice::parser::slice::ParserData> > >,slice::parser::slice::
             at /rustc/09c42c45858d5f3aedfa670698275303a3d19afa\library\core\src\iter\traits\iterator.rs:2381
  17: core::iter::adapters::impl$0::next<core::iter::adapters::map::Map<pest_consume::node::Nodes<enum$<slice::parser::slice::Rule>,ref$<core::cell::RefCell<slice::parser::slice::ParserData> > >,slice::parser::slice::impl$14::module_def::closure$1>,enum$<slice:
             at /rustc/09c42c45858d5f3aedfa670698275303a3d19afa\library\core\src\iter\adapters\mod.rs:159
  18: alloc::vec::Vec<enum$<slice::grammar::wrappers::Definition>,alloc::alloc::Global>::extend_desugared<enum$<slice::grammar::wrappers::Definition>,alloc::alloc::Global,core::iter::adapters::ResultShunt<core::iter::adapters::map::Map<pest_consume::node::Nodes
             at /rustc/09c42c45858d5f3aedfa670698275303a3d19afa\library\alloc\src\vec\mod.rs:2583
  19: alloc::vec::spec_extend::impl$0::spec_extend<enum$<slice::grammar::wrappers::Definition>,core::iter::adapters::ResultShunt<core::iter::adapters::map::Map<pest_consume::node::Nodes<enum$<slice::parser::slice::Rule>,ref$<core::cell::RefCell<slice::parser::s
             at /rustc/09c42c45858d5f3aedfa670698275303a3d19afa\library\alloc\src\vec\spec_extend.rs:18
  20: alloc::vec::spec_from_iter_nested::impl$0::from_iter<enum$<slice::grammar::wrappers::Definition>,core::iter::adapters::ResultShunt<core::iter::adapters::map::Map<pest_consume::node::Nodes<enum$<slice::parser::slice::Rule>,ref$<core::cell::RefCell<slice::p
             at /rustc/09c42c45858d5f3aedfa670698275303a3d19afa\library\alloc\src\vec\spec_from_iter_nested.rs:37
  21: alloc::vec::spec_from_iter::impl$0::from_iter<enum$<slice::grammar::wrappers::Definition>,core::iter::adapters::ResultShunt<core::iter::adapters::map::Map<pest_consume::node::Nodes<enum$<slice::parser::slice::Rule>,ref$<core::cell::RefCell<slice::parser::
             at /rustc/09c42c45858d5f3aedfa670698275303a3d19afa\library\alloc\src\vec\spec_from_iter.rs:33
  22: alloc::vec::impl$18::from_iter<enum$<slice::grammar::wrappers::Definition>,core::iter::adapters::ResultShunt<core::iter::adapters::map::Map<pest_consume::node::Nodes<enum$<slice::parser::slice::Rule>,ref$<core::cell::RefCell<slice::parser::slice::ParserDa
             at /rustc/09c42c45858d5f3aedfa670698275303a3d19afa\library\alloc\src\vec\mod.rs:2486
  23: core::iter::traits::iterator::Iterator::collect<core::iter::adapters::ResultShunt<core::iter::adapters::map::Map<pest_consume::node::Nodes<enum$<slice::parser::slice::Rule>,ref$<core::cell::RefCell<slice::parser::slice::ParserData> > >,slice::parser::slic
             at /rustc/09c42c45858d5f3aedfa670698275303a3d19afa\library\core\src\iter\traits\iterator.rs:1745
  24: core::result::impl$35::from_iter::closure$0<enum$<slice::grammar::wrappers::Definition>,pest::error::Error<enum$<slice::parser::slice::Rule> >,alloc::vec::Vec<enum$<slice::grammar::wrappers::Definition>,alloc::alloc::Global>,core::iter::adapters::map::Map
             at /rustc/09c42c45858d5f3aedfa670698275303a3d19afa\library\core\src\result.rs:1887
  25: core::iter::adapters::process_results<core::iter::adapters::map::Map<pest_consume::node::Nodes<enum$<slice::parser::slice::Rule>,ref$<core::cell::RefCell<slice::parser::slice::ParserData> > >,slice::parser::slice::impl$14::module_def::closure$1>,enum$<sli
             at /rustc/09c42c45858d5f3aedfa670698275303a3d19afa\library\core\src\iter\adapters\mod.rs:148
  26: core::result::impl$35::from_iter<enum$<slice::grammar::wrappers::Definition>,pest::error::Error<enum$<slice::parser::slice::Rule> >,alloc::vec::Vec<enum$<slice::grammar::wrappers::Definition>,alloc::alloc::Global>,core::iter::adapters::map::Map<pest_consu
             at /rustc/09c42c45858d5f3aedfa670698275303a3d19afa\library\core\src\result.rs:1887
  27: core::iter::traits::iterator::Iterator::collect<core::iter::adapters::map::Map<pest_consume::node::Nodes<enum$<slice::parser::slice::Rule>,ref$<core::cell::RefCell<slice::parser::slice::ParserData> > >,slice::parser::slice::impl$14::module_def::closure$1>
             at /rustc/09c42c45858d5f3aedfa670698275303a3d19afa\library\core\src\iter\traits\iterator.rs:1745
  28: slice::parser::slice::SliceParser::module_def
             at D:\IceRPC\icerpc-dev\slicec\src\parser\slice.rs:135
  29: slice::parser::slice::impl$14::main::closure$1
             at C:\Users\ppgut\.cargo\registry\src\github.com-1ecc6299db9ec823\pest_consume-1.1.1\src\match_nodes.rs:87
  30: core::iter::adapters::map::map_try_fold::closure$0<pest_consume::node::Node<enum$<slice::parser::slice::Rule>,ref$<core::cell::RefCell<slice::parser::slice::ParserData> > >,enum$<core::result::Result<slice::grammar::slice::Module,pest::error::Error<enum$<
             at /rustc/09c42c45858d5f3aedfa670698275303a3d19afa\library\core\src\iter\adapters\map.rs:91
  31: core::iter::traits::iterator::Iterator::try_fold<pest_consume::node::Nodes<enum$<slice::parser::slice::Rule>,ref$<core::cell::RefCell<slice::parser::slice::ParserData> > >,tuple$<>,core::iter::adapters::map::map_try_fold::closure$0,enum$<core::ops::contro
             at /rustc/09c42c45858d5f3aedfa670698275303a3d19afa\library\core\src\iter\traits\iterator.rs:1994
  32: core::iter::adapters::map::impl$2::try_fold<enum$<core::result::Result<slice::grammar::slice::Module,pest::error::Error<enum$<slice::parser::slice::Rule> > > >,pest_consume::node::Nodes<enum$<slice::parser::slice::Rule>,ref$<core::cell::RefCell<slice::par
             at /rustc/09c42c45858d5f3aedfa670698275303a3d19afa\library\core\src\iter\adapters\map.rs:117
  33: core::iter::adapters::impl$0::try_fold<core::iter::adapters::map::Map<pest_consume::node::Nodes<enum$<slice::parser::slice::Rule>,ref$<core::cell::RefCell<slice::parser::slice::ParserData> > >,slice::parser::slice::impl$14::main::closure$1>,slice::grammar
             at /rustc/09c42c45858d5f3aedfa670698275303a3d19afa\library\core\src\iter\adapters\mod.rs:177
  34: core::iter::traits::iterator::Iterator::find<core::iter::adapters::ResultShunt<core::iter::adapters::map::Map<pest_consume::node::Nodes<enum$<slice::parser::slice::Rule>,ref$<core::cell::RefCell<slice::parser::slice::ParserData> > >,slice::parser::slice::
             at /rustc/09c42c45858d5f3aedfa670698275303a3d19afa\library\core\src\iter\traits\iterator.rs:2381
  35: core::iter::adapters::impl$0::next<core::iter::adapters::map::Map<pest_consume::node::Nodes<enum$<slice::parser::slice::Rule>,ref$<core::cell::RefCell<slice::parser::slice::ParserData> > >,slice::parser::slice::impl$14::main::closure$1>,slice::grammar::sl
             at /rustc/09c42c45858d5f3aedfa670698275303a3d19afa\library\core\src\iter\adapters\mod.rs:159
  36: alloc::vec::spec_from_iter_nested::impl$0::from_iter<slice::grammar::slice::Module,core::iter::adapters::ResultShunt<core::iter::adapters::map::Map<pest_consume::node::Nodes<enum$<slice::parser::slice::Rule>,ref$<core::cell::RefCell<slice::parser::slice::
             at /rustc/09c42c45858d5f3aedfa670698275303a3d19afa\library\alloc\src\vec\spec_from_iter_nested.rs:23
  37: alloc::vec::spec_from_iter::impl$0::from_iter<slice::grammar::slice::Module,core::iter::adapters::ResultShunt<core::iter::adapters::map::Map<pest_consume::node::Nodes<enum$<slice::parser::slice::Rule>,ref$<core::cell::RefCell<slice::parser::slice::ParserD
             at /rustc/09c42c45858d5f3aedfa670698275303a3d19afa\library\alloc\src\vec\spec_from_iter.rs:33
  38: alloc::vec::impl$18::from_iter<slice::grammar::slice::Module,core::iter::adapters::ResultShunt<core::iter::adapters::map::Map<pest_consume::node::Nodes<enum$<slice::parser::slice::Rule>,ref$<core::cell::RefCell<slice::parser::slice::ParserData> > >,slice:
             at /rustc/09c42c45858d5f3aedfa670698275303a3d19afa\library\alloc\src\vec\mod.rs:2486
  39: core::iter::traits::iterator::Iterator::collect<core::iter::adapters::ResultShunt<core::iter::adapters::map::Map<pest_consume::node::Nodes<enum$<slice::parser::slice::Rule>,ref$<core::cell::RefCell<slice::parser::slice::ParserData> > >,slice::parser::slic
             at /rustc/09c42c45858d5f3aedfa670698275303a3d19afa\library\core\src\iter\traits\iterator.rs:1745
  40: core::result::impl$35::from_iter::closure$0<slice::grammar::slice::Module,pest::error::Error<enum$<slice::parser::slice::Rule> >,alloc::vec::Vec<slice::grammar::slice::Module,alloc::alloc::Global>,core::iter::adapters::map::Map<pest_consume::node::Nodes<e
             at /rustc/09c42c45858d5f3aedfa670698275303a3d19afa\library\core\src\result.rs:1887
  41: core::iter::adapters::process_results<core::iter::adapters::map::Map<pest_consume::node::Nodes<enum$<slice::parser::slice::Rule>,ref$<core::cell::RefCell<slice::parser::slice::ParserData> > >,slice::parser::slice::impl$14::main::closure$1>,slice::grammar:
             at /rustc/09c42c45858d5f3aedfa670698275303a3d19afa\library\core\src\iter\adapters\mod.rs:148
  42: core::result::impl$35::from_iter<slice::grammar::slice::Module,pest::error::Error<enum$<slice::parser::slice::Rule> >,alloc::vec::Vec<slice::grammar::slice::Module,alloc::alloc::Global>,core::iter::adapters::map::Map<pest_consume::node::Nodes<enum$<slice:
             at /rustc/09c42c45858d5f3aedfa670698275303a3d19afa\library\core\src\result.rs:1887
  43: core::iter::traits::iterator::Iterator::collect<core::iter::adapters::map::Map<pest_consume::node::Nodes<enum$<slice::parser::slice::Rule>,ref$<core::cell::RefCell<slice::parser::slice::ParserData> > >,slice::parser::slice::impl$14::main::closure$1>,enum$
             at /rustc/09c42c45858d5f3aedfa670698275303a3d19afa\library\core\src\iter\traits\iterator.rs:1745
  44: slice::parser::slice::SliceParser::main
             at D:\IceRPC\icerpc-dev\slicec\src\parser\slice.rs:101
  45: slice::parser::slice::SliceParser::parse_file
             at D:\IceRPC\icerpc-dev\slicec\src\parser\slice.rs:82
  46: slice::parser::slice::SliceParser::try_parse_file
             at D:\IceRPC\icerpc-dev\slicec\src\parser\slice.rs:55
  47: slice::parser::parse_files
             at D:\IceRPC\icerpc-dev\slicec\src\parser\mod.rs:39
  48: slice::parse_from_options
             at D:\IceRPC\icerpc-dev\slicec\src\lib.rs:25
  49: slicec_cs::try_main
             at D:\IceRPC\icerpc-dev\slicec-cs\src\main.rs:51
  50: slicec_cs::main
             at D:\IceRPC\icerpc-dev\slicec-cs\src\main.rs:42
  51: core::ops::function::FnOnce::call_once<void (*)(),tuple$<> >
             at /rustc/09c42c45858d5f3aedfa670698275303a3d19afa\library\core\src\ops\function.rs:227
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.

pepone · 2021-11-04T10:16:47Z

I got similar crashes with class, exception inheritance

pepone · 2021-11-04T10:21:01Z

Stream params no longer compile, I thought this already worked

module Demo
{
    interface Operations
    {
        stream byte opStreamByteReceive0();
    }
}

fails with:

error:  --> 9:9
  |
9 |         stream byte opStreamByteReceive0();␍␊
  |         ^---
  |
  = expected operation
Compilation failed with 1 error(s) and 0 warning(s).

pepone · 2021-11-04T11:23:25Z

enum MyEnum
    {
        enum1,
    }

    interface EnumOperations
    {
        [cs:generic(List)]sequence<MyEnum> opMyEnumSeq([cs:generic(List)]sequence<MyEnum> p1);
    }

There is a regression with the generated code for this seq

with dev this generates

(encoder, value) => encoder.EncodeSequence(value, (encoder, value) => MyEnumHelper.EncodeMyEnum(encoder, value)));

same with C++ compiler

(encoder, value) => encoder.EncodeSequence(value, (encoder, value) => MyEnumHelper.EncodeMyEnum(encoder, value)))

with this PR code becomes

(encoder, value) => encoder.EncodeSequence(value.Span))

slicec-cs/src/slicec_ext/operation_ext.rs

slicec-cs/src/dispatch_visitor.rs

pepone · 2021-11-04T10:37:56Z

slicec-cs/src/dispatch_visitor.rs

        .iter()
-        .filter(|o| o.has_non_streamed_return(ast))
+        .filter(|o| o.has_nonstreamed_return_members())
        .cloned()


Also not related to this PR but we don't need a Response.XXX method for operations that use encoded-result, we should filter them above too.

slicec/src/grammar/slice.rs

InsertCreativityHere · 2021-11-04T13:30:09Z

Using an undefined type causes a panic

Yep. This PR isn't nearly at the point I want it to be, but I was told that the only milestone to hit was getting a compiler that compiles valid slice files, that way we can swap out compilers as soon as possible. If you hand this an incorrect compiler, all bets are off. Obviously, I'm going to improve this, but right now error reporting and detection is almost non-existent.

I got similar crashes with class, exception inheritance

Should be fixed now. We try to validate the type of the inherited element (to make sure Classs only inherit from Class, etc) while parsing, before we've actually patched the types. I already moved this check to occur while we're patching types, but accidentally left the old check in the parser.

Stream params no longer compile, I thought this already worked

Yes, there was a bug with the whitespace sensitivity I patched on 'dev' but forgot to carry over. Fixed.

slicec-cs/src/dispatch_visitor.rs

externl

Need to do some more testing with actual slice files.

externl · 2021-11-04T15:31:23Z

slicec-cs/src/slicec_ext/operation_ext.rs

            self.escape_identifier()
        )
    }

    fn format_type(&self) -> String {
        // TODO: Austin - Implement this :)
+        // I don't even know what this is though!


I can implement this once this PR is merged.

slicec/src/ast.rs

externl · 2021-11-04T20:46:47Z

slicec/src/grammar/util.rs

@@ -0,0 +1,105 @@
+// Copyright (c) ZeroC, Inc. All rights reserved.
+
+use std::fmt;


Maybe just spilt this file into scope.rs and tag_format.rs.

slicec/src/parser/comments.rs

externl · 2021-11-04T21:00:39Z

I was thinking about how we lookup things. Is there any concern for "ambiguity" in cases like:

// File1.ice
module M {
    struct S {}
}

// File2.ice
module M1 {
    module M {
        struct S{}
    }
}

// File3.ice
module M1::M2::M {
    struct S {}
}

//File4.ice
module M1::M2 {   
    struct Foo {
        s: M::S
    }
}

The user may find it very confusing which M::S they're getting.

InsertCreativityHere · 2021-11-05T00:44:55Z

From an algorithmic perspective, there is no ambiguity.
In your example, s will have the type of ::M1::M2::M::S.

The scope of the data member is M1::M2, and the type it looks for is M::S. Our first attempt is to 'concenate' and check. In this case it exists. If it didn't, we go back a scope, and check again, and so on until global scope.
In this case, the types we'll check for are, in order:
::M1::M2::M::S
::M1::M::S
::M::S
Error
In this case, all of those exist, but we stop when we reach the first match (the most derived scope).

I documented the type resolution process in grammar/ast.rs: https://github.com/InsertCreativityHere/icerpc/blob/9226ade1124b240b2430ae7e18e9d44f6afd9b30/src/ast.rs#L193-L246
Let me know if you think any parts could be improved, or don't make sense.
ast.rs is one of the few files that I consider fully done, so give it extra scrutiny!

The user may find it very confusing which M::S they're getting.

I don't think is really a problem that we can solve, apart from having good documentation.
In this case, frankly, it's up to the user to solve their own problem: being horrible at writing APIs : v)

…string'.

externl · 2021-11-08T16:45:53Z

with this PR code becomes

(encoder, value) => encoder.EncodeSequence(value.Span))

Seems to be working correctly for me

(encoder, value) => encoder.EncodeSequence(value, (encoder, value) => MyEnumHelper.EncodeMyEnum(encoder, value)));

InsertCreativityHere · 2021-11-08T17:00:46Z

Interestingly, fixing the bug has caused the code to break like how Jose said it should be.
Now neither responses or requests generate the correct code...

InsertCreativityHere · 2021-11-08T19:29:58Z

I figured out why the encode_action for sequences was broken and pushed a fix. Let me know if works for you now @pepone.

While debugging, I was accidentally looking at the wrong function most of the time.
We've already fallen into the issue of having multiple, nearly identical functions scattered in the compiler:

has_fixed_size_numeric_elements:
https://github.com/InsertCreativityHere/icerpc/blob/50be8afb98f94b895e1d8e7b131124c74bb961c6/slicec/src/grammar/slice.rs#L858
and is_fixed_size_numeric_sequence:
https://github.com/InsertCreativityHere/icerpc/blob/50be8afb98f94b895e1d8e7b131124c74bb961c6/slicec-cs/src/slicec_ext/type_ref_ext.rs#L105

The 2nd function seems incorrect. It doesn't check if the underlying data is numeric, only if it's fixed size. Either it's logic is wrong, or it's name is.

More importantly, do we really need 2 functions here? They seem identical to me...

…eration attributes to single return types.

externl · 2021-11-08T19:39:19Z

I figured out why the encode_action for sequences was broken and pushed a fix. Let me know if works for you now Jose.

While debugging, I was accidentally looking at the wrong function most of the time. We've already fallen into the issue of having multiple, nearly identical functions scattered in the compiler:

has_fixed_size_numeric_elements: https://github.com/InsertCreativityHere/icerpc/blob/50be8afb98f94b895e1d8e7b131124c74bb961c6/slicec/src/grammar/slice.rs#L858 and is_fixed_size_numeric_sequence: https://github.com/InsertCreativityHere/icerpc/blob/50be8afb98f94b895e1d8e7b131124c74bb961c6/slicec-cs/src/slicec_ext/type_ref_ext.rs#L105

The 2nd function seems incorrect. It doesn't check if the underlying data is numeric, only if it's fixed size. Either it's logic is wrong, or it's name is.

More importantly, do we really need 2 functions here? They seem identical to me...

Pretty sure we can just delete this second ext one.

InsertCreativityHere · 2021-11-08T19:52:16Z

Okay, I'll just delete it for now. But you should still take a look to make sure it's okay to delete @pepone.

I think I've fixed all the things you both pointed out so far.
Let me know if you find anything else!

Leftover things I didn't fix:

Switch bit_sequence_index to be an Option<u32> instead of an i64 (Re-implemented 'slicec' #50 (comment))
Should we recursively check for deprecation? (Re-implemented 'slicec' #50 (comment))
Don't generate Response method for operations with encoded-result. (Re-implemented 'slicec' #50 (comment))
Split up 'grammar/util.rs' file (Re-implemented 'slicec' #50 (comment))

pepone · 2021-11-08T20:18:44Z

Would be good to run cargo fmt in the repository.

slicec-cs/src/proxy_visitor.rs

Co-authored-by: Jose <pepone@users.noreply.github.com>

Re-implemented 'slicec'.

b94cc36

InsertCreativityHere added the enhancement New feature or request label Nov 3, 2021

InsertCreativityHere requested review from pepone and externl November 3, 2021 19:53

Qualified grammar module re-exports to avoid ambiguity.

c249aa9

externl reviewed Nov 3, 2021

View reviewed changes

pepone reviewed Nov 3, 2021

View reviewed changes

InsertCreativityHere added 5 commits November 3, 2021 20:15

Fixed some review comments.

4cd6c26

Gutted useless passthrugh functions from wrapper types.

d306bca

Fixed bug in 'decode_operation' call.

31eb29f

TypeAlias holds it's own scope now.

5c425de

Minor linter fixes.

d60733d