Skip to content
New issue

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

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

Already on GitHub? # to your account

Remove visit_subpats parameter from check_pat #60043

Closed
varkor opened this issue Apr 17, 2019 · 2 comments · Fixed by #60152
Closed

Remove visit_subpats parameter from check_pat #60043

varkor opened this issue Apr 17, 2019 · 2 comments · Fixed by #60152
Labels
C-cleanup Category: PRs that clean code up or issues documenting cleanup. E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@varkor
Copy link
Member

varkor commented Apr 17, 2019

The visit_subpats parameter is a hack (to fix a problem with range patterns) that should be refactored. The idea is that the state should be contained in EllipsisInclusiveRangePatterns instead.

This should be straightforward to fix.

  1. Remove the &mut bool parameter from
    fn check_pat(a: &ast::Pat, b: &mut bool); // FIXME: &mut bool looks just broken

    (You'll also need to update the signature of check_pat in src/librustc_lint/unused.rs.
  2. Add a check_pat_post method with an identical signature below it.
  3. Remove the visit_subpats condition from
    fn visit_pat(&mut self, p: &'a ast::Pat) {
    let mut visit_subpats = true;
    run_early_pass!(self, check_pat, p, &mut visit_subpats);
    self.check_id(p.id);
    if visit_subpats {
    ast_visit::walk_pat(self, p);
    }
    }
  4. Run an early lint pass for check_pat_post at the end.
  5. Add a Option<NodeId> field to EllipsisInclusiveRangePatterns in
    declare_lint_pass!(EllipsisInclusiveRangePatterns => [ELLIPSIS_INCLUSIVE_RANGE_PATTERNS]);

    (Look at DeprecatedAttr for an example of a lint with fields.)
    You'll also need to initialise it with None here:
    EllipsisInclusiveRangePatterns: EllipsisInclusiveRangePatterns,
  6. This should be replaced with an assignment of the new field in EllipsisInclusiveRangePatterns to pat.id:
    *visit_subpats = false;
  7. check_pat should be early-exited if the field is Some(..).
  8. Add a check_pat_post implementation to EllipsisInclusiveRangePatterns and reset the field to None if the pattern ID matches the field ID.
  9. Make sure the UI tests still pass with no changes.
@varkor varkor added E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. C-cleanup Category: PRs that clean code up or issues documenting cleanup. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. labels Apr 17, 2019
@jonas-schievink jonas-schievink added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Apr 17, 2019
@stepnivlk
Copy link
Contributor

hi @varkor, I'm happy to take care of this one if it's not taken already.

@varkor
Copy link
Member Author

varkor commented Apr 17, 2019

@stepnivlk: go ahead! Let me know if you have any questions or problems with the implementation! :)

bors added a commit that referenced this issue Apr 23, 2019
Remove `visit_subpats` parameter from `check_pat`

The core idea is to keep track of current ID directly in `EllipsisInclusiveRangePatterns` struct and early return in `check_pat` based on it.

Fixes #60043.

r? @varkor
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
C-cleanup Category: PRs that clean code up or issues documenting cleanup. E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants