Lint for unnecessary type casts #11135
Conversation
|
This seems like an interesting lint, but in the FFI case I can imagine that this will warn about completely legitimate casts. The corresponding FFI types for integers in C often coincide with the current architecture's uint or int, but it may not if you cross compile for another architecture. This means in one mode of compilation the cast will be warned about but in the other it isn't. In general I feel like lints should be encouraging you to improve your program or catching possible bugs, but this seem like a regular thing in some circumstances that shouldn't be warned about. |
|
Isn't that what |
|
I think the problem @alexcrichton mentioned is similar to the problem related to macros. I think it would be nice to have an option to ignore typecasts in macros, although I don't have an idea how to do it at the moment. Does the AST, that's used by the linter, contain any information about expanded macros? As far as I understand, the macros are expanded at an earlier stage and the macro nodes are dropped afterwards. For the FFI case, maybe an option to ignore typecasts with libc types would be useful? Or we could simply use |
|
This isn't even in macros, for example casting to I am worried that this will warn about enough legitimate casts that |
|
That's true On Wed, Dec 25, 2013 at 7:13 PM, Alex Crichton notifications@github.comwrote:
|
|
I've added tests for the three cases mentioned in the discussion above: During the last couple of days, I had a look into possible solutions for the problems mentioned above. To skip cast checks in expanded macros I added a boolean flag the Both fixes probably miss a couple of unnecessay casts, e.g. if someone uses their own type with a name like in a macro. This patch will probably require more work, but I would like to get some feedback about the direction of the patch. |
|
I'm personally not a fan of singling out I personally think that for this lint to be useful it will require a lot of special casing that probably shouldn't be in the compiler anyway, but it also may just need a different direction to go in. |
|
I agree this isn't a viable solution. It was intended as quick proof of concept, but at the moment I do not see a way to special case the libc types. Therefore it would be necessary to check if a type is platform depended. But I managed to lint How should we move on with this pull request? Should I close it? |
|
Could this lint default to allow? I still think it's a useful analysis to On Sat, Jan 4, 2014 at 5:57 PM, Florian Hahn notifications@github.comwrote:
|
|
I like @cmr's suggestion, but let's revert the changes to the AST and the parser for now. I would expect the lint to be entirely self contained in middle::lint, and then I'm more than happy to see all the unnecessary casts go away. |
Conflicts: src/librustc/middle/lint.rs
|
I've reverted the changes to the AST and the parser. Somehow I couldn't manage to overwrite the branch of this pull request (fhahn:unused-cast-lint) with the updated commits. I'm going to close this PR and create a new one (#11329) with the updates (Sorry for this inconvenience) . |
…=Centri3
Fix `unwrap_or_else_default` false positive
This PR fixes a false positive in the handling of `unwrap_or_else` with a default value when the value is needed for type inference.
An easy example to exhibit the false positive is the following:
```rust
let option = None;
option.unwrap_or_else(Vec::new).push(1);
```
The following code would not compile, because the fact that the value is a `Vec` has been lost:
```rust
let option = None;
option.unwrap_or_default().push(1);
```
The fix is to:
- implement a heuristic to tell whether an expression's type can be determined purely from its subexpressions, and the arguments and locals they use;
- apply the heuristic to `unwrap_or_else`'s receiver.
The heuristic returns false when applied to `option` in the above example, but it returns true when applied to `option` in either of the following examples:
```rust
let option: Option<Vec<u64>> = None;
option.unwrap_or_else(Vec::new).push(1);
```
```rust
let option = None::<Vec<u64>>;
option.unwrap_or_else(Vec::new).push(1);
```
(Aside: rust-lang/rust-clippy#10120 unfairly contained multiple changes in one PR. I am trying to break that PR up into smaller pieces.)
---
changelog: FP: [`unwrap_or_else_default`]: No longer lints if the default value is needed for type inference
type certainty: clear `DefId` when an expression's type changes to non-adt Fixes rust-lang#12585 The root cause of the ICE in the linked issue was in the expression `one.x`, in the array literal. The type of `one` is the `One` struct: an adt with a DefId, so its certainty is `Certain(def_id_of_one)`. However, the field access `.x` can then change the type (to `i32` here) and that should update that `DefId` accordingly. It does do that correctly when `one.x` would be another adt with a DefId: https://github.com/rust-lang/rust-clippy/blob/97ba291d5aa026353ad93e48cf00e06f08c73830/clippy_utils/src/ty/type_certainty/mod.rs#L90-L91 but when it *isn't* an adt and there is no def id (which is the case in the linked issue: `one.x` is an i32), it keeps the `DefId` of `One`, even though that's the wrong type (which would then lead to a contradiction later when joining `Certainty`s): https://github.com/rust-lang/rust-clippy/blob/97ba291d5aa026353ad93e48cf00e06f08c73830/clippy_utils/src/ty/type_certainty/mod.rs#L92-L93 In particular, in the linked issue, `from_array([one.x, two.x])` would try to join the `Certainty` of the two array elements, which *should* have been `[Certain(None), Certain(None)]`, because `i32`s have no `DefId`, but instead it was `[Certain(One), Certain(Two)]`, because the DefId wasn't cleared from when it was visiting `one` and `two`. This is the "contradiction" that could be seen in the ICE message ... so this changes it to clear the `DefId` when it isn't an adt. cc `@smoelius` you implemented this initially in rust-lang#11135, does this change make sense to you? changelog: none
This patch adds a lint for unnecessary casts and removes a couple of unnecessary casts as mentioned in the discussion of #7313.
The main problem I have with this lint is handling macros. Some macros contain typecasts, which are necessary for most expansions, but for some expansion they are unnecessary. This leads to a lot of, probably useless, lint warnings (/errors) during compilation.
I found no way to find out, if nodes in the ast were expanded from a macro, to disable type cast checks for macros. Is there a way to find out, if an expression was expanded from a macro during linting?