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

Check method input expressions once #94438

Merged
merged 1 commit into from
Mar 1, 2022

Conversation

compiler-errors
Copy link
Member

@compiler-errors compiler-errors commented Feb 28, 2022

If the user mistakenly forgets to wrap their method args in a tuple, then the compiler tries to check that types within the tuple match the expression args. This means we call check_expr once within this diagnostic code, so when we check the expr once again in demand_compatible, we attempt to apply expr adjustments twice, leading to ICEs.

This PR attempts to fix this by skipping the expression type check in demand_compatible if we have detected an method arg mismatch at all.

This does lead to a single UI test regressing slightly, due to a diagnostic disappearing, though I don't know if it is generally meaningful to even raise an type error after noting that the argument count is incorrect in a function call, since the user might be providing (in-context) meaningless expressions to the wrong method.

I can adjust this to be a bit more targeted (to just skip checking exprs in demand_compatible in the tuple case) if this UI test regression is a problem.

fixes #94334
cc #94291

Also drive-by fixup of .node_type(expr.hir_id) to .expr_ty(expr), since that method exists.

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Feb 28, 2022
@rust-highfive
Copy link
Collaborator

r? @michaelwoerister

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 28, 2022
@rust-log-analyzer

This comment has been minimized.

@michaelwoerister
Copy link
Member

Thanks for the PR, @compiler-errors!

r? rust-lang/diagnostics

Copy link
Member

@davidtwco davidtwco left a comment

Choose a reason for hiding this comment

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

I think this seems reasonable and the slight regression on overloaded-calls-bad is okay.

@davidtwco
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Mar 1, 2022

📌 Commit 7543067 has been approved by davidtwco

@bors 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-review Status: Awaiting review from the assignee but also interested parties. labels Mar 1, 2022
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 1, 2022
…askrgr

Rollup of 3 pull requests

Successful merges:

 - rust-lang#94438 (Check method input expressions once)
 - rust-lang#94459 (Update cargo)
 - rust-lang#94470 (:arrow_up: rust-analyzer)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 2353e83 into rust-lang:master Mar 1, 2022
@rustbot rustbot added this to the 1.61.0 milestone Mar 1, 2022
@ouz-a ouz-a mentioned this pull request Mar 1, 2022
@camelid camelid added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Mar 2, 2022
@camelid
Copy link
Member

camelid commented Mar 2, 2022

I'm not sure if this makes sense to backport or not, but I nominated it just in case.

@camelid
Copy link
Member

camelid commented Mar 2, 2022

Actually, this PR seems to have led to an ICE of its own, so it probably doesn't make sense to backport it: #94516

@compiler-errors
Copy link
Member Author

Yeah, can't be backported until that one is fixed too. I can investigate it soon.

Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Mar 4, 2022
…plicate, r=estebank

Delay bug in expr adjustment when check_expr is called multiple times

Instead of including slightly more complicated logic in `check_argument_types` to fix the bug (rust-lang#94516) I introduced in rust-lang#94438, and inevitably have this bug appear once again when some other diagnostic is written that causes `check_expr` to be called an expression during a (bad) code path, just delay the bug in adjustment logic.

I am open to other implementations that don't delay the bug here.

Fixes rust-lang#94516
@apiraino
Copy link
Contributor

Beta backport approved as per compiler team on Zulip

@rustbot label +beta-accepted

@rustbot rustbot added the beta-accepted Accepted for backporting to the compiler in the beta channel. label Mar 10, 2022
@Mark-Simulacrum Mark-Simulacrum modified the milestones: 1.61.0, 1.60.0 Mar 14, 2022
@Mark-Simulacrum Mark-Simulacrum removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Mar 14, 2022
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 14, 2022
…ulacrum

[beta] backports

* Update LLVM submodule rust-lang#94764
* Statically compile libstdc++ everywhere if asked rust-lang#94719
* Downgrade #[test] on macro call to warning rust-lang#94624
* Delay bug in expr adjustment when check_expr is called multiple times rust-lang#94596
* bootstrap: correct reading of flags for llvm rust-lang#94466
* Check method input expressions once rust-lang#94438
* remove feature gate in control_flow examples rust-lang#94283

r? `@Mark-Simulacrum`
compiler-errors added a commit to compiler-errors/rust that referenced this pull request Mar 18, 2022
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 22, 2022
…k-Simulacrum

[beta] Remove statement that was forgotten when backporting rust-lang#94596

This `if` statement was introduced in rust-lang#94438, then removed in rust-lang#94596. Both of these PRs were beta-backported in rust-lang#94933, but I think there was a mistake in the order they were applied or this removal was overlooked. I think this fixes the remaining issues referenced in rust-lang#94511 (comment).

Not sure this is the correct way to put something up for beta-backport, but the PR is at least open so it can be referenced and the commit can be cherry-picked. Feel free to close this PR.

r? `@Mark-Simulacrum`
cc: rust-lang#94511
@compiler-errors compiler-errors deleted the check-method-inputs-once branch April 7, 2022 04:33
MabezDev pushed a commit to esp-rs/rust that referenced this pull request Apr 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
beta-accepted Accepted for backporting to the compiler in the beta channel. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Compiler crashes on a piece of code that misses some parenthesis around a tuple contained in Result::Ok
10 participants