fix: Nail destructuring assignment once and for all #18254
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rustbot
added
the
S-waiting-on-review
ChayimFriedman2
force-pushed
the
fix-mut
branch
from
84c11b4 to
632a21d
Compare
|
☔ The latest upstream changes (presumably #18252) made this pull request unmergeable. Please resolve the merge conflicts. |
ChayimFriedman2
force-pushed
the
fix-mut
branch
from
632a21d to
34bf156
Compare
|
☔ The latest upstream changes (presumably #18278) made this pull request unmergeable. Please resolve the merge conflicts. |
ChayimFriedman2
force-pushed
the
fix-mut
branch
from
34bf156 to
28955c3
Compare
|
☔ The latest upstream changes (presumably #18350) made this pull request unmergeable. Please resolve the merge conflicts. |
Because our lint infra *can* handle allows from within macro expansions! (Also, what did this reason have to do with something that is a hard error and not a lint? I'm puzzled). I wonder how many such diagnostics we have... Maybe that also mean we can change `unused_mut` to no-longer-experimental? But this is a change I'm afraid to do without checking.
Instead of lowering them to `<expr> = <expr>`, then hacking on-demand to resolve them, we lower them to `<pat> = <expr>`, and use the pattern infrastructure to handle them. It turns out, destructuring assignments are surprisingly similar to pattern bindings, and so only minor modifications are needed. This fixes few bugs that arose because of the non-uniform handling (for example, MIR lowering not handling slice and record patterns, and closure capture calculation not handling destructuring assignments at all), and furthermore, guarantees we won't have such bugs in the future, since the programmer will always have to explicitly handle `Expr::Assignment`. Tests don't pass yet; that's because the generated patterns do not exist in the source map. The next commit will fix that.
And few more fixups. I was worried this will lead to more memory usage since `ExprOrPatId` is double the size of `ExprId`, but this does not regress `analysis-stats .`. If this turns out to be a problem, we can easily use the high bit to encode this information.
ChayimFriedman2
force-pushed
the
fix-mut
branch
from
28955c3 to
2d4d6b6
Compare
Veykril
reviewed
Oct 22, 2024
| @@ -32,15 +32,15 @@ pub fn missing_unsafe(db: &dyn HirDatabase, def: DefWithBodyId) -> (Vec<ExprId>, | |||
| let infer = db.infer(def); | |||
| unsafe_expressions(db, &infer, def, &body, body.body_expr, &mut |expr| { | |||
| if !expr.inside_unsafe_block { | |||
| res.push(expr.expr); | |||
| res.push(expr.node); | |||
| } | |||
| }); | |||
|
|
|||
| (res, is_unsafe) | |||
| } | |||
|
|
|||
| pub struct UnsafeExpr { | |||
There was a problem hiding this comment.
Reading this made me realize we have yet to add checking patterns for unsafety here
union U {
a: u8,
b: u32,
}
fn f(U { a }: U) {}for example doesn't trigger a warning (given the changes here I imagine that should be doable in a nicer way than before even). That struct should also be renamed then to UnsafeOp or so
Either way not to be done in this PR given its size.
There was a problem hiding this comment.
Ah I just realiezd there is a FIXME on line 47 regarding patterns
| lower::lower_to_chalk_mutability, | ||
| primitive::UintTy, | ||
| static_lifetime, InferenceDiagnostic, Interner, Mutability, Scalar, Substitution, Ty, | ||
| TyBuilder, TyExt, TyKind, | ||
| }; | ||
|
|
||
| /// Used to generalize patterns and assignee expressions. | ||
| pub(super) trait PatLike: Into<ExprOrPatId> + Copy { |
|
@Veykril Added a test as requested. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
By lowering it into patterns instead of expressions.
This is a large PR (phew!) but it also removes a lot of code, almost as much as it inserts. All present and probably even future code dedicated to handling destructuring assignments is thrown out of the window. Besides fixing a bunch of bugs, this also means that any future expansion is going to work much less to work with destructuring assignments, and is much more likely to not have bugs/rough edges/forgotten cases.
rustc goes even more than this and desugars destructuring assignment completely; coming to the HIR it vanishes. But we cannot do that, because this will severely affect our ability to map destructuring assignments up and down the lowering chain (for instance, consider what happens when a macro expands to the "pattern". With rustc's approach, the macro expansion will be split, and we have no way to represent that).
Fixes #18209.