Perform type inference in range pattern #88090

nbdd0121 · 2021-08-16T20:24:22Z

rust-highfive · 2021-08-16T20:24:25Z

(rust-highfive has picked a reviewer for you, use r? to override)

jackh726 · 2021-08-17T20:19:18Z

This is technically a behavior change (even though I agree it's more of a bug fix), so I'm just going to cc @rust-lang/compiler

(If anyone else knows this code better and wants to take the review, feel free)

estebank

The changes look reasonable after a cursory review. @rust-lang/lang should chime in and approve the behavioral change (unless this was intentional). With that I can take a closer look at the code to approve or as for more changes.

estebank · 2021-08-18T10:28:43Z

src/test/ui/pattern/issue-88074-pat-range-type-inference-err.rs

+}
+
+impl Zero for String {
+    const ZERO: Self = String::new();


When associated consts are involved in this error, we should likely point at them.

estebank · 2021-08-18T10:29:54Z

src/test/ui/pattern/issue-88074-pat-range-type-inference-err.stderr

+   |         |              this is of type `_` but it should be `char` or numeric
+   |         this is of type `_` but it should be `char` or numeric


Somewhere we are not actually using resolve_ty_if_possible. We should be mentioning that these are of type String.

estebank · 2021-08-18T10:31:25Z

compiler/rustc_typeck/src/check/pat.rs

+            if let Some((ref mut fail, _, _)) = rhs {
+                *fail = true;
+            }
+            self.emit_err_pat_range(span, lhs, rhs);


I think this is why. We pass in lhs and rhs. Inside of emit_err_pat_range we don't resolve_vars_if_possible on lhs.1 or rhs.1. I think that doing that should make the labels talk about String instead of _.

jackh726 · 2021-09-07T19:51:15Z

I'm going to go ahead and nominate this for the lang team:

Do we want to allow inference in range patterns (as in https://github.com/rust-lang/rust/blob/353c03955dfe0831e4b58f9e05e99447c06adff2/src/test/ui/pattern/issue-88074-pat-range-type-inference.rs)? The Self type here could be inferred to be i32, as would be in a non-range pattern.

nikomatsakis · 2021-09-09T14:52:34Z

To summarize the effects of this PR, I believe it implements the following logic:

When type checking a range pattern...
- In a range pattern, the LHS and RHS must be of equal type
- Further, those types must be known to be char or integral

whereas before the logic was

When type checking a range pattern...
- In a range pattern, the LHS and RHS must be known to be char or integral
- And they must be the same type

This change in ordering makes all the difference for the inferencer, of course, since it is able to propagate types from one side to the other. Does this sound correct?

jackh726 · 2021-09-09T15:10:03Z

To summarize the effects of this PR, I believe it implements the following logic:

When type checking a range pattern...

In a range pattern, the LHS and RHS must be of equal type

Further, those types must be known to be char or integral

whereas before the logic was

When type checking a range pattern...

In a range pattern, the LHS and RHS must be known to be char or integral

And they must be the same type

This change in ordering makes all the difference for the inferencer, of course, since it is able to propagate types from one side to the other. Does this sound correct?

That looks correct to me.

@nbdd0121 could you actually add that explanation as a comment? That makes it abundantly clear what's going on.

nbdd0121 · 2021-09-09T16:40:46Z

A more accurate description:

When type checking a range pattern...
- In a range pattern, if types of LHS and RHS are known, they must be char or integral
  - This is just for better diagnostics (see note 1 below).
- LHS, RHS and the expected type must all be the same type
- Further, those types must be known to be char or integral

whereas before the logic was

When type checking a range pattern...
- In a range pattern, the LHS and RHS must be known to be char or integral
- And LHS, RHS and the expected type must all be the same type

Note 1: the early check if LHS or RHS types are known is needed because range pattern will strip away references. So if the early check is omitted and we just start unifying types, then

match "A" {
  "A".."B" => (),
  _ => (),
}

will have LHS and RHS be &str while expected type is str, and the unification will fail and produce very terrible diagnostics. So in this case I just keep the current early check and let rustc complain that &str isn't char or integral rather than &str and str mismatches.

nbdd0121 · 2021-09-09T16:43:23Z

BTW one more thing: with this PR,

match Zero::ZERO {
	Zero::ZERO ..= Zero::ZERO => {},
	_ => {},
}

will still complain that _ is not char or integral like it does now. I wonder if I should change it to complain that "type must be known at this point" instead?

jackh726 · 2021-09-09T16:48:33Z

Note 1: the early check if LHS or RHS types are known is needed because range pattern will strip away references. So if the early check is omitted and we just start unifying types, then
match "A" {
  "A".."B" => (),
  _ => (),
}
will have LHS and RHS be &str while expected type is str, and the unification will fail and produce very terrible diagnostics. So in this case I just keep the current early check and let rustc complain that &str isn't char or integral rather than &str and str mismatches.

Interesting...can you make sure there is a test that covers this case?

BTW one more thing: with this PR,
match Zero::ZERO {
	Zero::ZERO ..= Zero::ZERO => {},
	_ => {},
}
will still complain that _ is not char or integral like it does now. I wonder if I should change it to complain that "type must be known at this point" instead?

What happens with non-range patterns?

nbdd0121 · 2021-09-09T16:54:57Z

Interesting...can you make sure there is a test that covers this case?

This is from a existing test case ^^

What happens with non-range patterns?

Non-range patterns do not need to inspect the type, so

match Zero::ZERO {
	Zero::ZERO => {},
	_ => {},
}

will just behave like

let _ = Zero::ZERO;

and gives a "type annotations needed" error.

jackh726 · 2021-09-09T17:26:04Z

I would say if the change to emit "type annotations needed" is small, that's preferable, so behavior lines up. If not, then I think it can be left as a followup FIXME

nbdd0121 · 2021-09-10T20:40:34Z

v1 -> v2:

add extra explanatory comments
add resolve_vars_if_possible to emit_err_pat_range
emit E0282 type annotations needed if type is _

nikomatsakis · 2021-09-21T18:06:58Z

@rfcbot fcp merge

We discussed this in the @rust-lang/lang meeting today. General consensus was that this change was quite reasonable; in order to change the assumption that L/R hand sides of .. are the same type, we'd have to alter the Range type to have multiple type parameters, which would be a very large change (and of course the assumption is already baked in, just enforced later).

rfcbot · 2021-09-21T18:06:59Z

nikomatsakis · 2021-09-21T18:07:17Z

@rfcbot fcp cancel

nikomatsakis · 2021-09-21T18:08:14Z

@rfcbot fcp merge

We discussed this in the @rust-lang/lang meeting today. General consensus was that this change was quite reasonable; in order to change the assumption that L/R hand sides of .. are the same type, we'd have to alter the Range type to have multiple type parameters, which would be a very large change (and of course the assumption is already baked in, just enforced later).

rfcbot · 2021-09-21T18:08:15Z

Team member @nikomatsakis has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

rfcbot · 2021-09-21T21:34:24Z

🔔 This is now entering its final comment period, as per the review above. 🔔

rfcbot · 2021-10-03T02:56:48Z

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

nikomatsakis · 2021-10-04T15:30:11Z

@bors r+

bors · 2021-10-04T15:30:13Z

📌 Commit 52a0403 has been approved by nikomatsakis

Perform type inference in range pattern Fix rust-lang#88074

…ingjubilee Rollup of 15 pull requests Successful merges: - rust-lang#87993 (Stabilize try_reserve) - rust-lang#88090 (Perform type inference in range pattern) - rust-lang#88780 (Added abs_diff for integer types.) - rust-lang#89270 (path.push() should work as expected on windows verbatim paths) - rust-lang#89413 (Correctly handle supertraits for min_specialization) - rust-lang#89456 (Update to the final LLVM 13.0.0 release) - rust-lang#89466 (Fix bug with query modifier parsing) - rust-lang#89473 (Fix extra `non_snake_case` warning for shorthand field bindings) - rust-lang#89474 (rustdoc: Improve doctest pass's name and module's name) - rust-lang#89478 (Fixed numerus of error message) - rust-lang#89480 (Add test for issue 89118.) - rust-lang#89487 (Try to recover from a `=>` -> `=` or `->` typo in a match arm) - rust-lang#89494 (Deny `where` clauses on `auto` traits) - rust-lang#89511 (:arrow_up: rust-analyzer) - rust-lang#89536 (update Miri) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

rust-highfive assigned jackh726 Aug 16, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 16, 2021

estebank reviewed Aug 18, 2021

View reviewed changes

inquisitivecrystal added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Aug 24, 2021

nikomatsakis approved these changes Sep 9, 2021

View reviewed changes

nbdd0121 added 3 commits September 10, 2021 21:28

Perform type inference in range pattern

d8dae4f

Add ui test for issue 88074

ca1616c

Add a range pattern inference failing test

52a0403

nbdd0121 force-pushed the inference branch from 353c039 to 52a0403 Compare September 10, 2021 20:38

rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Sep 21, 2021

nikomatsakis removed the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Sep 21, 2021

rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Sep 21, 2021

rfcbot added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels Sep 21, 2021

rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. to-announce Announce this issue on triage meeting and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Oct 3, 2021

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-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels Oct 4, 2021

Manishearth added a commit to Manishearth/rust that referenced this pull request Oct 5, 2021

Rollup merge of rust-lang#88090 - nbdd0121:inference, r=nikomatsakis

d1a40d7

Perform type inference in range pattern Fix rust-lang#88074

Manishearth mentioned this pull request Oct 5, 2021

Rollup of 18 pull requests #89540

Closed

Manishearth added a commit to Manishearth/rust that referenced this pull request Oct 5, 2021

Rollup merge of rust-lang#88090 - nbdd0121:inference, r=nikomatsakis

537d112

Perform type inference in range pattern Fix rust-lang#88074

Manishearth mentioned this pull request Oct 5, 2021

Rollup of 17 pull requests #89543

Closed

Manishearth added a commit to Manishearth/rust that referenced this pull request Oct 5, 2021

Rollup merge of rust-lang#88090 - nbdd0121:inference, r=nikomatsakis

d87f3f3

Perform type inference in range pattern Fix rust-lang#88074

Manishearth mentioned this pull request Oct 5, 2021

Rollup of 16 pull requests #89544

Closed

workingjubilee mentioned this pull request Oct 5, 2021

Rollup of 15 pull requests #89545

Merged

bors merged commit 4f6afee into rust-lang:master Oct 5, 2021

rustbot added this to the 1.57.0 milestone Oct 5, 2021

apiraino removed the to-announce Announce this issue on triage meeting label Oct 7, 2021

nbdd0121 deleted the inference branch October 7, 2021 21:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Perform type inference in range pattern #88090

Perform type inference in range pattern #88090

nbdd0121 commented Aug 16, 2021

rust-highfive commented Aug 16, 2021

jackh726 commented Aug 17, 2021

estebank left a comment

estebank Aug 18, 2021

estebank Aug 18, 2021

estebank Aug 18, 2021

jackh726 commented Sep 7, 2021

nikomatsakis commented Sep 9, 2021

jackh726 commented Sep 9, 2021

nbdd0121 commented Sep 9, 2021

nbdd0121 commented Sep 9, 2021

jackh726 commented Sep 9, 2021

nbdd0121 commented Sep 9, 2021

jackh726 commented Sep 9, 2021

nbdd0121 commented Sep 10, 2021

nikomatsakis commented Sep 21, 2021

rfcbot commented Sep 21, 2021 •

edited by cramertj

Loading

nikomatsakis commented Sep 21, 2021

nikomatsakis commented Sep 21, 2021

rfcbot commented Sep 21, 2021 •

edited by cramertj

Loading

rfcbot commented Sep 21, 2021

rfcbot commented Oct 3, 2021

nikomatsakis commented Oct 4, 2021

bors commented Oct 4, 2021

		\| \| this is of type `_` but it should be `char` or numeric
		\| this is of type `_` but it should be `char` or numeric

Perform type inference in range pattern #88090

Perform type inference in range pattern #88090

Conversation

nbdd0121 commented Aug 16, 2021

rust-highfive commented Aug 16, 2021

jackh726 commented Aug 17, 2021

estebank left a comment

Choose a reason for hiding this comment

estebank Aug 18, 2021

Choose a reason for hiding this comment

estebank Aug 18, 2021

Choose a reason for hiding this comment

estebank Aug 18, 2021

Choose a reason for hiding this comment

jackh726 commented Sep 7, 2021

nikomatsakis commented Sep 9, 2021

jackh726 commented Sep 9, 2021

nbdd0121 commented Sep 9, 2021

nbdd0121 commented Sep 9, 2021

jackh726 commented Sep 9, 2021

nbdd0121 commented Sep 9, 2021

jackh726 commented Sep 9, 2021

nbdd0121 commented Sep 10, 2021

nikomatsakis commented Sep 21, 2021

rfcbot commented Sep 21, 2021 • edited by cramertj Loading

nikomatsakis commented Sep 21, 2021

nikomatsakis commented Sep 21, 2021

rfcbot commented Sep 21, 2021 • edited by cramertj Loading

rfcbot commented Sep 21, 2021

rfcbot commented Oct 3, 2021

nikomatsakis commented Oct 4, 2021

bors commented Oct 4, 2021

rfcbot commented Sep 21, 2021 •

edited by cramertj

Loading

rfcbot commented Sep 21, 2021 •

edited by cramertj

Loading