unnecessary_wraps is a bad default lint

[mitsuhiko]

Lint name: unnecessary_wraps

The newly introduced unnecessary_wraps lint is one I now need to disable everywhere it has popped up so far as a warning. The reason for this is that we use serde(default = func) a lot and when you have a nullable type you need to return a Some(value) from that function.

I would make an argument that this lint should not be in the list of default lints because of there being a lot of legitimate cases where one does need "unnecessary wrapping".

[foresterre]

I also think the unnecessary_wraps should not be enabled by default. Although the intentions of this lint are good, I believe sometimes having some, perhaps unnecessary, wrapping makes code more readable in the end ☺️.

In my use case, some functions return a Result<(), Error>. Strictly speaking, this is unnecessary, but this is a small cost when the code at the call site becomes cleaner and more readable.

Take for example the following code where the unnecessary_wraps lint pops up:

fn process_instruction(&mut self, instruction: Instr) -> Result<(), SicImageEngineError> {
    match instruction {
        Instr::Operation(op) => self.process_operation(op),
        Instr::EnvAdd(item) => self.insert_env(*item),
        Instr::EnvRemove(key) => self.remove_env(*key),
    }
}

Here, self.insert_env and self.remove_env, do not result in a failure case, but return a Ok(()) regardless, to improve the readability of the process_instruction from which they are called.

The self.process_operation(op) can result in a SicImageEngineError so does not unnecessary wrap.

After fixing the code to resolve this linter issue, it becomes:

fn process_instruction(&mut self, instruction: &Instr) -> Result<(), SicImageEngineError> {
    match instruction {
        Instr::Operation(op) => self.process_operation(op),
        Instr::EnvAdd(item) => {
            self.insert_env(*item);
            Ok(())
        }
        Instr::EnvRemove(key) => {
            self.remove_env(*key);
            Ok(())
        }
    }
}

Here, the complexity of the Instr::EnvAdd and Instr::EnvRemove match arms are increased and consequently, the complete function is less readable.

Another example

impl Action for UpdateManifest<'_> {
    fn run(&mut self, args: &PublishWorkspace) -> anyhow::Result<()> {
        if args.dry_run {
        	// -> This fn returns Ok(()) "unnecessarily"
            dry_update_dependency_version(self.package, args.version())
        } else {
        	// -> This fn can actually fail
            live_update_dependency_version(self.package, args.version())
        }
    }
}

[mitsuhiko]

It's also add that you would have to refactor your API just because the code in the current implementation might not return an error.

[glts]

Another example where this lint may not be desirable: I have a small parser here where all parsing functions have a similar signature (very much like nom parsers):

fn some_object(input: &str) -> Option<(&str, SomeObject)>

Sometimes the parsing function is infallible and always returns Some. As a design, and for callers, I believe it is more consistent to keep Option as the return type.

[llogiq]

Even with downgrading the lint, a check to not lint exported items would be useful.