Remove `visit_subpats` parameter from `check_pat` #60152

stepnivlk · 2019-04-21T14:16:51Z

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

Centril · 2019-04-22T00:55:51Z

src/librustc_lint/builtin.rs

+
+impl_lint_pass!(EllipsisInclusiveRangePatterns => [ELLIPSIS_INCLUSIVE_RANGE_PATTERNS]);
+
+impl EllipsisInclusiveRangePatterns {


You can #[derive(Default)] for this instead.

I chose it b/c of consistency, there's new on other linters, e.g. https://github.com/rust-lang/rust/blob/5160955b96636db3b3b70a348c53454e73473538/src/librustc_lint/builtin.rs#L552 and https://github.com/rust-lang/rust/blob/5160955b96636db3b3b70a348c53454e73473538/src/librustc_lint/lib.rs#L99

#[derive(Default)] would give me default method

@stepnivlk To achieve semantic compression, it seems preferable to use #[derive(Default)] wherever possible for these lints. Could you file an issue for converting all places in a different PR?

varkor

This looks good to me! Just a couple of tiny comments about formatting (and adding a couple of comments).

varkor · 2019-04-22T13:19:29Z

src/librustc_lint/builtin.rs


 impl EarlyLintPass for EllipsisInclusiveRangePatterns {
-    fn check_pat(&mut self, cx: &EarlyContext<'_>, pat: &ast::Pat, visit_subpats: &mut bool) {
+    fn check_pat(&mut self, cx: &EarlyContext<'_>, pat: &ast::Pat) {
+        if let Some(_node_id) = self.node_id { return }


Suggested change

if let Some(_node_id) = self.node_id { return }

if self.node_id.is_some() {

// Don't recursively warn about patterns inside range endpoints.

return

}

varkor · 2019-04-22T13:21:58Z

src/librustc_lint/builtin.rs

@@ -1286,10 +1286,24 @@ declare_lint! {
    "`...` range patterns are deprecated"
 }

-declare_lint_pass!(EllipsisInclusiveRangePatterns => [ELLIPSIS_INCLUSIVE_RANGE_PATTERNS]);
+pub struct EllipsisInclusiveRangePatterns {
+    node_id: Option<ast::NodeId>,


Suggested change

node_id: Option<ast::NodeId>,

/// If `Some(_)`, suppress all subsequent pattern

/// warnings for better diagnostics.

node_id: Option<ast::NodeId>,

varkor · 2019-04-22T13:22:50Z

src/librustc_lint/builtin.rs

+
+    fn check_pat_post(&mut self, _cx: &EarlyContext<'_>, pat: &ast::Pat) {
+        if let Some(node_id) = self.node_id {
+            if pat.id == node_id { self.node_id = None }


Suggested change

if pat.id == node_id { self.node_id = None }

if pat.id == node_id {

self.node_id = None;

}

(Blocks should be on new lines.)

…usiveRangePatterns

stepnivlk · 2019-04-22T21:39:38Z

@Centril issue was filed, thx for the info.
@varkor PR was updated according to your comments, thx for it and introducing me to lints :)

varkor · 2019-04-22T21:47:19Z

@stepnivlk: thanks!

@bors r+

bors · 2019-04-22T21:47:20Z

📌 Commit 1dc13b5 has been approved by varkor

@varkor

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

bors · 2019-04-23T12:27:45Z

⌛ Testing commit 1dc13b5 with merge fe0a415...

bors · 2019-04-23T15:20:48Z

☀️ Test successful - checks-travis, status-appveyor
Approved by: varkor
Pushing fe0a415 to master...

rust-highfive · 2019-04-23T15:21:46Z

📣 Toolstate changed by #60152!

Tested on commit fe0a415.
Direct link to PR: #60152

💔 clippy-driver on windows: test-fail → build-fail (cc @Manishearth @llogiq @mcarton @oli-obk @phansch, @rust-lang/infra).
💔 clippy-driver on linux: test-fail → build-fail (cc @Manishearth @llogiq @mcarton @oli-obk @phansch, @rust-lang/infra).
💔 rls on windows: test-fail → build-fail (cc @Xanewok, @rust-lang/infra).
💔 rls on linux: test-fail → build-fail (cc @Xanewok, @rust-lang/infra).

@Manishearth

Tested on commit rust-lang/rust@fe0a415. Direct link to PR: <rust-lang/rust#60152> 💔 clippy-driver on windows: test-fail → build-fail (cc @Manishearth @llogiq @mcarton @oli-obk @phansch, @rust-lang/infra). 💔 clippy-driver on linux: test-fail → build-fail (cc @Manishearth @llogiq @mcarton @oli-obk @phansch, @rust-lang/infra). 💔 rls on windows: test-fail → build-fail (cc @Xanewok, @rust-lang/infra). 💔 rls on linux: test-fail → build-fail (cc @Xanewok, @rust-lang/infra).

rust-highfive assigned varkor Apr 21, 2019

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 21, 2019

Centril reviewed Apr 22, 2019

View reviewed changes

varkor suggested changes Apr 22, 2019

View reviewed changes

stepnivlk mentioned this pull request Apr 22, 2019

Lints should be initialized via derived Default wherever possible #60181

Closed

Remove visit_subpats from check_pat in favor of state in EllipsisIncl…

1dc13b5

…usiveRangePatterns

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 22, 2019

bors added the merged-by-bors This PR was explicitly merged by bors. label Apr 23, 2019

bors merged commit 1dc13b5 into rust-lang:master Apr 23, 2019

tesuji mentioned this pull request Oct 14, 2020

ICE: Encountered errors [FulfillmentError(Obligation(predicate=TraitPredicate(<Five as TypeVal<[type error]>>), depth=1),Ambiguity)] resolving bounds after type-checking #77919

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove `visit_subpats` parameter from `check_pat` #60152

Remove `visit_subpats` parameter from `check_pat` #60152

stepnivlk commented Apr 21, 2019 •

edited by varkor

Loading

Centril Apr 22, 2019

stepnivlk Apr 22, 2019

Centril Apr 22, 2019 •

edited

Loading

varkor left a comment

varkor Apr 22, 2019

varkor Apr 22, 2019

varkor Apr 22, 2019

stepnivlk commented Apr 22, 2019

varkor commented Apr 22, 2019

bors commented Apr 22, 2019

bors commented Apr 23, 2019

bors commented Apr 23, 2019

rust-highfive commented Apr 23, 2019


		impl_lint_pass!(EllipsisInclusiveRangePatterns => [ELLIPSIS_INCLUSIVE_RANGE_PATTERNS]);

		impl EllipsisInclusiveRangePatterns {

-        if let Some(_node_id) = self.node_id { return }
+        if self.node_id.is_some() {
+            // Don't recursively warn about patterns inside range endpoints.
+            return
+        }

Remove visit_subpats parameter from check_pat #60152

Remove visit_subpats parameter from check_pat #60152

Conversation

stepnivlk commented Apr 21, 2019 • edited by varkor Loading

Centril Apr 22, 2019

Choose a reason for hiding this comment

stepnivlk Apr 22, 2019

Choose a reason for hiding this comment

Centril Apr 22, 2019 • edited Loading

Choose a reason for hiding this comment

varkor left a comment

Choose a reason for hiding this comment

varkor Apr 22, 2019

Choose a reason for hiding this comment

varkor Apr 22, 2019

Choose a reason for hiding this comment

varkor Apr 22, 2019

Choose a reason for hiding this comment

stepnivlk commented Apr 22, 2019

varkor commented Apr 22, 2019

bors commented Apr 22, 2019

bors commented Apr 23, 2019

bors commented Apr 23, 2019

rust-highfive commented Apr 23, 2019

Remove `visit_subpats` parameter from `check_pat` #60152

Remove `visit_subpats` parameter from `check_pat` #60152

stepnivlk commented Apr 21, 2019 •

edited by varkor

Loading

Centril Apr 22, 2019 •

edited

Loading