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.

Sign up for GitHub

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

Jump to bottom

Do not call check_expr in check_compatible, since it has side-effects #98785

Merged
merged 1 commit into from
Jul 10, 2022
Merged

Do not call check_expr in check_compatible, since it has side-effects #98785

merged 1 commit into from
Jul 10, 2022

Conversation

compiler-errors
Copy link
Member

@compiler-errors compiler-errors commented Jul 1, 2022 •
edited
Loading

Fixes a weird suggestion in #98784

found later:
Fixes #98894
Fixes #98897

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

r? @lcnr

(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 Jul 1, 2022
This was referenced Jul 1, 2022
Merged
Closed
@compiler-errors compiler-errors force-pushed the no-check-expr-in-check-compatible branch from d6c26d7 to e1770a2 Compare July 4, 2022 17:51
@compiler-errors
Copy link
Member Author

r? @jackh726 do you mind taking a look at this? it actually ends up making the code slightly simpler, too.

this ICEs in production found by @WaffleLapkin, so I'm also gonna beta nominate this since it's a pretty simple to understand and self-contained fix.

@rust-highfive rust-highfive assigned jackh726 and unassigned lcnr Jul 4, 2022
@compiler-errors compiler-errors added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Jul 4, 2022
@compiler-errors
Copy link
Member Author

well, I guess it didn't ICE in the motivating example that Waffle originally found... but idk. Worth nominating at least. Hopefully this is the last of the arg mismatch algorithm issues, hehe.

@apiraino
Copy link
Contributor

apiraino commented Jul 7, 2022

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 Jul 7, 2022
@bors
Copy link
Contributor

bors commented Jul 8, 2022

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

estebank
estebank reviewed Jul 8, 2022
View reviewed changes
src/test/ui/issues/issue-3044.rs Outdated
//~^^ ERROR mismatched types
//~| ERROR this function takes 2 arguments but 1 argument was supplied
//~^ ERROR this function takes 2 arguments but 1 argument was supplied
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Incorrect formatting

Suggested change
});
});

@estebank
Copy link
Contributor

estebank commented Jul 8, 2022

r=me after rebase

@compiler-errors compiler-errors force-pushed the no-check-expr-in-check-compatible branch from e1770a2 to 6858fbc Compare July 9, 2022 20:50
@compiler-errors
Copy link
Member Author

@bors r=estebank

@bors
Copy link
Contributor

bors commented Jul 9, 2022

📌 Commit 6858fbc has been approved by estebank

It is now in the queue for this repository.

@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 Jul 9, 2022
@Mark-Simulacrum
Copy link
Member

@bors p=1 -- beta-nominated

@bors
Copy link
Contributor

bors commented Jul 10, 2022

⌛ Testing commit 6858fbc with merge c6ff90b...

@bors
Copy link
Contributor

bors commented Jul 10, 2022

☀️ Test successful - checks-actions
Approved by: estebank
Pushing c6ff90b to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jul 10, 2022
@bors bors merged commit c6ff90b into rust-lang:master Jul 10, 2022
@rustbot rustbot added this to the 1.64.0 milestone Jul 10, 2022
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (c6ff90b): comparison url.

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

This benchmark run did not return any relevant results for this metric.

Cycles

This benchmark run did not return any relevant results for this metric.

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

@Mark-Simulacrum
Copy link
Member

@compiler-errors it looks like this doesn't backport to the beta branch cleanly. Can you prepare a beta-branch targeted PR with this PR's changes that we can review and merge?

@compiler-errors
Copy link
Member Author

I can do that. Thanks for the heads up.

@compiler-errors
Copy link
Member Author

Oh, yeah, this definitely doesn't cleanly backport.

For some reason I had thought that the PR this is building off (#97542) was landing in 1.63, not 1.64. I will have to use a different solution for 1.63.

This was referenced Jul 18, 2022
Merged
Closed
@compiler-errors compiler-errors removed beta-nominated Nominated for backporting to the compiler in the beta channel. beta-accepted Accepted for backporting to the compiler in the beta channel. labels Jul 18, 2022
@compiler-errors
Copy link
Member Author

Beta backport will be done in separate PR with another approval, so removing tags

GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Jul 23, 2022
@GuillaumeGomez
Rollup merge of rust-lang#99396 - compiler-errors:missing-tests, r=Ma…
93f58b6 
…rk-Simulacrum

Add some additional double-adjustment regression tests

I accidentally missed these when I rebased rust-lang#98785

cc rust-lang#98894 and rust-lang#98897
@compiler-errors compiler-errors deleted the no-check-expr-in-check-compatible branch August 11, 2023 20:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wesleywiser wesleywiser wesleywiser left review comments

@estebank estebank estebank left review comments

Assignees

@jackh726 jackh726

Labels
merged-by-bors This PR was explicitly merged by bors. 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
Milestone
1.64.0