Perform type inference in range pattern by nbdd0121 · Pull Request #88090 · rust... - JOYK Joy of Geek, Geek News, Link all geek

Copy link

Contributor

rust-highfive commented on Aug 16

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

Copy link

Contributor

jackh726 commented on Aug 17

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)

Copy link

Contributor

estebank left a comment

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.

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

}

impl Zero for String {

const ZERO: Self = String::new();

Copy link

Contributor

estebank on Aug 18

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

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

Outdated

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

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

Copy link

Contributor

estebank on Aug 18

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

compiler/rustc_typeck/src/check/pat.rs

if let Some((ref mut fail, _, _)) = rhs {

*fail = true;

}

self.emit_err_pat_range(span, lhs, rhs);

Copy link

Contributor

estebank on Aug 18

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 _.

Copy link

Contributor

jackh726 commented on Sep 7

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.

Copy link

Contributor

nikomatsakis commented on Sep 9

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?

Copy link

Contributor

jackh726 commented on Sep 9

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.

Copy link

Contributor

Author

nbdd0121 commented on Sep 9

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.

Copy link

Contributor

Author

nbdd0121 commented on Sep 9

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?

Copy link

Contributor

jackh726 commented on Sep 9

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?

Copy link

Contributor

Author

nbdd0121 commented on Sep 9

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.

Copy link

Contributor

jackh726 commented on Sep 9

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

Copy link

Contributor

Author

nbdd0121 commented on Sep 10

v1 -> v2:

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

Copy link

Contributor

nikomatsakis commented 19 days ago

@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).

Copy link

rfcbot commented 19 days ago •

edited by cramertj

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.

Copy link

Contributor