CosmWasm / sylvia

CosmWasm smart contract framework
Apache License 2.0
93 stars 14 forks source link

Silence `clippy::new_without_default` #394

Closed jawoznia closed 1 month ago

jawoznia commented 2 months ago

Sylvia contracts requires the constructor to generate dispatch properly.

Two approaches to that problem were new and Default.

We chose new method, because we were able to validate it's existence in the compile time and inform user about the missing method. Also this way we were able to provide some input into the new method, although this turned out to be unnecessary and we end up only with the first benefit.

Although we could switch to requiring user to implement the Default trait on their contracts, I think because of the compile time validation, and because it would be breaking for the users I suggest to keep the new method requirement and instead make contract macro auto-generate Default implementation.

I think this should be considered breaking, as users most likely provided this implementation themselves to silence clippy, thus it should land in the 1.2.0 version.

The generated code:

impl Default for Contract {
    fn default() -> Self {
        Self::new()
    }
}
kulikthebird commented 1 month ago

I tried to use some sort of a const-time assertion to check whether the Default trait is implemented on the contract and there's no user-friendly way to do so. Compile time assertions are possible, but we cannot control the error messages in this case (in stable rust). So yes, it seems that auto-generating Default trait for the contract is the most elegant way to fix the warning issue and to keep error messages clear for the user.

kulikthebird commented 1 month ago

The second thought came to mind after a while. If sylvia requires the contract to implement fn new() -> Self; method, then it means that it's done by design. Purposely we want users to implement this method instead of impl Default, so we should not treat it as a mistake. In such case the clippy warning is not appropriate and we can silence it without any further actions.

Other than that, generating Default implementation by sylvia would be surprising for the users, as they would want to add #[derive(Default)] on their own and implement new method as:

    fn new() -> Self {
        Self::default()
    }

For the above I think it's better to simply add #[cfg_attr(feature = "cargo-clippy", allow(clippy::new_without_default))] to the impl Contract block in the contract macro.