Improve type constraints on `AcmeConfig` methods to allow chaining

[joshtriplett]

By requiring the type parameters of AcmeConfig to match the associated types of Cache, rustc can work backwards from a subsequent call to .cache to determine the type of AcmeConfig. This allows chaining methods directly after AcmeConfig::new(...).

[FlorianUekermann]

Both approaches have problems. In my original version you need to state the type twice (let config: AcmeConfig = AcmeConfig::new(...)) to make inference from defaults work, due to lack of progress on this Rust issue here. The workaround is a admittedly a little ugly, but doesn't usually add extra loc. Adding an explicit Code example demonstrating the problem and solution in the docs of AcmeConfig is probably a good idea if I stick with this.

Removing the default type parameters makes statements that include specifying a cache prettier, but relies on their presence for type inference. But if you remove the line involving the cache you need to specify the types, which makes it even less accessible.

I see a couple of solutions that may be better than what's currently published as beta:

1. Add a type alias with fully specified parameters type AcmeConfig = AcmeConfigGeneric<Infallible, Infallible>.
2. Add a proper builder type instead of using the config as a builder. Specifying the cache would be the final call on the builder and yield a config. This is technically very similar to 1, but may be more intuitive (type AcmeConfigBuilder = AcmeConfig<Infallible, Infallible> may even be enough, instead of adding a real builder type)
3. Introduce a fn new_config() -> AcmeConfig<Infallible, Infallible>
4. Include the cache parameter the constructor AcmeConfig::new(domains: Vec<String>, cache: Cache). At least I hope that solves inference.

[FlorianUekermann]

Btw, I'm a bit surprised that removing the default solves inference if a cache method is part of the statement. I didn't expect that the default types make inference even worse.

[joshtriplett]

Removing the default type parameters makes statements that include specifying a cache prettier, but relies on their presence for type inference.

Pretty much every usage of this needs to specify a cache of some kind. We could add a no_cache() method (with documented caveats), and have that one nail down the types as well. Or we could add a new_uncached() constructor, which nails down the types.

(Also, the removal of the Infallible default isn't required for this change; I just removed it because it didn't seem needed anymore. I can add that default back, and then cases without a cache end up being no worse than they were before this change, but I think the no_cache() or new_uncached() approaches above would be simpler and less error-prone for users, by avoiding the need for a type annotation on the let binding, and avoiding the need for a let binding at all.)

[joshtriplett]

I just updated this change to add the Infallible default back, which doesn't affect the cached case.

I also added a second commit that re-removes the Infallible default and adds a new_uncached constructor, which makes the uncached case better by not requiring a type annotation on the let binding (and not requiring a let binding at all, which is convenient when passing AcmeConfig as a parameter to something).

Thoughts?

[FlorianUekermann]

I do like how that first commit fixes the inference. But the fact that there's no way to change the cache to one with a different type bothers me. Maybe that is just a matter of adding one more method.

I'm still unsure about this... ...I wish there was a more predictable timeline for (https://github.com/rust-lang/rust/issues/27336) to be solved on the Rust side. Then I would just stick with what we have already.

[FlorianUekermann]

After playing around with how the different proposals feel in different situations, I opted for the type alias idea (with some documentation) and released beta3.

I'll keep this open for the moment in case I missed something. Please try it out and see how you feel about it. This should work:

let config = AcmeBuilder::new(vec!["example.com".to_string()])
    .cache(DirCache::new("./rustls_acme_cache"));

But this should also be fine.

let config = AcmeBuilder::new(vec!["example.com".to_string()]);

If RFC213 ever gets unstuck and makes it into stable, I'll deprecate AcmeBuilder.

There may be value in also providing cache_mut() -> &mut Box<dyn Cache<EC, EA>> to have a solution for situations where the user only holds a mutable reference and can't move out of the AcmeConfig instance, but I haven't seen evidence of that yet.

[joshtriplett]

But the fact that there's no way to change the cache to one with a different type bothers me.

I would be surprised if users of this interface ever need to change the cache in an existing AcmeConfig object after initially configuring it. Setup occurs once, and then the resulting Config gets used. Users might mutate their cache object, but I don't think they're likely to replace it with something that's a different type, and if they do want that they could box.

Given that, I don't think that case is worth complicating the interface.

The AcmeBuilder alias does address the problem somewhat, but it feels like added complexity exposed to the user, to have to distinguish between AcmeBuilder and AcmeConfig.

I think I might have another possible approach that would improve the ergonomics and still allow for changing caches; I'll try an experiment and see what I can come up with.

(And yes, I would love to have default type parameter fallback too, for many different purposes.)

[joshtriplett]

@FlorianUekermann I managed to rework this to avoid the need for AcmeBuilder entirely, while still making inference work automatically, and without preventing the user from changing cache types.

I realized that most of the problem arises because AcmeConfig::new always creates a config that initially has no cache, but doesn't set the type parameters to reflect that, so chaining doesn't know the initial type. By making AcmeConfig::new always create an AcmeConfig<Infallible, Infallible> initially, that gives the compiler enough information, and then the cache methods can all change the types.

I think this provides a simpler interface that's easy to use and doesn't add any constraints. What do you think?

[FlorianUekermann]

I would be surprised if users of this interface ever need to change the cache in an existing AcmeConfig object ...

Yes and no. It won't be very common. But this already bites if the user for whatever reason decides to splits the config stuff into multiple statements or returns one without cache from a function (without making the function generic over the error types). Especially for less experienced users this becomes unwieldy pretty quickly.

I do like your idea of removing the alias type by making new less generic. I didn't do that because it makes constructing AcmeConfig with non-Infallible errors less obvious without adding a cache. But I can just add a few lines of docs to new hinting at let config: AcmeConfig<EC, EA> = AcmeConfig::new(domains).config(NoCache::<EC, EA>::new())