wildcard_enum_match_arm lint takes the enum origin into account #10250
Conversation
|
r? @dswij (rustbot has picked a reviewer for you, use r? to override) |
rustbot
added
the
S-waiting-on-review
|
r? @xFrednet Tagging you for review as it is your issue, and you've helped me before. I believe with this PR, the behavior is correct, however, I can't figure out how to declare an enum outside of this crate to test with. For that reason, with this change, all the previous tests that would trigger this lint do not (as the enum is from within this crate). |
There was a problem hiding this comment.
Hey 👋, thank you for the PR.
You can create a file under tests/ui/auxiliary and then access it with extern crate <file_name> to have external items. #10260 recently did this and might be a good example.
The logic looks good to me, only a small suggestion to move the check to the DefId of the ADT. I contemplated for a second, if a config value might be good, but I think this is sufficient :)
| cx.tcx.is_doc_hidden(variant_def.def_id) || cx.tcx.has_attr(variant_def.def_id, sym::unstable) | ||
| fn is_hidden_and_external(cx: &LateContext<'_>, variant_def: &VariantDef) -> bool { | ||
| (cx.tcx.is_doc_hidden(variant_def.def_id) || cx.tcx.has_attr(variant_def.def_id, sym::unstable)) | ||
| && variant_def.def_id.as_local().is_none() |
There was a problem hiding this comment.
I would do this check once on the ADT DefId and return early if its not local. That should be slightly faster and clearer.
There was a problem hiding this comment.
I'm not sure what you mean about the ordering of the check. I re-ordered the checks between external and hidden.
As to the tests, wildcard_enum_atch_arm.rs is already doing this and the test result is as I would expect which is sweet.
The local enums with hidden types are now expanded, and the external one is not in the test output.
There was a problem hiding this comment.
I think I figured out what you are asking for here. I pulled the test for this being external out of the first loop. I can't figure out how to get the logic right to pull it out for the second one. I did at least make it just testing a captured bool instead of making a function call each time.
There was a problem hiding this comment.
My comment wasn't clear, as I misunderstood the code in question a bit. But you got what I recommended. I'll have a second look at it :)
Signed-off-by: Tyler Weaver <maybe@tylerjw.dev>
|
Looks good to me, thank you for the update, I like this change! @bors r+ |
|
☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test |
1 similar comment
|
☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test |
fixes #7419
changelog: Enhancement: [
wildcard_enum_match_arm]: Now lints missing private variants, for local enums#10250