False positive for boxed local with `wasm_bindgen`

[rivertam]

wasm_bindgen is a little bit different than the issue that's been resolved with traits. In particular, wasm_bindgen just generates code around the non-trait implemented function, and the only way to use native JS arrays as parameters is to use Box<[...]>.

In general, I think maybe boxed slices should be considered ok for local parameters, as they're conceptually very similar to vectors.

[rivertam]

Documentation for convenience.

[flip1995]

I'm not familiar with wasm[_bindgen]. Is the boxing only necessary for [JsValue] or are there other cases where a Box is required?

If this is only necessary for the [JsValue] case, we could add a special case to the lint checking for this type.

[rivertam]

It also uses boxed number slices (Box<[f64]>, for example): https://rustwasm.github.io/docs/wasm-bindgen/reference/types/boxed-number-slices.html

[rivertam]

I'm not sure I know of a time when a Box<[f64]> logically breaks the boxed_local rule. The point of the boxed local rule is to find instances of heap allocation where it's just not necessary, right? So I think any object that doesn't fulfill Sized (slices included) should be fine to put in Boxes, even locally.

[flip1995]

Yeah you're write, that would be a good fix to just check for Sized in the box_local lint.

[rivertam]

(s/write/right btw)

[rivertam]

rust-clippy/clippy_lints/src/escape.rs

Line 200 in 5c1e30a

fn is_large_box(&self, ty: Ty<'tcx>) -> bool {

I'm guessing this needs a sister method called is_sized_box or something.

[flip1995]

Oops yeah of course thanks. Won't edit because ¯\_(ツ)_/¯

I think we already have a is_sized method in clippy_lints/src/utils

[flip1995]

Ah rustc's ty::Ty has a is_sized method: https://doc.rust-lang.org/nightly/nightly-rustc/rustc/ty/struct.TyS.html#method.is_sized

[rivertam]

Yeah I'm working on a PR. Should be really fast.

I'm against the change of allowing this for unsized types.

For example, you should always use fn f(x:&[f64]) or fn f(x:&mut [f64]) instead of fn f(x: Box<[f64]>). Otherwise, a caller has to convert whatever they have into a Box<[f64]> instead of just passing a reference. At best it will be inconvenient; as worst if will require allocations or copying. The only time when Box<[f64]> is needed is when the argument is consumed or moved and the lint checks that.

The fix for this issue is adding an exception for Box<[JsValue]> on functions marked with #[wasm_bindgen].

[rivertam]

I see what you're saying. Is the philosophy of clippy to warn unless it can prove that what you're doing is reasonable, or is the philosophy to warn if it can prove that what you're doing is unreasonable?

Box<[f64]> is reasonable, like you said, when the argument is consumed or moved. If we can't check whether the argument is consumed or moved, I personally would rather use a tool that gives me the benefit of the doubt. This is especially true for bindgenned functions, where "unnecessary" moves is sometimes a feature to make it so the client doesn't have to remember to free the object.

Box<[JsValue]> only works for Array<number>. It won't cover Box<[f64]>, for example, which is allowed with #[wasm_bindgen]. wasm_bindgen happens to be limited to only a select few parameter types, so I'm more likely to say anything that gets bindgenned should ignore this lint, regardless of parameter type.