Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Correct inference of primitive operand type behind binary operation #68129

Merged
merged 2 commits into from Feb 15, 2020

Conversation

@varkor
Copy link
Member

varkor commented Jan 11, 2020

Fixes #57447.

r? @nikomatsakis

src/librustc_typeck/check/op.rs Outdated Show resolved Hide resolved
@varkor varkor force-pushed the varkor:infer-binary-operand-behind-reference branch from 932763a to 056d48a Jan 11, 2020
@Centril Centril added this to the 1.42 milestone Jan 12, 2020
@Centril Centril added the I-nominated label Jan 12, 2020
@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Jan 13, 2020

The code seems fine, but I think we probably ought to document the behavior of this part of the type-checker better. This effects the lang spec in the end. Perhaps the reference is the right place? I don't think we have a lot of "framework" for doing so, but I think it's probably a good idea for us to start collecting these sorts of things in the reference.

This is tricky, though, as we don't have any sort of "framework", and it's hard to describe the structure of the rules without that. Still, I wouldn't want to get too hung up on (e.g) figuring out the best way to write typing judgements.

Maybe we should start by documenting in rustc-guide or in an open issue to start?

@varkor

This comment has been minimized.

Copy link
Member Author

varkor commented Jan 14, 2020

I've written a comment on a relevant issue in the Rust reference repository.

@varkor

This comment has been minimized.

Copy link
Member Author

varkor commented Jan 14, 2020

I find it slightly surprising that x + y and &x + &y work, but &&x + &&y (and similar) do not. This is due to

// implements binary operators "&T op U", "T op &U", "&T op &U"
// based on "T op U" where T and U are expected to be `Copy`able
macro_rules! forward_ref_binop {

which seems a little ad hoc (added in #21227).

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Jan 15, 2020

It seems a bit surprising, but then those are the impls that exist, right? (i.e., we don't have a general impl for &T, just for specific types?)

@varkor

This comment has been minimized.

Copy link
Member Author

varkor commented Jan 15, 2020

Yes, that's right. It's related, but only tangentially in that the cases look similar from a user's perspective.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Jan 16, 2020

@rfcbot fcp merge

Discussed in today's @rust-lang/lang meeting. The consensus of the meeting was that we should make this change, but we're doing an fcp merge to make it official.

We did think it would be good to add some more tests:

  • To cover other sorts of operators, like <<
  • To test cases where the integer is "indirect", like this one
  • To test cases where there is no expected type (which we expect to error)
@rfcbot

This comment has been minimized.

Copy link

rfcbot commented Jan 16, 2020

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

This comment has been minimized.

Copy link

rfcbot commented Jan 23, 2020

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

@rfcbot

This comment has been minimized.

Copy link

rfcbot commented Feb 2, 2020

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.

The RFC will be merged soon.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Feb 3, 2020

@bors r+

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Feb 3, 2020

📌 Commit 056d48a has been approved by nikomatsakis

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Feb 3, 2020

@bors r-

Wait, @varkor, did you add the extra tests we mentioned in the meeting...?

@varkor

This comment has been minimized.

Copy link
Member Author

varkor commented Feb 3, 2020

@nikomatsakis: no, I was waiting for the FCP to complete. I'll do so soon.

@varkor

This comment has been minimized.

Copy link
Member Author

varkor commented Feb 9, 2020

To test cases where there is no expected type (which we expect to error)

Just to clarify, do you mean cases like the following?

let _ = 0 + &0;

This currently passes, as it defaults to i32. Or was a different case supposed to error?

I've added the suggested test cases.

@varkor varkor force-pushed the varkor:infer-binary-operand-behind-reference branch from 056d48a to 0276d7a Feb 9, 2020
@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Feb 14, 2020

Good question @varkor, what did I mean... I'm not sure, but the tests seem good.

@bors r+

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Feb 14, 2020

📌 Commit 0276d7a has been approved by nikomatsakis

JohnTitor added a commit to JohnTitor/rust that referenced this pull request Feb 14, 2020
…reference, r=nikomatsakis

Correct inference of primitive operand type behind binary operation

Fixes rust-lang#57447.

r? @nikomatsakis
bors added a commit that referenced this pull request Feb 14, 2020
Rollup of 7 pull requests

Successful merges:

 - #68129 (Correct inference of primitive operand type behind binary operation)
 - #68475 (Use a `ParamEnvAnd<Predicate>` for caching in `ObligationForest`)
 - #68856 (typeck: clarify def_bm adjustments & add tests for or-patterns)
 - #69051 (simplify_try: address some of eddyb's comments)
 - #69128 (Fix extra subslice lowering)
 - #69150 (Follow-up to #68848)
 - #69164 (Update pulldown-cmark dependency)

Failed merges:

r? @ghost
@bors bors merged commit 0276d7a into rust-lang:master Feb 15, 2020
4 checks passed
4 checks passed
pr Build #20200209.7 succeeded
Details
pr (Linux mingw-check) Linux mingw-check succeeded
Details
pr (Linux x86_64-gnu-llvm-7) Linux x86_64-gnu-llvm-7 succeeded
Details
pr (Linux x86_64-gnu-tools) Linux x86_64-gnu-tools succeeded
Details
@varkor varkor deleted the varkor:infer-binary-operand-behind-reference branch Feb 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

7 participants
You can’t perform that action at this time.