Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

False positive for boxed local with wasm_bindgen #4351

Open
Tracked by #12046
rivertam opened this issue Aug 7, 2019 · 14 comments
Open
Tracked by #12046

False positive for boxed local with wasm_bindgen #4351

rivertam opened this issue Aug 7, 2019 · 14 comments
Labels
C-bug Category: Clippy is not doing the correct thing good-first-issue These issues are a good way to get started with Clippy I-false-positive Issue: The lint was triggered on code it shouldn't have

Comments

@rivertam
Copy link

rivertam commented Aug 7, 2019

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
Copy link
Author

rivertam commented Aug 7, 2019

Documentation for convenience.

@flip1995
Copy link
Member

flip1995 commented Aug 8, 2019

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
Copy link
Author

rivertam commented Aug 9, 2019

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

@rivertam
Copy link
Author

rivertam commented Aug 9, 2019

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
Copy link
Member

flip1995 commented Aug 9, 2019

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

@flip1995 flip1995 added good-first-issue These issues are a good way to get started with Clippy C-bug Category: Clippy is not doing the correct thing labels Aug 9, 2019
@rivertam
Copy link
Author

rivertam commented Aug 9, 2019

(s/write/right btw)

@rivertam
Copy link
Author

rivertam commented Aug 9, 2019

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

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

@flip1995
Copy link
Member

flip1995 commented Aug 9, 2019

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

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

@flip1995
Copy link
Member

flip1995 commented Aug 9, 2019

@rivertam
Copy link
Author

rivertam commented Aug 9, 2019

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

@ghost
Copy link

ghost commented Aug 25, 2019

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
Copy link
Author

rivertam commented Aug 26, 2019

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.

@ghost
Copy link

ghost commented Aug 29, 2019

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.

We can and do check if is consumed or moved. Log a bug (with a MCVE) If you find any false positives due to that.

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.

Please provide a code sample of this. AFAIK, it's a Javascript TypedArray either way.

Box<[JsValue]> only works for Array. 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.

Yes, Box<[f64]> is allowed but so is &[f64] and the wasm documentation notes that the Box<[f64]> involves copying. This is exactly what this lint is supposed to warn about.

@rivertam
Copy link
Author

rivertam commented Aug 29, 2019

We can and do check if is consumed or moved. Log a bug (with a MCVE) If you find any false positives due to that.

Got it. I didn't realize this was the case. Thanks!

Please provide a code sample of this. AFAIK, it's a Javascript TypedArray either way.

I think you're right that this is true for Box<[f64]> and similar. This is useful for types defined in Rust and exported to JS, but those are already unboxed, so it's not relevant.

Yes, Box<[f64]> is allowed but so is &[f64] and the wasm documentation notes that the Box<[f64]> involves copying.

Well, you just taught me this. =) My critical reading skills are not what they once were.

Given that I can't think of any other objections, I think you're entirely correct that Box<[JsValue]> is the only remaining example of where this should be allowed. It's actually kind of confusing to me that they don't allow &[JsValue] given they do allow &[f64]. Maybe they do and it's just not documented. (edit: Just checked and they don't)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: Clippy is not doing the correct thing good-first-issue These issues are a good way to get started with Clippy I-false-positive Issue: The lint was triggered on code it shouldn't have
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants