Skip to content

Tighten up assignment operator representations. - #138017

Merged
bors merged 3 commits into
rust-lang:masterfrom
nnethercote:tighten-assignment-op
Apr 4, 2025
Merged

Tighten up assignment operator representations.#138017
bors merged 3 commits into
rust-lang:masterfrom
nnethercote:tighten-assignment-op

Conversation

@nnethercote

@nnethercotennethercote commented Mar 4, 2025

Copy link
Copy Markdown
Contributor

This is step 3 of MCP 831.

r? @spastorino

@rustbotrustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Mar 4, 2025
@rustbot

rustbot commented Mar 4, 2025

Copy link
Copy Markdown
Collaborator

Some changes occurred in src/tools/rustfmt

cc @rust-lang/rustfmt

This PR changes MIR

cc @oli-obk, @RalfJung, @JakobDegen, @davidtwco, @vakaras

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

@nnethercote

nnethercote commented Mar 4, 2025

Copy link
Copy Markdown
ContributorAuthor

@spastorino: I'm not 100% certain we should do this. It does make the representation better in the sense that it makes it impossible to express some nonsensical combinations, such as ExprKind::AssignOp(BinOpKind::Lt) (there is no &&= operator). But it makes the code longer, and we have to do some extra type conversions in a few places.

I suggest starting with the third commit, which is the most important one. If you are happy with how it looks, then look at the first and second commits, which are just some preliminary refactorings.

@nnethercotennethercote mentioned this pull request Mar 4, 2025
Closed
@bors

bors commented Mar 15, 2025

Copy link
Copy Markdown
Collaborator

☔ The latest upstream changes (presumably #138464) made this pull request unmergeable. Please resolve the merge conflicts.

@nnethercote
nnethercoteforce-pushed the tighten-assignment-op branch from 74227e1 to 7d1e5d0CompareMarch 16, 2025 21:39
@nnethercote

nnethercote commented Mar 16, 2025

Copy link
Copy Markdown
ContributorAuthor

I rebased.

@bors

bors commented Mar 19, 2025

Copy link
Copy Markdown
Collaborator

☔ The latest upstream changes (presumably #138653) made this pull request unmergeable. Please resolve the merge conflicts.

@nnethercote
nnethercoteforce-pushed the tighten-assignment-op branch from 7d1e5d0 to e43b280CompareMarch 19, 2025 20:12
@nnethercote

nnethercote commented Mar 19, 2025

Copy link
Copy Markdown
ContributorAuthor

@spastorino is busy and asked for a different reviewer, so let's try:

r? @estebank

@rustbotrustbot assigned estebank and unassigned spastorinoMar 19, 2025
@nnethercote

nnethercote commented Mar 19, 2025

Copy link
Copy Markdown
ContributorAuthor

From above, for the reviewer:

I suggest starting with the third commit, which is the most important one. If you are happy with how it looks, then look at the first and second commits, which are just some preliminary refactorings.

@bors

bors commented Mar 21, 2025

Copy link
Copy Markdown
Collaborator

☔ The latest upstream changes (presumably #138747) made this pull request unmergeable. Please resolve the merge conflicts.

spastorino
spastorino reviewed Apr 2, 2025
View reviewed changes
Comment threadcompiler/rustc_hir_typeck/src/op.rs Outdated
let by_ref_binop = !op.node.is_by_value();
if is_assign == IsAssign::Yes || by_ref_binop {
let by_ref_binop = !op.is_by_value();
if op.is_assign() || by_ref_binop {

@spastorinospastorinoApr 2, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if I prefer is_assign or just an explicit matches! checking the type. Whatever you prefer :).

@nnethercotennethercoteApr 2, 2025

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I switched to matches!/if let.

@spastorino

spastorino commented Apr 2, 2025

Copy link
Copy Markdown
Member

@nnethercote r=me after rebasing. I've made a minor comment but do whatever you feel is better :).

@nnethercote
nnethercoteforce-pushed the tighten-assignment-op branch from e43b280 to 98b46f5CompareApril 2, 2025 23:18
@nnethercote
Remove Op type.
076cccd
First, move the `lang_item_for_op` call from the top of
`lookup_op_method`'s body to its callsites. It makes those callsites a
little more verbose, but also means `lookup_op_method` no longer cares
whether it's handling a binop or unop. This lets us remove `Op` and
split `lang_item_for_op` into `lang_item_for_{bin,un}op`, which is a
little simpler.
This change is a prerequisite for adding the `ast::AssignOpKind` type in
a subsequent commit.
@nnethercote
Use BinOpKind instead of BinOp for function args where possible.
ac8ccf0
Because it's nice to avoid passing in unnecessary data.
@nnethercote
Tighten up assignment operator representations.
ddcb370
In the AST, currently we use `BinOpKind` within `ExprKind::AssignOp` and
`AssocOp::AssignOp`, even though this allows some nonsensical
combinations. E.g. there is no `&&=` operator. Likewise for HIR and
THIR.
This commit introduces `AssignOpKind` which only includes the ten
assignable operators, and uses it in `ExprKind::AssignOp` and
`AssocOp::AssignOp`. (And does similar things for `hir::ExprKind` and
`thir::ExprKind`.) This avoids the possibility of nonsensical
combinations, as seen by the removal of the `bug!` case in
`lang_item_for_binop`.
The commit is mostly plumbing, including:
- Adds an `impl From<AssignOpKind> for BinOpKind` (AST) and `impl
From<AssignOp> for BinOp` (MIR/THIR).
- `BinOpCategory` can now be created from both `BinOpKind` and
`AssignOpKind`.
- Replaces the `IsAssign` type with `Op`, which has more information and
a few methods.
- `suggest_swapping_lhs_and_rhs`: moves the condition to the call site,
it's easier that way.
- `check_expr_inner`: had to factor out some code into a separate
method.
I'm on the fence about whether avoiding the nonsensical combinations is
worth the extra code.
@nnethercote
nnethercoteforce-pushed the tighten-assignment-op branch from 98b46f5 to ddcb370CompareApril 2, 2025 23:34
@nnethercote

nnethercote commented Apr 2, 2025

Copy link
Copy Markdown
ContributorAuthor

I rebased.

@bors r=spastorino

@bors

bors commented Apr 2, 2025

Copy link
Copy Markdown
Collaborator

📌 Commit ddcb370 has been approved by spastorino

It is now in the queue for this repository.

@borsbors 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 2, 2025
@matthiaskrgrmatthiaskrgr mentioned this pull request Apr 3, 2025
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 3, 2025
@bors
Auto merge ofrust-lang#139336- matthiaskrgr:rollup-zsi8pgf, r=matth…
4fd8c04
…iaskrgr
Rollup of 9 pull requests
Successful merges:
- rust-lang#138017 (Tighten up assignment operator representations.)
- rust-lang#138462 (Dedup `&mut *` reborrow suggestion in loops)
- rust-lang#138610 (impl !PartialOrd for HirId)
- rust-lang#138767 (Allow boolean literals in `check-cfg`)
- rust-lang#139068 (io: Avoid marking some bytes as uninit)
- rust-lang#139255 (Remove unused variables generated in merged doctests)
- rust-lang#139270 (Add a mailmap entry for myself)
- rust-lang#139303 (Put Noratrieb on vacation)
- rust-lang#139312 (add Marco Ieni to mailmap)
r? `@ghost`
`@rustbot` modify labels: rollup
@bors
bors merged commit e5c7451 into rust-lang:masterApr 4, 2025
@rustbotrustbot added this to the 1.88.0 milestone Apr 4, 2025
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Apr 4, 2025
@rust-timer
Unrolled build forrust-lang#138017
b78066d
Rollup merge of rust-lang#138017 - nnethercote:tighten-assignment-op, r=spastorino
Tighten up assignment operator representations.
This is step 3 of [MCP 831](rust-lang/compiler-team#831).
r? `@spastorino`
@nnethercote
nnethercote deleted the tighten-assignment-op branch April 4, 2025 01:55
@dtolnaydtolnay mentioned this pull request Apr 5, 2025
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@spastorinospastorinospastorino left review comments

Assignees

@estebankestebank

Labels

S-waiting-on-borsStatus: Waiting on bors to run and complete tests. Bors will change the label on completion.T-compilerRelevant to the compiler team, which will review and decide on the PR/issue.

Projects

None yet

Milestone

1.88.0

Development

Successfully merging this pull request may close these issues.

5 participants

@nnethercote@rustbot@bors@spastorino@estebank