conflicting names when package is implicit

[stan-is-hate]

The issue happens when a message package is implicit, but message itself is defined in a different file from the request/response rpcs, like in this case we have Rectangle message defined in the separate primary/rectangle.proto file.

[rholshausen]

You should avoid using unwraps, as they will case a panic if there is no value. Panics aren't always handled like exceptions, some environments (like embedded) they result in process aborts.

Unwraps are ok to use if you know for certain they contain a value or they are used in a test, as that will just cause the test to fail if there is no value.

Better to use the ? operator (as the function returns a result). This will cause it to bail from the function if it is an error, otherwise just unwrap the value.

[stan-is-hate]

done

[rholshausen]

For debug statements, you can use the debug output instead of tying to unwrap the values
i.e. debug!("Checking file descriptor '{:?}' with package '{:?}'", descriptor.name, descriptor.package)'

[rholshausen]

Not sure about this return value. I tend to try avoid using lifetimes, unless I want to return a reference for performance reasons.

You are calling cloned on the iterator below, so the values are being cloned anyway, and then return a vector of references. Might as well as remove the references and lifetime and just clone the collection at the end.

[stan-is-hate]

I'm getting a bit confused by this if I'm honest. Can you please provide an example? Rust ownership logic is very new to me.

[stan-is-hate]

If I leave the code as is, and remove lifetime specified (which AI suggested lol), I'm getting this error:

missing lifetime specifier
this function's return type contains a borrowed value, but the signature does not say whether it is borrowed from `package` or one of `all_descriptors`'s 2 lifetimes
...
utils.rs(141, 45): consider introducing a named lifetime parameter: `<'a>`, `'a `, `'a `, `'a `, `'a `

[rholshausen]

I'll try to explain.

The return type Vec<&'a FileDescriptorProto> is a vector of pointers to FileDescriptorProto objects. Rust forces you to define a lifetime for these pointers to guarantee that they will never live longer that then data they are pointing to (i.e. have dangling pointers). It does this by tying the lieftime of the pointers to the passed in HashMap &'a HashMap<String, &FileDescriptorProto>. So once the destructor for all_descriptors is run, the compiler will not let you use those pointers.

However, lower done the function cloned() is being called on the iteration, which clones every item in iteration (actually looking at the types in RustRover, it is just cloning the pointers). So if you're going to clone the items, you might as well just create a new Vector of cloned objects and return that. Then you don't need any lifetime specifiers.

So the function definition becomes:

fn find_all_file_descriptors_for_package(
    package: &str,
    all_descriptors: &HashMap<String, &FileDescriptorProto>,
) -> anyhow::Result<Vec<FileDescriptorProto>>

see, no pointers and no lifetimes

and lower down we change the cloned() call to something like

        .map(|fd| *fd.clone()) // fd here is a pointer to a pointer here, so we must first de-reference it
        .collect(); // No longer need the type defined here, Rust will infer it

[rholshausen]

The reason that cloned() won't work, as the HashMap values are references, and the values() function returns an iterator over references to the values, which ends up being double references. You can see these if you use RustRover

[stan-is-hate]

thanks for the detailed explanation!

[stan-is-hate]

This makes a lot of sense, if we're cloning the values we don't need lifetime specifiers, and if we're keeping lifetime, we should only be cloning references (which is what was happening?).
However, when I change my code like so:

fn filter_file_descriptors<F>(all_descriptors: &HashMap<String, &FileDescriptorProto>, filter: F
) -> Vec<FileDescriptorProto>
where
    F: FnMut(&&&FileDescriptorProto) -> bool,
{
    all_descriptors.values()
        .filter(filter)
        .map(|d| *d.clone())
        .collect()
}

I'm getting the following error:

cannot move out of a shared reference
move occurs because value has type `FileDescriptorProto`, which does not implement the `Copy` trait

I'm sure I'm missing something here

[rholshausen]

This compiles for me:

fn find_all_file_descriptors_with_no_package(
  all_descriptors: &HashMap<String, &FileDescriptorProto>
) -> anyhow::Result<Vec<FileDescriptorProto>> {
  let found = filter_file_descriptors(all_descriptors, |d| d.package.is_none());
  if found.is_empty() {
      Err(anyhow!("Did not find any file descriptors with no package specified"))
  } else {
      debug!("Found {} file descriptors with no package", found.len());
      Ok(found)
  }
}

fn filter_file_descriptors<F>(
    all_descriptors: &HashMap<String, &FileDescriptorProto>,
    filter: F
) -> Vec<FileDescriptorProto>
where
    F: FnMut(&&FileDescriptorProto) -> bool
{
    all_descriptors.values()
        .map(|fd| *fd) // Convert &&FileDescriptorProto -> &FileDescriptorProto
        .filter(filter)
        .cloned()
        .collect()
}

[stan-is-hate]

Huh that's some high level pointer magic right there :) Thanks!

[rholshausen]

Looks ok to me. I think I understand what it is doing, and that makes sense to me.

[rholshausen]

There are some failing tests

failures:
    protobuf::tests::build_embedded_message_field_value_with_repeated_field_configured_from_map_test
    protobuf::tests::build_embedded_message_field_value_with_repeated_field_configured_from_map_with_eachvalue_test
    protobuf::tests::construct_protobuf_interaction_for_service_returns_error_on_invalid_request_type
    protobuf::tests::construct_protobuf_interaction_for_service_supports_string_value_type

[rholshausen]

I think those tests just need packages added to the test data.

[stan-is-hate]

Some tests are still failing, gonna take a look at those.