Azure / azure-sdk-for-rust

This repository is for active development of the *unofficial* Azure SDK for Rust. This repository is *not* supported by the Azure SDK team.
MIT License
685 stars 232 forks source link

Handling "required" arguments moving to "optional" without being a breaking change #1582

Open demoray opened 6 months ago

demoray commented 6 months ago

During internal discussions earlier today, it was brought up that the SDK architects want to be able to change required arguments to optional without making it a breaking change.

Consider the method foo which had 3 arguments of a, b, and c where a and b were required. Today, this would look like the following [^1]:


impl Client {
   fn foo(&self, a: String, b: String) -> FooBuilder {
   }
}

struct FooBuilder {
   a: String,
   b: String,
   c: Option<STring>
}

impl FooBuilder {
    fn new(a: String, b: String) -> Self {
        Self { a, b, c: None }
    }
    fn c(mut self, c: Option<String>) -> Self {
        self.c = c;
        self
    }
}

What was discussed was the need enable making b optional for future code, while also not breaking code that already sets b.

This issue is to discuss potential ways we could do this without making everything an Option.

[^1]: Note: In practice, we use the operation! macro to implement FooBuilder and a handful of other useful components, but this is what the macro expands out for the builder.

demoray commented 6 months ago

One potential solution would be to move to a type-state builder pattern, such that all parameters must be set via builder methods. This could be implemented something like this:

enum Set {}
enum Unset {}

struct <A, B> FooBuilder<A, B> {
    a: Option<String>,
    b: Option<String>,
    c: Option<String>,
    typestate: PhantomData<(A, B)>,
}

impl FooBuilder<Unset, Unset> {
     fn new() -> Self {
          Self { a: None, b: None, c: None, typestate: PhantomData, }
     }
}

impl<A> FooBuilder<A, Unset> {
    fn a(self, a: Into<String>) -> Self<A, Set> {
        self.a = Some(a.into());
        self
    }
}

impl<B> FooBuilder<Unset, B> {
    fn b(self, b: Into<String>) -> Self<Set, B> {
        self.b = Some(b.into());
        self
    }
}

impl<A, B> FooBuilder<A, B> {
    fn c(self, C: Into<String>) -> Self<A, B> {
        self.c = Some(c.into());
        self
    }
}

impl <Set, Set> FooBuilder<Set, Set> {
    fn build(self) -> (...) {
        ....
    }
}

Consumers of FooBuilder would look something like this:

FooBuilder::new().a("hi").b("there").c("mom");

Until the b setter was set, the code would fail to compile.

Then in the future, when b becomes optional, we could adjust to this:

enum Set {}
enum Unset {}

struct <A> FooBuilder<A> {
    a: Option<String>,
    b: Option<String>,
    c: Option<String>,
    typestate: PhantomData<(A)>,
}

impl FooBuilder<Unset> {
     fn new() -> Self {
          Self { a: None, b: None, c: None, typestate: PhantomData, }
     }
     fn a(self, a: Into<String>) -> Self<A, Set> {
        self.a = Some(a.into());
        self
    }
}

impl<A> FooBuilder<A> {
    fn b(self, b: Into<String>) -> Self<Set, B> {
        self.b = Some(b.into());
        self
    }
    fn c(self, C: Into<String>) -> Self<A, B> {
        self.c = Some(c.into());
        self
    }
}

impl <Set> FooBuilder<Set> {
    fn build(self) -> (...) {
        ....
    }
}

The previous consumer code, FooBuilder::new().a("hi").b("there").c("mom"); referenced above would still compile. However, now consumer code that uses this FooBuilder::new().a("hi").c("mom"); would now also compile.

demoray commented 6 months ago

While this isn't strictly semver-compatible, it does reduce the code churn required by the consumer of the API when we make something go from required to optional, which I believe is the underlying intent of the architects.