Add a lint for not using field pattern shorthands#17813
Add a lint for not using field pattern shorthands#17813bors merged 1 commit intorust-lang:masterfrom
Conversation
src/librustc/lint/builtin.rs
Outdated
There was a problem hiding this comment.
This would be more efficient with the is_shorthand check first.
08b53ae to
97699d0
Compare
|
@huonw Comments addressed. |
src/librustc/lint/builtin.rs
Outdated
There was a problem hiding this comment.
There's a small subtlety here, which the following example demonstrates:
struct Struct { x: uint }
const x: uint = 42u;
fn main() {
match (Struct { x: 1u }) {
Struct { x: x } => (),
Struct { x: _ } => ()
}
}We need to check that the PatIdent here is not resolved to any definition (it can be a constant but I think in some weird cross-crate situations it could also be a variant or a unit struct).
I think this would read well:
let def_map = cx.tcx.def_map.borrow();
match pat.node {
ast::PatStruct(_, ref v, _) => {
for fieldpat in v.iter()
.filter(|fieldpat| !fieldpat.node.is_shorthand)
.filter(|fieldpat| !def_map.contains_key(fieldpat.node.pat.id)) {
match fieldpat.node.pat.node {
ast::PatIdent(_, ident, None) if ident.node.as_str()There was a problem hiding this comment.
I tried that and it looks like the lint just stopped triggering for anything. I’m thinking of trying something like this:
for fieldpat in v.iter()
.filter(|fieldpat| !fieldpat.node.is_shorthand)
.filter(|fieldpat| match def_map.find(&fieldpat.node.pat.id) {
Some(&def::DefConst(_)) => false,
Some(&def::DefVariant(_)) => false,
_ => true,
}) {but I’m not sure if that will work, and if it does how I’ll handle the tuple/unit struct case.
|
@P1start This looks great. I think if you rebased and addressed the def_map issue, we could get someone to r+ it. :) |
97699d0 to
aaa8b5a
Compare
|
OK, I’ve made an attempt (that seems to work) at fixing the issue with |
src/librustc/lint/builtin.rs
Outdated
There was a problem hiding this comment.
@P1start I see! My bad, I completely forgot about DefLocal. I think this should work:
for fieldpat in v.iter().
.filter(|fieldpat| def_map.find(&fieldpat.node.pat.id) == Some(&def::DefLocal(fieldpat.node.pat.id)))
.filter(|fieldpat| !fieldpat.node.is_shorthand)
{
match fieldpat.node.pat.node {(no need for if fieldpat.node.is_shorthand { continue }).
|
@P1start r=me with the last two changes pending @huonw's approval. |
e518478 to
1478d0a
Compare
|
I'm happy. @huonw? |
1478d0a to
fa98441
Compare
|
@P1start This will need another rebase. :( |
fa98441 to
baada44
Compare
|
@P1start Sorry, will need another rebase. Please ping someone on IRC as soon as you rebase so that we can get the PR in ASAP as it's really prone to bitrotting. :) |
baada44 to
ca70e86
Compare
ca70e86 to
ead6c4b
Compare
Use the `is_shorthand` field introduced by rust-lang#17813 (ead6c4b) to make the prettyprinter output the shorthand form. Fixes a few places that set `is_shorthand: true` when the pattern is not a PatIdent with the same name as the field.
fix: tyck for non-ADT types when searching refs for `Self` kw See https://github.com/rust-lang/rust-analyzer/pull/15864/files/e0276dc5ddc38c65240edb408522bb869f15afb4#r1389848845 For ADTs, to handle `{error}` in generic args, we should to convert them to ADT for comparisons; for others, we can directly compare the types.
Closes #17792.