flatbuffers icon indicating copy to clipboard operation
flatbuffers copied to clipboard

Fix Rust codegen escaping field in tables.

Open CasperN opened this issue 2 years ago • 1 comments

Fixes #7617 - the problem is that if you have a union in a table with the field name "type", the Rust code-gen escaped "type" to "type_" before appending "_type"` which does not equal the magic field "type_type" which is generated by flatc.

CasperN avatar Nov 22 '22 21:11 CasperN

@dbaileychess hey can you take a look at the CI failures? I can't quite figure out what's broken, though it appears unrelated to Rust changes

CasperN avatar Nov 28 '22 14:11 CasperN

Hi @CasperN , The problem is when we have a field that match with one of keyword, the namer add suffix (underline here). As in the comments describe the problem previously. On the other hand, the parser add an auto-generated field for union type. That means, we have 2 field per Union usage. one of them has exact name of the field, the other have name + suffix (_type) link. So we have 2 method for union, one return Union type, one pointer to union field.

#[inline]
  pub fn type_type(&self) -> KeywordsInUnion {
    // Safety:
    // Created from valid Table for this object
    // which contains a valid value in this slot
    unsafe { self._tab.get::<KeywordsInUnion>(Table2::VT_TYPE_TYPE, Some(KeywordsInUnion::NONE)).unwrap()}
  }
  #[inline]
  pub fn type_(&self) -> Option<flatbuffers::Table<'a>> {
    // Safety:
    // Created from valid Table for this object
    // which contains a valid value in this slot
    unsafe { self._tab.get::<flatbuffers::ForwardsUOffset<flatbuffers::Table<'a>>>(Table2::VT_TYPE_, None)}
  }

As you can see the second one match with keyword and get an underline suffix.

Now, When code generator wants to use first method to get union type in code, by mistake, take the second one, as result we add "_type" suffix to fix the problem, but we should have use the second one and it does not need to add suffix and use another variable name DISCRIMINAT.

 #[inline]
  #[allow(non_snake_case)]
  pub fn type__as_static_(&self) -> Option<KeywordsInTable<'a>> {
    if self.type_type() == KeywordsInUnion::static_ {
      self.type_().map(|t| {
       // Safety:
       // Created from a valid Table for this object
       // Which contains a valid union in this slot
       unsafe { KeywordsInTable::init_from_table(t) }
     })
    } else {
      None
    }
  }

The same problem exist in c++ code generator. example of fbs:

table Null {                                                                    
}                                                                               
                                                                                
union DType {                                                                   
  Null,                                                                         
}                                                                               
                                                                                
table Field {                                                                   
  /// This is the type of the decoded value if the field is dictionary encoded. 
  static: DType;                                                                
  saman: DType;                                                                 
} 

the result should be something like this:

  DType static_type() const {                                                   
    return static_cast<DType>(GetField<uint8_t>(VT_STATIC_TYPE, 0));            
  }                                                                             
  /// This is the type of the decoded value if the field is dictionary encoded. 
  const void *static_() const {                                                 
    return GetPointer<const void *>(VT_STATIC_);                                
  } 
  template<typename T> const T *static__as() const;                             
  const Null *static__as_Null() const {                                         
    // <= static__type is WRONG !!!
    return static__type() == DType_Null ? static_cast<const Null *>(static_()) : nullptr;
  }

IMO the best way to fix these problems is to pick the correct method. cc: @dbaileychess

enum-class avatar Nov 29 '22 23:11 enum-class

@dbaileychess hey can you take a look at the CI failures? I can't quite figure out what's broken, though it appears unrelated to Rust changes

I made some fixes to the MacOS CI yesterday, you just have to update (rebase/merge) to trigger it again.

dbaileychess avatar Nov 29 '22 23:11 dbaileychess

@dbaileychess can you approve this?

CasperN avatar Dec 08 '22 05:12 CasperN

Ping @dbaileychess for LGTM

CasperN avatar Dec 13 '22 04:12 CasperN

Sorry for the delay!

dbaileychess avatar Dec 13 '22 05:12 dbaileychess