New lint [trivial_default_constructed_types]
#10985
Conversation
|
r? @Jarcho (rustbot has picked a reviewer for you, use r? to override) |
rustbot
added
the
S-waiting-on-review
Centri3
force-pushed
the
trivial_default_constructed_types
branch
from
ad0ed39
to
fa1c3c8
Compare
|
☔ The latest upstream changes (presumably #10951) made this pull request unmergeable. Please resolve the merge conflicts. |
Centri3
force-pushed
the
trivial_default_constructed_types
branch
from
fa1c3c8
to
f1b4fbc
Compare
|
☔ The latest upstream changes (presumably #10884) made this pull request unmergeable. Please resolve the merge conflicts. |
Centri3
force-pushed
the
trivial_default_constructed_types
branch
2 times, most recently
from
9fdd6a1
to
834f089
Compare
Centri3
force-pushed
the
trivial_default_constructed_types
branch
2 times, most recently
from
5a9b2be
to
dcaf1b1
Compare
| fn default_value(cx: &LateContext<'_>, ty: Ty<'_>) -> Option<Cow<'static, str>> { | ||
| match ty.kind() { | ||
| ty::Adt(def, substs) if let [subst] = substs.as_slice() => { | ||
| is_lang_item_or_ctor(cx, def.did(), LangItem::Option).then(|| format!("None::<{subst}>").into()) |
There was a problem hiding this comment.
Adding the type argument to None when it's not needed isn't really an improvement. None::<u32> is ok. None::<Vec<String>> is not great. None::<Arc<Mutex<SomeStruct<String>>>> is terrible. None of them are better than Option::default().
There was a problem hiding this comment.
We would likely need to use the QPath instead of Ty to fix that, but this would ignore type aliases and the like
There was a problem hiding this comment.
I think the easiest way to "fix" this is to just make it HasPlaceholders in the None case, and use ::<_>.
There was a problem hiding this comment.
You bring up a fun issue with type aliases. Numeric literals can't just use type suffixes if the required type is an alias since it could be configuration dependent.
With all the edge cases I don't think there's any way to avoid parsing the type part of the path. Only linting when either the type is missing (e.g. Default::default()), or the type is a single identifier matching the expected type's name (e.g. Option::default()) is fine. If somebody does something stupid like type u32 = i32; then the lint can not trigger. type MyInt = i32; is probably better not linted as well.
There was a problem hiding this comment.
That's a good point, so parsing the path is necessary then. I believe we can use .into() for type aliases in particular, or <name>::from(x) perhaps (which is usually what I use for creating type aliases). But we probably shouldn't lint them yeah
There was a problem hiding this comment.
This is easier to do now with expr_use_ctxt. You'll have to add a function to get the defined type as a HIR type, but that's mostly copying the existing function. This won't work for anything defined in another crate unfortunately.
There was a problem hiding this comment.
Honestly I feel like at this point it'd just be easier to store where it's a type alias directly in the Ty's info 😅 It can probably be changed upstream rather easily
I may look into that...
There was a problem hiding this comment.
You still need to take type inference into account, so you would still have to look up the type as it was written either way.
There was a problem hiding this comment.
Indeed, but at least we'd be able to ignore type aliases more easily. Not a big deal for u32::default where u32 is type u32 = i32 but Default::default is currently pretty annoying (same with stuff like function calls outside the current crate)
There was a problem hiding this comment.
I think, for now, I'll change this to lint type aliases for Default::default and ignoring them on u32::default using the Res. One day, once lazy_type_alias is stable (and hopefully that normalization is delayed until after typeck 😅) then we can ignore them entirely.
|
☔ The latest upstream changes (presumably #11020) made this pull request unmergeable. Please resolve the merge conflicts. |
Centri3
force-pushed
the
trivial_default_constructed_types
branch
from
7aee58b
to
d8ec7df
Compare
|
☔ The latest upstream changes (presumably #10788) made this pull request unmergeable. Please resolve the merge conflicts. |
Centri3
force-pushed
the
trivial_default_constructed_types
branch
from
d8ec7df
to
38731ab
Compare
|
☔ The latest upstream changes (presumably #11140) made this pull request unmergeable. Please resolve the merge conflicts. |
Centri3
force-pushed
the
trivial_default_constructed_types
branch
from
38731ab
to
5f49b56
Compare
|
Hey this is triage, I'm closing this due to inactivity. Currently, @Centri3 sadly doesn't have the time to continue this implementation. If anyone is interested in continuing this PR, you're more than welcome to create a new PR and push it over the finish line. :D Thank you to @Centri3 and the reviewers for the time, that you already put into this! @rustbot label +S-inactive-closed -S-waiting-on-author -S-waiting-on-review |
rustbot
added
S-inactive-closed
|
Triaging some of my PRs in case someone wants to pick them up, this one is pretty close to being done I think. The ergonomics are good, It doesn't mess with anything it knows it can't change, and also tries to omit generic parameters, etc. Mostly, I think we should extract this weird HIR + If nobody else does, I'll definitely finish this at some point in the future |
Will lint on cases like
<&str>::defaultoru32::default, etc.changelog: New lint [
trivial_default_constructed_types]